fix: hide sensitive data on tari repo (see issue #4846) #4967

jorgeantonio21 · 2022-11-28T12:32:44Z

Description

The goal of this PR is to zeroize sensitive data content across the Tari repo. We pay special attention to kdf key data and other occurrences of sensitive data. For more details, we refer to issue #4846. Moreover, this PR is a companion to #4953 and #4925

Motivation and Context

Tackle issue #4846.

How Has This Been Tested?

sdbondi

LGTM

I'm guessing there is still a leak at the clap level. Doesn't look like they support marking sensitive args.

applications/tari_console_wallet/src/lib.rs

comms/core/src/noise/mod.rs

SWvheerden

utACK

AaronFeickert · 2022-12-02T16:56:54Z

base_layer/core/src/transactions/transaction_components/encrypted_value.rs


+use super::{CoreTransactionAEADKey, AEAD_KEY_LEN};


Might consider renaming to something that describes the intent more clearly, like ValueEncryptionKey. But that's just a nit.

Done in PR 4994.

AaronFeickert · 2022-12-02T17:15:04Z

base_layer/core/src/transactions/transaction_components/encrypted_value.rs

-    const AEAD_KEY_LENGTH: usize = 32; // The length in bytes of a ChaCha20-Poly1305 AEAD key
+fn kdf_aead(shared_secret: &PrivateKey, commitment: &Commitment) -> CoreTransactionAEADKey {
    let output = DomainSeparatedHasher::<Blake256, TransactionKdfDomain>::new_with_label("encrypted_value")
        .chain(shared_secret.as_bytes())
        .chain(commitment.as_bytes())
        .finalize();

-    *Key::from_slice(&output.as_ref()[..AEAD_KEY_LENGTH])
+    let default_array = SafeArray::<u8, AEAD_KEY_LEN>::default();
+    let mut aead_key = CoreTransactionAEADKey::from(default_array);
+    aead_key.reveal_mut().copy_from_slice(&output.as_ref()[..AEAD_KEY_LEN]);
+
+    aead_key
 }


You can simplify this by using finalize_into on the hasher to populate aead_key in place. Otherwise output sticks around in memory.

use digest::FixedOutput; let mut aead_key = CoreTransactionAEADKey::from(SafeArray::default()); DomainSeparatedHasher::<Blake256, TransactionKdfDomain>::new_with_label("encrypted_value") .chain(shared_secret.as_bytes()) .chain(commitment.as_bytes()) .finalize_into(GenericArray::from_mut_slice(aead_key.reveal_mut())); aead_key

Done in PR 4994.

AaronFeickert · 2022-12-02T17:18:26Z

base_layer/core/src/transactions/transaction_components/encrypted_value.rs

        value_bytes.clone_from_slice(&decrypted_bytes[..8]);
        Ok(u64::from_le_bytes(value_bytes).into())
    }
 }

 // Generate a ChaCha20-Poly1305 key from an ECDH shared secret and commitment using Blake2b
-fn kdf_aead(shared_secret: &PrivateKey, commitment: &Commitment) -> Key {
-    const AEAD_KEY_LENGTH: usize = 32; // The length in bytes of a ChaCha20-Poly1305 AEAD key
+fn kdf_aead(shared_secret: &PrivateKey, commitment: &Commitment) -> CoreTransactionAEADKey {
    let output = DomainSeparatedHasher::<Blake256, TransactionKdfDomain>::new_with_label("encrypted_value")


May wish to rename the label to be consistent with the reverse-domain notation used elsewhere.

AaronFeickert · 2022-12-02T17:26:43Z

base_layer/wallet_ffi/src/lib.rs

@@ -1550,7 +1551,8 @@ pub unsafe extern "C" fn encrypted_value_as_bytes(
 #[no_mangle]
 pub unsafe extern "C" fn encrypted_value_destroy(encrypted_value: *mut TariEncryptedValue) {
    if !encrypted_value.is_null() {
-        Box::from_raw(encrypted_value);
+        // zeroize the data content of encrypted_value, as to prevent memory leaks


Is this functionality even necessary since the encrypted value is already included on chain?

AaronFeickert · 2022-12-02T17:43:42Z

comms/core/src/noise/crypto_resolver.rs

+fn noise_kdf(shared_key: &CommsDHKE) -> CommsNoiseKey {
    let hasher = DomainSeparatedHasher::<Blake256, CommsCoreHashDomain>::new_with_label("noise.dh");
-    Digest::finalize(hasher.chain(shared_key.as_bytes())).into()
+    let mut comms_noise_kdf = CommsNoiseKey::from([0u8; NOISE_KEY_LEN]);
+    comms_noise_kdf
+        .reveal_mut()
+        .copy_from_slice(hasher.chain(shared_key.as_bytes()).finalize().as_ref());
+    comms_noise_kdf
 }


You can simplify this and avoid an in-memory copy of the KDF output using finalize_into.

use digest::FixedOutput; use digest::generic_array::GenericArray; use tari_utilities::safe_array::SafeArray; let mut comms_noise_key = CommsNoiseKey::from(SafeArray::default()); DomainSeparatedHasher::<Blake256, CommsCoreHashDomain>::new_with_label("noise.dh") .chain(shared_key.as_bytes()) .finalize_into(GenericArray::from_mut_slice(comms_noise_key.reveal_mut())); comms_noise_key

Done in PR 4994.

AaronFeickert · 2022-12-02T17:44:15Z

comms/core/src/noise/mod.rs

+
+pub(crate) const NOISE_KEY_LEN: usize = 32;
+
+hidden_type!(CommsNoiseKey, [u8; NOISE_KEY_LEN]);


Recommend redefining this as a SafeArray.

@sdbondi made a comment regarding the use of SafeArray, in this PR. I think his point was accurate, given the use of this CommsNoiseKey.

Done in PR 4994 but certainly open to discussion.

Description --- Updates the use of `CoreTransactionAEADKey` and `CommsNoiseKey` to be populated in place. Switches `CommsNoiseKey` to use a `SafeArray` under the hood. Extends the work of [PR 4967](#4967). Motivation and Context --- Earlier work updates the use of keys for value encryption and Noise. Value encryption keys use a `SafeArray` type in a `Hidden` wrapper, and Noise keys use a `Hidden` array. However, in both cases, a copy of the hash output used to populate the keys is left in memory. This work mitigates the problem. Because the hashing API now supports in-place output finalization via `finalize_into` and `finalize_into_reset`, we can populate keys directly by mutable reference. It also renames `CoreTransactionAEADKey` to `EncryptedValueKey` for clarity, and switches `CommsNoiseKey` to be a `SafeArray` under the hood. There is [discussion](#4967 (comment)) on whether `CommsNoiseKey` needs to be a `SafeArray`; while the reasoning makes sense, I still think it's good practice to take advantage of the benefits of `SafeArray` for array-like key types unless there's a compelling performance reason otherwise. Discussion on this is welcome! How Has This Been Tested? --- Existing tests pass.

jorgeantonio21 added 6 commits November 28, 2022 11:15

first commit

960ba69

Merge branch 'development' into ja-hide-sensitive-data-general

0d8c05c

zeroize bunch of structs away

55c8834

Merge branch 'development' into ja-hide-sensitive-data-general

e62428e

add zeroize to noise kdfs

77024b8

Merge branch 'development' into ja-hide-sensitive-data-general

1296b77

jorgeantonio21 marked this pull request as ready for review November 28, 2022 13:44

sdbondi previously approved these changes Nov 28, 2022

View reviewed changes

applications/tari_console_wallet/src/lib.rs Outdated Show resolved Hide resolved

comms/core/src/noise/mod.rs Outdated Show resolved Hide resolved

jorgeantonio21 added 3 commits November 28, 2022 15:30

cargo clippy

4285b6a

Merge branch 'development' into ja-hide-sensitive-data-general

806b048

refactor PR comments

f7ec9bb

jorgeantonio21 dismissed sdbondi’s stale review via f7ec9bb November 28, 2022 15:37

sdbondi previously approved these changes Nov 29, 2022

View reviewed changes

SWvheerden previously approved these changes Nov 29, 2022

View reviewed changes

merge development and fix conflicts

1c5df3e

jorgeantonio21 dismissed stale reviews from SWvheerden and sdbondi via 1c5df3e November 29, 2022 08:57

sdbondi previously approved these changes Nov 30, 2022

View reviewed changes

resolve conflicts

dbbffcf

jorgeantonio21 dismissed sdbondi’s stale review via dbbffcf November 30, 2022 14:32

stringhandler previously approved these changes Nov 30, 2022

View reviewed changes

removed file

a0f7e91

jorgeantonio21 dismissed stringhandler’s stale review via a0f7e91 November 30, 2022 15:21

stringhandler approved these changes Nov 30, 2022

View reviewed changes

stringhandler merged commit bcc47e1 into tari-project:development Nov 30, 2022

AaronFeickert reviewed Dec 2, 2022

View reviewed changes

AaronFeickert mentioned this pull request Dec 2, 2022

fix: improve key handling #4994

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: hide sensitive data on tari repo (see issue #4846) #4967

fix: hide sensitive data on tari repo (see issue #4846) #4967

jorgeantonio21 commented Nov 28, 2022

sdbondi left a comment

SWvheerden left a comment

AaronFeickert Dec 2, 2022

AaronFeickert Dec 2, 2022

AaronFeickert Dec 2, 2022

AaronFeickert Dec 2, 2022

AaronFeickert Dec 2, 2022

AaronFeickert Dec 2, 2022

AaronFeickert Dec 2, 2022

AaronFeickert Dec 2, 2022

AaronFeickert Dec 2, 2022

jorgeantonio21 Dec 2, 2022

AaronFeickert Dec 2, 2022


		pub(crate) const NOISE_KEY_LEN: usize = 32;

		hidden_type!(CommsNoiseKey, [u8; NOISE_KEY_LEN]);

fix: hide sensitive data on tari repo (see issue #4846) #4967

fix: hide sensitive data on tari repo (see issue #4846) #4967

Conversation

jorgeantonio21 commented Nov 28, 2022

Description

Motivation and Context

How Has This Been Tested?

sdbondi left a comment

Choose a reason for hiding this comment

SWvheerden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment