From 39a3fbacf28142c8fd2615d4e07bdd2d489b1da4 Mon Sep 17 00:00:00 2001 From: Aaron Feickert <66188213+AaronFeickert@users.noreply.github.com> Date: Mon, 15 Apr 2024 05:50:46 -0500 Subject: [PATCH] fix!: avoid `Encryptable` domain collisions (#6275) Description --- Updates a few `Encryptable` implementations to make domains canonical. Closes #6274. Motivation and Context --- As noted in #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. --- .../storage/sqlite_db/key_manager_state.rs | 26 ++++++++++++++----- .../wallet/src/storage/sqlite_db/wallet.rs | 12 ++++++--- 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/base_layer/key_manager/src/key_manager_service/storage/sqlite_db/key_manager_state.rs b/base_layer/key_manager/src/key_manager_service/storage/sqlite_db/key_manager_state.rs index 888744edb3..be569018c5 100644 --- a/base_layer/key_manager/src/key_manager_service/storage/sqlite_db/key_manager_state.rs +++ b/base_layer/key_manager/src/key_manager_service/storage/sqlite_db/key_manager_state.rs @@ -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::{ @@ -147,9 +147,15 @@ impl KeyManagerStateSql { impl Encryptable for KeyManagerStateSql { fn domain(&self, field_name: &'static str) -> Vec { - [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 { @@ -172,9 +178,15 @@ impl Encryptable for KeyManagerStateSql { impl Encryptable for NewKeyManagerStateSql { fn domain(&self, field_name: &'static str) -> Vec { - [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 { diff --git a/base_layer/wallet/src/storage/sqlite_db/wallet.rs b/base_layer/wallet/src/storage/sqlite_db/wallet.rs index 5be73d2888..74b073b711 100644 --- a/base_layer/wallet/src/storage/sqlite_db/wallet.rs +++ b/base_layer/wallet/src/storage/sqlite_db/wallet.rs @@ -954,9 +954,15 @@ impl ClientKeyValueSql { impl Encryptable for ClientKeyValueSql { fn domain(&self, field_name: &'static str) -> Vec { - [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)]