fix!: refactor `CipherSeed`, zeroize, and fix key derivation #4860

AaronFeickert · 2022-10-26T22:12:49Z

Description

Significantly refactors CipherSeed for clarity, zeroizes internal secrets, fixes key derivation, and verifies MACs in constant time. Does not zeroize external passphrases, which should be done separately. Fixes issue 4859.

Motivation and Context

As noted in issue 4859, CipherSeed MAC keys are derived incorrectly; in fact, the same main key is derived twice to obtain the MAC key and encryption key used for CipherSeed encryption. This work refactors for several goals:

The password is only hashed a single time, so MAC and encryption keys are produced at the same time. This makes it easier to identify that they are being derived correctly, which they now are.
Internal secrets are zeroized. Notably, passphrases from callers are not yet zeroized, which should be done in subsequent work.
MACs are verified in constant time using subtle functionality.
The code is much clearer, making it easier to identify secret and non-secret data for review.

How Has This Been Tested?

Existing tests, as the external API does not change. Manual inspection that keys are now derived correctly.

BREAKING CHANGE: Existing (version 0) seeds no longer work. Only version 1 seeds are valid.

base_layer/key_manager/src/cipher_seed.rs

sdbondi

LGTM - some minor comments

base_layer/key_manager/src/cipher_seed.rs

hansieodendaal · 2022-10-27T06:26:31Z

utACK

stringhandler · 2022-10-27T10:40:31Z

Nice. Please add a note on how existing seed phrases and wallets are affected

AaronFeickert · 2022-10-27T13:55:05Z

Nice. Please add a note on how existing seed phrases and wallets are affected

Done. If you wanted an interesting test of the versioning system, we could bump the CipherSeed version number. This would allow for immediate detection of incompatible seeds that can be reported as such. Otherwise, existing seeds will generically fail as manipulated. But that seems a bit much in this case.

jorgeantonio21 · 2022-10-29T07:27:48Z

base_layer/key_manager/src/cipher_seed.rs

@@ -113,12 +116,13 @@ pub const CIPHER_SEED_MAC_BYTES: usize = 5;
 pub struct CipherSeed {
    version: u8,
    birthday: u16,
-    entropy: [u8; CIPHER_SEED_ENTROPY_BYTES],
-    salt: [u8; CIPHER_SEED_SALT_BYTES],
+    entropy: Vec<u8>,


Why should we replace fixed size arrays to Vec's, in this case ? I personally think it is expressive to know in advance the size of both entropy and salt.

Using a Vec made it easier to handle zeroizing of secret data (due to heap storage), and also should keep the structure more general in case of future updates, where data sizes could change based on version. I could easily imagine a future where all size constants are part of a version-dependent lookup.

I'm certainly open to changing back to arrays for now, but it seems like this requires much more careful zeroizing wrappers to account for copies/moves, which seems like a footgun to avoid.

Box<[u8; CIPHER_SEED_SALT_BYTES]> is also an option as that allocates a fixed size array on the heap and you move a pointer (Box) around, not the data. But happy to keep it as a vec

Thanks for the explanations @AaronFeickert. I would say that @sdbondi suggestion is the most explicit. I am also happy to use Vec here, it is probably not worth spending time on refactoring it it, as of now.

The refactoring was pretty straightforward, so I went ahead and switched from Vec<u8> to Box<[u8]> for clarity on sizes. For some reason Zeroizing didn't like Box<[u8]>, so there's one spot where we do Zeroizing<[u8]> and then create the Box later (this is in case of MAC failure, as @sdbondi pointed out elsewhere); it didn't complain when we #[derive(Zeroize)] on Box<[u8]> though, which is odd...

jorgeantonio21 · 2022-10-29T07:49:29Z

Very cool ! Thanks for the detailed explanations as well.

utACK

Description --- Updates `argon2` for the gRPC and wallet use cases. Improves handling of keys and secret data. Fixes [issue 4882](#4882). Motivation and Context --- [Issue 4882](#4882) notes that different versions of `argon2` are used throughout the codebase. The newer minor version `0.4` changes the API significantly, and the older version `0.2` is [no longer supported](https://crates.io/crates/argon2/0.2.0). Additionally, gRPC and wallet implementations use the default parameter set from the crate, which is not in line with the [OWASP recommendations](https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#argon2id) that we use elsewhere in the key manager. Further, it's recommended that a [specific function](https://docs.rs/argon2/0.4.1/argon2/struct.Argon2.html#method.hash_password_into) be used when performing KDF functionality, as this binds the parameter set into the output. While not a major security issue, it's worth updating to this function where appropriate. Finally, secret data is kept in memory in many places through the codebase, and this area is no exception. As part of good practice, we should try to zeroize such data wherever possible. This PR addresses these issues. It updates the gRPC and wallet `argon2` versions to `0.4` (the key manager is addressed in [PR 4860](#4860) and makes the necessary API changes. It updates the parameter set to be consistent with the linked recommendations. It also adds some improved handling of secret data (but does not do so comprehensively, limiting the scope to the updated code). How Has This Been Tested? --- Existing tests pass. BREAKING: This changes how wallet passphrase-based hashes and keys are derived.

CjS77 added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Oct 26, 2022

AaronFeickert force-pushed the zeroize-cipherseed branch from 9a01ad9 to 0f02318 Compare October 27, 2022 00:57

AaronFeickert marked this pull request as ready for review October 27, 2022 01:03

AaronFeickert commented Oct 27, 2022

View reviewed changes

base_layer/key_manager/src/cipher_seed.rs Outdated Show resolved Hide resolved

AaronFeickert commented Oct 27, 2022

View reviewed changes

base_layer/key_manager/src/cipher_seed.rs Outdated Show resolved Hide resolved

AaronFeickert force-pushed the zeroize-cipherseed branch from 0f02318 to fc39869 Compare October 27, 2022 01:47

sdbondi reviewed Oct 27, 2022

View reviewed changes

stringhandler changed the title ~~fix: refactor CipherSeed, zeroize, and fix key derivation~~ fix!: refactor CipherSeed, zeroize, and fix key derivation Oct 27, 2022

CjS77 removed the P-acks_required Process - Requires more ACKs or utACKs label Oct 27, 2022

AaronFeickert mentioned this pull request Oct 27, 2022

chore: replace manual implementation of char methods #4864

Merged

jorgeantonio21 reviewed Oct 29, 2022

View reviewed changes

AaronFeickert force-pushed the zeroize-cipherseed branch from 2420ae4 to 894a424 Compare November 1, 2022 21:14

AaronFeickert mentioned this pull request Nov 3, 2022

Standardize on an argon2 version #4882

Closed

AaronFeickert added 3 commits November 3, 2022 17:26

Refactor CipherSeed with fixes

12acebb

Review comments and additional fixes

9ae6dea

Switch to boxed data for size clarity

e8a9a39

AaronFeickert force-pushed the zeroize-cipherseed branch from 894a424 to e8a9a39 Compare November 3, 2022 22:26

AaronFeickert added 2 commits November 4, 2022 10:59

Fix argon2 version and update typing

68b0d20

Remove the need for key zeroizing

5e882f2

AaronFeickert mentioned this pull request Nov 4, 2022

fix!: update argon2 and improve key handling #4892

Merged

stringhandler added A-wallet Area - related to the wallet W-transaction-breaking Warning - Changes data that wallets use to send transactions labels Nov 7, 2022

stringhandler approved these changes Nov 7, 2022

View reviewed changes

CjS77 removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Nov 7, 2022

stringhandler merged commit b190c26 into tari-project:development Nov 7, 2022

AaronFeickert deleted the zeroize-cipherseed branch November 7, 2022 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: refactor `CipherSeed`, zeroize, and fix key derivation #4860

fix!: refactor `CipherSeed`, zeroize, and fix key derivation #4860

AaronFeickert commented Oct 26, 2022 •

edited

Loading

sdbondi left a comment

hansieodendaal commented Oct 27, 2022

stringhandler commented Oct 27, 2022

AaronFeickert commented Oct 27, 2022

jorgeantonio21 Oct 29, 2022 •

edited

Loading

AaronFeickert Oct 29, 2022

AaronFeickert Oct 29, 2022

sdbondi Oct 31, 2022

jorgeantonio21 Nov 1, 2022

AaronFeickert Nov 1, 2022 •

edited

Loading

jorgeantonio21 commented Oct 29, 2022

fix!: refactor CipherSeed, zeroize, and fix key derivation #4860

fix!: refactor CipherSeed, zeroize, and fix key derivation #4860

Conversation

AaronFeickert commented Oct 26, 2022 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

sdbondi left a comment

Choose a reason for hiding this comment

hansieodendaal commented Oct 27, 2022

stringhandler commented Oct 27, 2022

AaronFeickert commented Oct 27, 2022

jorgeantonio21 Oct 29, 2022 • edited Loading

Choose a reason for hiding this comment

AaronFeickert Oct 29, 2022

Choose a reason for hiding this comment

AaronFeickert Oct 29, 2022

Choose a reason for hiding this comment

sdbondi Oct 31, 2022

Choose a reason for hiding this comment

jorgeantonio21 Nov 1, 2022

Choose a reason for hiding this comment

AaronFeickert Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

jorgeantonio21 commented Oct 29, 2022

fix!: refactor `CipherSeed`, zeroize, and fix key derivation #4860

fix!: refactor `CipherSeed`, zeroize, and fix key derivation #4860

AaronFeickert commented Oct 26, 2022 •

edited

Loading

jorgeantonio21 Oct 29, 2022 •

edited

Loading

AaronFeickert Nov 1, 2022 •

edited

Loading