Skip to content

Commit

Permalink
fix!: avoid Encryptable domain collisions (tari-project#6275)
Browse files Browse the repository at this point in the history
Description
---
Updates a few `Encryptable` implementations to make domains canonical.

Closes tari-project#6274.

Motivation and Context
---
As noted in tari-project#6274, concatenation of variable-length data when generating
`Encryptable` domains for AEAD associated data is not canonical for
several types, and could result in collisions.

This PR updates the affected implementations to use fixed-size length
prepending.

How Has This Been Tested?
---
Existing tests.

What process can a PR reviewer use to test or verify this change?
---
Ensure that all `Encryptable` implementations have canonical domain
encodings that cannot collide.

BREAKING CHANGE: Affects the way that local encrypted data is
authenticated, so existing encrypted databases will not function
correctly.
  • Loading branch information
AaronFeickert authored Apr 15, 2024
1 parent b5f2c46 commit 39a3fba
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use chrono::{NaiveDateTime, Utc};
use diesel::{prelude::*, SqliteConnection};
use tari_common_sqlite::util::diesel_ext::ExpectedRowsExtension;
use tari_common_types::encryption::{decrypt_bytes_integral_nonce, encrypt_bytes_integral_nonce};
use tari_utilities::Hidden;
use tari_utilities::{ByteArray, Hidden};

use crate::{
key_manager_service::{
Expand Down Expand Up @@ -147,9 +147,15 @@ impl KeyManagerStateSql {

impl Encryptable<XChaCha20Poly1305> for KeyManagerStateSql {
fn domain(&self, field_name: &'static str) -> Vec<u8> {
[Self::KEY_MANAGER, self.branch_seed.as_bytes(), field_name.as_bytes()]
.concat()
.to_vec()
// Because there are two variable-length inputs in the concatenation, we prepend the length of the first
[
Self::KEY_MANAGER,
(self.branch_seed.len() as u64).to_le_bytes().as_bytes(),
self.branch_seed.as_bytes(),
field_name.as_bytes(),
]
.concat()
.to_vec()
}

fn encrypt(mut self, cipher: &XChaCha20Poly1305) -> Result<Self, String> {
Expand All @@ -172,9 +178,15 @@ impl Encryptable<XChaCha20Poly1305> for KeyManagerStateSql {

impl Encryptable<XChaCha20Poly1305> for NewKeyManagerStateSql {
fn domain(&self, field_name: &'static str) -> Vec<u8> {
[Self::KEY_MANAGER, self.branch_seed.as_bytes(), field_name.as_bytes()]
.concat()
.to_vec()
// Because there are two variable-length inputs in the concatenation, we prepend the length of the first
[
Self::KEY_MANAGER,
(self.branch_seed.len() as u64).to_le_bytes().as_bytes(),
self.branch_seed.as_bytes(),
field_name.as_bytes(),
]
.concat()
.to_vec()
}

fn encrypt(mut self, cipher: &XChaCha20Poly1305) -> Result<Self, String> {
Expand Down
12 changes: 9 additions & 3 deletions base_layer/wallet/src/storage/sqlite_db/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -954,9 +954,15 @@ impl ClientKeyValueSql {

impl Encryptable<XChaCha20Poly1305> for ClientKeyValueSql {
fn domain(&self, field_name: &'static str) -> Vec<u8> {
[Self::CLIENT_KEY_VALUE, self.key.as_bytes(), field_name.as_bytes()]
.concat()
.to_vec()
// Because there are two variable-length inputs in the concatenation, we prepend the length of the first
[
Self::CLIENT_KEY_VALUE,
(self.key.len() as u64).to_le_bytes().as_bytes(),
self.key.as_bytes(),
field_name.as_bytes(),
]
.concat()
.to_vec()
}

#[allow(unused_assignments)]
Expand Down

0 comments on commit 39a3fba

Please sign in to comment.