Skip to content

Commit

Permalink
fix!: refactor CipherSeed, zeroize, and fix key derivation (#4860)
Browse files Browse the repository at this point in the history
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](#4859).

Motivation and Context
---
As noted in [issue 4859](#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.
  • Loading branch information
AaronFeickert authored Nov 7, 2022
1 parent 9f9179a commit b190c26
Show file tree
Hide file tree
Showing 7 changed files with 193 additions and 195 deletions.
3 changes: 2 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion base_layer/key_manager/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ tari_crypto = { git = "https://github.com/tari-project/tari-crypto.git", tag = "
tari_utilities = { git = "https://github.com/tari-project/tari_utilities.git", tag="v0.4.7" }

arrayvec = "0.7.1"
argon2 = { version = "0.2", features = ["std"] }
argon2 = { version = "0.4.1", features = ["std", "alloc"] }
blake2 = "0.9.1"
chacha20 = "0.7.1"
console_error_panic_hook = { version = "0.1.7", optional = true }
Expand All @@ -35,6 +35,7 @@ strum_macros = "0.22"
strum = { version = "0.22", features = ["derive"] }
wasm-bindgen = { version = "0.2", features = ["serde-serialize", "nightly"], optional = true }
zeroize = "1"
subtle = "2.4.1"

[dev-dependencies]
sha2 = "0.9.8"
Expand Down
358 changes: 177 additions & 181 deletions base_layer/key_manager/src/cipher_seed.rs

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions base_layer/key_manager/src/wasm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,8 @@ mod test {
#[wasm_bindgen_test]
fn it_creates_key_manager_from() {
let bytes = [
0, 2, 116, 75, 54, 160, 21, 1, 43, 55, 107, 155, 189, 230, 182, 215, 17, 191, 94, 156, 114, 136, 40, 175,
144, 166, 93, 233, 179, 11, 8, 49, 139,
1, 34, 207, 175, 242, 162, 209, 98, 199, 251, 212, 88, 214, 61, 84, 199, 115, 189, 159, 168, 6, 137, 216,
235, 137, 235, 26, 192, 38, 195, 217, 218, 53,
];
let seed = CipherSeed::from_enciphered_bytes(&bytes, None).unwrap();
let seed = JsValue::from_serde(&seed).unwrap();
Expand All @@ -194,7 +194,7 @@ mod test {
let next_key = response.key_manager.next_key().unwrap();
assert_eq!(
next_key.k.to_hex(),
"84feaddf54f1b4321db67f7aae382c338d03c56280a417651c4e0cde3363d00a".to_string()
"7220010f6eb7b1a5429c3e29f3186190312a824cb6551c0c0c4640ecc676da0e".to_string()
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,9 @@ async fn setup_output_manager_service<T: OutputManagerBackend + 'static, U: KeyM

let cipher_seed = CipherSeed::from_mnemonic(
&[
"cactus", "fruit", "amount", "strong", "join", "tuna", "combine", "actor", "plug", "north", "defense",
"husband", "roof", "alpha", "present", "daughter", "spare", "trial", "border", "bridge", "actor",
"receive", "leader", "fashion",
"scan", "train", "success", "hover", "prepare", "donor", "upgrade", "attitude", "debate", "emotion",
"myself", "ladder", "display", "athlete", "welcome", "artist", "home", "punch", "sense", "park",
"midnight", "quantum", "bright", "carbon",
]
.iter()
.map(ToString::to_string)
Expand Down
6 changes: 3 additions & 3 deletions base_layer/wallet/tests/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -763,9 +763,9 @@ async fn test_recovery_birthday() {
// println!("{:?}", mnemonic_seq);

let seed_words: Vec<String> = [
"octavo", "joroba", "aplicar", "lamina", "semilla", "tiempo", "codigo", "contar", "maniqui", "guiso",
"imponer", "barba", "torpedo", "mejilla", "fijo", "grave", "caer", "libertad", "sol", "sordo", "alacran",
"bucle", "diente", "vereda",
"octubre", "rinon", "ameno", "rigido", "verbo", "dosis", "ocaso", "fallo", "tez", "ladron", "entrar", "pedal",
"fortuna", "ahogo", "llanto", "mascara", "intuir", "buey", "cubrir", "anillo", "cajon", "entrar", "clase",
"latir",
]
.iter()
.map(|w| w.to_string())
Expand Down
6 changes: 3 additions & 3 deletions base_layer/wallet_ffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9063,9 +9063,9 @@ mod test {
// println!("{:?}", mnemonic_seq);

let mnemonic = vec![
"scale", "poem", "sorry", "language", "gorilla", "despair", "alarm", "jungle", "invite", "orient",
"blast", "try", "jump", "escape", "estate", "reward", "race", "taxi", "pitch", "soccer", "matter",
"team", "parrot", "enter",
"scan", "couch", "work", "water", "find", "electric", "weasel", "code", "column", "sick", "secret",
"birth", "word", "infant", "fatigue", "upper", "vacuum", "senior", "build", "post", "lend", "electric",
"pact", "retire",
];

let seed_words = seed_words_create();
Expand Down

0 comments on commit b190c26

Please sign in to comment.