diff --git a/Cargo.lock b/Cargo.lock index 65e6c1e1b..5c9d09b84 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4189,6 +4189,7 @@ version = "1.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "525b4ec142c6b68a2d10f01f7bbf6755599ca3f81ea53b8431b7dd348f5fdb2d" dependencies = [ + "serde", "zeroize_derive", ] diff --git a/crates/bitwarden-crypto/Cargo.toml b/crates/bitwarden-crypto/Cargo.toml index f07fc2a04..9da62a44c 100644 --- a/crates/bitwarden-crypto/Cargo.toml +++ b/crates/bitwarden-crypto/Cargo.toml @@ -17,6 +17,7 @@ keywords.workspace = true default = [] mobile = ["dep:uniffi"] # Mobile-specific features +test = [] # Test methods [dependencies] aes = { version = ">=0.8.2, <0.9", features = ["zeroize"] } diff --git a/crates/bitwarden-crypto/src/sensitive/sensitive.rs b/crates/bitwarden-crypto/src/sensitive/sensitive.rs index 2ae54d7a2..4ae779f05 100644 --- a/crates/bitwarden-crypto/src/sensitive/sensitive.rs +++ b/crates/bitwarden-crypto/src/sensitive/sensitive.rs @@ -228,9 +228,7 @@ impl JsonSchema for Sensitive { // We use a lot of `&str` and `&[u8]` in our tests, so we expose this helper // to make it easier. // IMPORTANT: This should not be used outside of test code -// Note that we can't just mark it with #[cfg(test)] because that only applies -// when testing this crate, not when testing other crates that depend on it. -// By at least limiting it to &'static reference we should be able to avoid accidental usages +#[cfg(any(test, feature = "test"))] impl Sensitive { pub fn test(value: &'static T) -> Self where diff --git a/crates/bitwarden-exporters/Cargo.toml b/crates/bitwarden-exporters/Cargo.toml index ddfff5b38..3b19d4bf5 100644 --- a/crates/bitwarden-exporters/Cargo.toml +++ b/crates/bitwarden-exporters/Cargo.toml @@ -29,5 +29,8 @@ serde_json = ">=1.0.96, <2.0" thiserror = ">=1.0.40, <2.0" uuid = { version = ">=1.3.3, <2.0", features = ["serde", "v4"] } +[dev-dependencies] +bitwarden-crypto = { workspace = true, features = ["test"] } + [lints] workspace = true diff --git a/crates/bitwarden/Cargo.toml b/crates/bitwarden/Cargo.toml index 3e92981f0..0be39073e 100644 --- a/crates/bitwarden/Cargo.toml +++ b/crates/bitwarden/Cargo.toml @@ -81,6 +81,7 @@ reqwest = { version = ">=0.12, <0.13", features = [ ], default-features = false } [dev-dependencies] +bitwarden-crypto = { workspace = true, features = ["test"] } rand_chacha = "0.3.1" tokio = { version = "1.36.0", features = ["rt", "macros"] } wiremock = "0.6.0" diff --git a/crates/memory-testing/Cargo.toml b/crates/memory-testing/Cargo.toml index 120fcaad1..6f2c81b67 100644 --- a/crates/memory-testing/Cargo.toml +++ b/crates/memory-testing/Cargo.toml @@ -17,4 +17,4 @@ comfy-table = "7.1.0" hex = "0.4.3" serde = "1.0.196" serde_json = "1.0.113" -zeroize = "1.7.0" +zeroize = { version = "1.7.0", features = ["serde"] } diff --git a/crates/memory-testing/Dockerfile b/crates/memory-testing/Dockerfile index 601800355..f8556f1e2 100644 --- a/crates/memory-testing/Dockerfile +++ b/crates/memory-testing/Dockerfile @@ -14,7 +14,7 @@ RUN mkdir -p /app/crates/bitwarden-crypto/src \ && mkdir -p /app/crates/memory-testing/src \ && touch /app/crates/bitwarden-crypto/src/lib.rs \ && echo 'fn main(){}' > /app/crates/memory-testing/src/main.rs \ - && cargo build -p memory-testing + && cargo build -p memory-testing --release # Delete dummy files and copy the actual source code RUN rm /app/crates/bitwarden-crypto/src/lib.rs /app/crates/memory-testing/src/main.rs @@ -23,7 +23,7 @@ COPY crates/memory-testing/src /app/crates/memory-testing/src # Build the project. We use touch to force a rebuild of the now real files RUN touch /app/crates/bitwarden-crypto/src/lib.rs /app/crates/memory-testing/src/main.rs -RUN cargo build -p memory-testing +RUN cargo build -p memory-testing --release ############################################### # App stage # @@ -36,7 +36,7 @@ USER root RUN apt-get update && apt-get install -y --no-install-recommends gdb=13.1-3 && apt-get clean && rm -rf /var/lib/apt/lists/* # Copy built project from the build stage and the cases.json file -COPY --from=build /app/target/debug/memory-testing /app/target/debug/capture-dumps ./ +COPY --from=build /app/target/release/memory-testing /app/target/release/capture-dumps ./ COPY crates/memory-testing/cases.json . CMD [ "/capture-dumps", "./memory-testing", "/output" ] diff --git a/crates/memory-testing/cases.json b/crates/memory-testing/cases.json index 5923e350a..c24cb63d6 100644 --- a/crates/memory-testing/cases.json +++ b/crates/memory-testing/cases.json @@ -1,41 +1,78 @@ { - "symmetric_key": [ + "cases": [ { - "key": "FfhVVP8fmFIZY1WmRszPmRmVCxXNWVcJffPrbkywTPtBNkgfhYGT+D9sVGizYXrPffuj2yoyWqMwF9iF5aMQhQ==", - "decrypted_key_hex": "15f85554ff1f9852196355a646cccf9919950b15cd5957097df3eb6e4cb04cfb", - "decrypted_mac_hex": "4136481f858193f83f6c5468b3617acf7dfba3db2a325aa33017d885e5a31085" - } - ], - "master_key": [ + "name": "Symmetric Key", + "symmetric_key": { + "key": "FfhVVP8fmFIZY1WmRszPmRmVCxXNWVcJffPrbkywTPtBNkgfhYGT+D9sVGizYXrPffuj2yoyWqMwF9iF5aMQhQ==" + }, + "memory_lookups": [ + { + "name": "Decrypted key", + "hex": "15f85554ff1f9852196355a646cccf9919950b15cd5957097df3eb6e4cb04cfb" + }, + { + "name": "Decrypted MAC", + "hex": "4136481f858193f83f6c5468b3617acf7dfba3db2a325aa33017d885e5a31085" + } + ] + }, + { - "password": "123412341234", - "email": "test@mail.com", + "name": "Master Key PBKDF2", + "master_key": { + "password": "123412341234", + "email": "test@mail.com", - "kdf": { - "pBKDF2": { - "iterations": 100000 + "kdf": { + "pBKDF2": { + "iterations": 100000 + } } }, - - "key_hex": "2d55ac8e33bd14ee9eee26fa651163a41049a37b3b20c914a8b6abc9a38a89d5", - "hash": "gQ9O5ZhtQ23dL5r93e4BL04ATYOJVEvBAOwYsDDEJFA=", - "hash_hex": "810f4ee5986d436ddd2f9afdddee012f4e004d8389544bc100ec18b030c42450" + "memory_lookups": [ + { + "name": "Key", + "hex": "2d55ac8e33bd14ee9eee26fa651163a41049a37b3b20c914a8b6abc9a38a89d5" + }, + { + "name": "Hash B64", + "string": "gQ9O5ZhtQ23dL5r93e4BL04ATYOJVEvBAOwYsDDEJFA=" + }, + { + "name": "Hash bytes", + "hex": "810f4ee5986d436ddd2f9afdddee012f4e004d8389544bc100ec18b030c42450" + } + ] }, { - "password": "asdfasdfasdf", - "email": "test@mail.com", + "name": "Master Key Argon2", + "master_key": { + "password": "asdfasdfasdf", + "email": "test@mail.com", - "kdf": { - "argon2id": { - "iterations": 3, - "memory": 4, - "parallelism": 1 + "kdf": { + "argon2id": { + "iterations": 3, + "memory": 4, + "parallelism": 1 + } } }, - - "key_hex": "3bc0520a0abff0097d521ce0ee5e5b1cee301939a84742623c0c1697d7a4bd46", - "hash": "lHkprdORlICVJ4Umwi94Uz/nATK6Y7If7e+iFoabzh0=", - "hash_hex": "947929add391948095278526c22f78533fe70132ba63b21fedefa216869bce1d" + "memory_lookups": [ + { + "name": "Key", + "hex": "3bc0520a0abff0097d521ce0ee5e5b1cee301939a84742623c0c1697d7a4bd46", + "allowed_count": 3 + }, + { + "name": "Hash B64", + "string": "lHkprdORlICVJ4Umwi94Uz/nATK6Y7If7e+iFoabzh0=" + }, + { + "name": "Hash bytes", + "hex": "947929add391948095278526c22f78533fe70132ba63b21fedefa216869bce1d" + } + ] } ] } diff --git a/crates/memory-testing/run_test.sh b/crates/memory-testing/run_test.sh index 750bbc8ed..8e1f84b4a 100755 --- a/crates/memory-testing/run_test.sh +++ b/crates/memory-testing/run_test.sh @@ -9,14 +9,14 @@ BASE_DIR="./crates/memory-testing" mkdir -p $BASE_DIR/output rm $BASE_DIR/output/* || true -cargo build -p memory-testing +cargo build -p memory-testing --release if [ "$1" = "no-docker" ]; then # This specifically needs to run as root to be able to capture core dumps - sudo ./target/debug/capture-dumps ./target/debug/memory-testing $BASE_DIR + sudo ./target/release/capture-dumps ./target/release/memory-testing $BASE_DIR else docker build -f crates/memory-testing/Dockerfile -t bitwarden/memory-testing . docker run --rm -it -v $BASE_DIR:/output bitwarden/memory-testing fi -./target/debug/analyze-dumps $BASE_DIR +./target/release/analyze-dumps $BASE_DIR diff --git a/crates/memory-testing/src/bin/analyze-dumps.rs b/crates/memory-testing/src/bin/analyze-dumps.rs index 8b86337f9..d55bf8b46 100644 --- a/crates/memory-testing/src/bin/analyze-dumps.rs +++ b/crates/memory-testing/src/bin/analyze-dumps.rs @@ -1,6 +1,5 @@ -use std::{env, fmt::Display, io, path::Path, process}; +use std::{env, io, path::Path, process}; -use bitwarden_crypto::Kdf; use memory_testing::*; fn find_subarrays(needle: &[u8], haystack: &[u8]) -> Vec { @@ -31,26 +30,6 @@ fn comma_sep(nums: &[usize]) -> String { .join(", ") } -fn add_row( - table: &mut comfy_table::Table, - name: N, - initial_pos: &[usize], - final_pos: &[usize], - ok_cond: bool, -) -> bool { - table.add_row(vec![ - name.to_string(), - comma_sep(initial_pos), - comma_sep(final_pos), - if ok_cond { - OK.to_string() - } else { - FAIL.to_string() - }, - ]); - !ok_cond -} - fn main() -> io::Result<()> { let args: Vec = env::args().collect(); if args.len() < 2 { @@ -72,114 +51,36 @@ fn main() -> io::Result<()> { let test_string: Vec = TEST_STRING.as_bytes().to_vec(); let test_initial_pos = find_subarrays(&test_string, &initial_core); - let test_final_pos = find_subarrays(&test_string, &final_core); - - error |= add_row( - &mut table, - "Test String", - &test_initial_pos, - &test_final_pos, - !test_final_pos.is_empty(), - ); if test_initial_pos.is_empty() { println!("ERROR: Test string not found in initial core dump, is the dump valid?"); error = true; } - for (idx, case) in cases.symmetric_key.iter().enumerate() { - let key_part: Vec = hex::decode(&case.decrypted_key_hex).unwrap(); - let mac_part: Vec = hex::decode(&case.decrypted_mac_hex).unwrap(); - let key_in_b64: Vec = case.key.as_bytes().to_vec(); - - let key_initial_pos = find_subarrays(&key_part, &initial_core); - let mac_initial_pos = find_subarrays(&mac_part, &initial_core); - let b64_initial_pos = find_subarrays(&key_in_b64, &initial_core); - - let key_final_pos = find_subarrays(&key_part, &final_core); - let mac_final_pos = find_subarrays(&mac_part, &final_core); - let b64_final_pos = find_subarrays(&key_in_b64, &final_core); - - error |= add_row( - &mut table, - format!("Symm. Key, case {}", idx), - &key_initial_pos, - &key_final_pos, - key_final_pos.is_empty(), - ); - - error |= add_row( - &mut table, - format!("Symm. MAC, case {}", idx), - &mac_initial_pos, - &mac_final_pos, - mac_final_pos.is_empty(), - ); - - error |= add_row( - &mut table, - format!("Symm. Key in Base64, case {}", idx), - &b64_initial_pos, - &b64_final_pos, - b64_final_pos.is_empty(), - ); - } - - for (idx, case) in cases.master_key.iter().enumerate() { - let password = case.password.as_bytes().to_vec(); - let key = hex::decode(&case.key_hex).unwrap(); - let hash_b64 = case.hash.as_bytes().to_vec(); - let hash = hex::decode(&case.hash_hex).unwrap(); - - let pass_initial_pos = find_subarrays(&password, &initial_core); - let key_initial_pos = find_subarrays(&key, &initial_core); - let hashb64_initial_pos = find_subarrays(&hash_b64, &initial_core); - let hash_initial_pos = find_subarrays(&hash, &initial_core); - - let pass_final_pos = find_subarrays(&password, &final_core); - let key_final_pos = find_subarrays(&key, &final_core); - let hashb64_final_pos = find_subarrays(&hash_b64, &final_core); - let hash_final_pos = find_subarrays(&hash, &final_core); - - error |= add_row( - &mut table, - format!("Master Key password, case {}", idx), - &pass_initial_pos, - &pass_final_pos, - pass_final_pos.is_empty(), - ); - - // At the moment the argon library is producing some hard to pinpoint leaks - // Upgrading to the latest pre-release version solves at least one of them - let allowed_leaks = if matches!(case.kdf, Kdf::Argon2id { .. }) { - 3 - } else { - 0 - }; - - error |= add_row( - &mut table, - format!("Master Key, case {}", idx), - &key_initial_pos, - &key_final_pos, - key_final_pos.len() <= allowed_leaks, - ); - - error |= add_row( - &mut table, - format!("Master Key Hash B64, case {}", idx), - &hashb64_initial_pos, - &hashb64_final_pos, - hashb64_final_pos.is_empty(), - ); - - error |= add_row( - &mut table, - format!("Master Key Hash, case {}", idx), - &hash_initial_pos, - &hash_final_pos, - hash_final_pos.is_empty(), - ); + for (idx, case) in cases.cases.iter().enumerate() { + for lookup in &case.memory_lookups { + let value = match &lookup.value { + MemoryLookupValue::String { string } => string.as_bytes().to_vec(), + MemoryLookupValue::Binary { hex } => hex::decode(hex).unwrap(), + }; + + let initial_pos = find_subarrays(&value, &initial_core); + let final_pos = find_subarrays(&value, &final_core); + + let name = format!("{idx}: {} / {}", case.name, lookup.name); + let ok_cond = final_pos.len() <= lookup.allowed_count.unwrap_or(0); + + table.add_row([ + name.as_str(), + &comma_sep(&initial_pos), + &comma_sep(&final_pos), + if ok_cond { OK } else { FAIL }, + ]); + + if !ok_cond { + error = true; + } + } } println!("{table}"); diff --git a/crates/memory-testing/src/bin/capture-dumps.rs b/crates/memory-testing/src/bin/capture-dumps.rs index caa8b8dfd..86a006988 100644 --- a/crates/memory-testing/src/bin/capture-dumps.rs +++ b/crates/memory-testing/src/bin/capture-dumps.rs @@ -2,9 +2,7 @@ use std::{ fs, io::{self, prelude::*}, path::Path, - process::{Command, Stdio}, - thread::sleep, - time::Duration, + process::{ChildStdin, ChildStdout, Command, Stdio}, }; fn dump_process_to_bytearray(pid: u32, output_dir: &Path, output_name: &Path) -> io::Result { @@ -19,6 +17,33 @@ fn dump_process_to_bytearray(pid: u32, output_dir: &Path, output_name: &Path) -> Ok(len) } +fn wait_dump_and_continue( + stdin: &mut ChildStdin, + stdout: &mut ChildStdout, + id: u32, + base_dir: &Path, + name: &Path, +) -> Result<(), io::Error> { + // Read the input from the process until we get the "Waiting for dump..." message + // That way we know the process is ready to be dumped, and we don't need to just sleep a fixed + // amount of time + loop { + let mut buf = [0u8; 1024]; + let read = stdout.read(&mut buf).unwrap(); + let buf_str = std::str::from_utf8(&buf[..read]).unwrap(); + if buf_str.contains("Waiting for dump...") { + break; + } + } + let dump_size = dump_process_to_bytearray(id, &base_dir.join("output"), name)?; + println!("Got memory dump of file size: {}", dump_size); + + stdin.write_all(b".")?; + stdin.flush()?; + + Ok(()) +} + fn main() -> io::Result<()> { let args: Vec = std::env::args().collect(); if args.len() < 3 { @@ -29,38 +54,19 @@ fn main() -> io::Result<()> { let binary_path = &args[1]; let base_dir: &Path = args[2].as_ref(); - println!("Memory dump capture script started"); - let mut proc = Command::new(binary_path) .arg(base_dir) - .stdout(Stdio::inherit()) + .stdout(Stdio::piped()) .stdin(Stdio::piped()) .spawn()?; let id = proc.id(); println!("Started memory testing process with PID: {}", id); - let stdin = proc.stdin.as_mut().expect("Valid stdin"); - // Wait a bit for it to process - sleep(Duration::from_millis(1500)); - - // Dump the process before the variables are freed - let initial_core = - dump_process_to_bytearray(id, &base_dir.join("output"), "initial_dump.bin".as_ref())?; - println!("Initial core dump file size: {}", initial_core); - - stdin.write_all(b".")?; - stdin.flush()?; - - // Wait a bit for it to process - sleep(Duration::from_millis(500)); - - // Dump the process after the variables are freed - let final_core = - dump_process_to_bytearray(id, &base_dir.join("output"), "final_dump.bin".as_ref())?; - println!("Final core dump file size: {}", final_core); + let stdin = proc.stdin.as_mut().expect("Valid stdin"); + let stdout = proc.stdout.as_mut().expect("Valid stdin"); - stdin.write_all(b".")?; - stdin.flush()?; + wait_dump_and_continue(stdin, stdout, id, base_dir, "initial_dump.bin".as_ref())?; + wait_dump_and_continue(stdin, stdout, id, base_dir, "final_dump.bin".as_ref())?; // Wait for the process to finish and print the output let output = proc.wait()?; diff --git a/crates/memory-testing/src/lib.rs b/crates/memory-testing/src/lib.rs index f87966d3b..0300e287a 100644 --- a/crates/memory-testing/src/lib.rs +++ b/crates/memory-testing/src/lib.rs @@ -1,7 +1,7 @@ use std::path::Path; use bitwarden_crypto::Kdf; -use zeroize::Zeroize; +use zeroize::{Zeroize, Zeroizing}; pub const TEST_STRING: &str = "THIS IS USED TO CHECK THAT THE MEMORY IS DUMPED CORRECTLY"; @@ -9,36 +9,55 @@ pub fn load_cases(base_dir: &Path) -> Cases { let mut json_str = std::fs::read_to_string(base_dir.join("cases.json")).unwrap(); let cases: Cases = serde_json::from_str(&json_str).unwrap(); - // Make sure that we don't leave extra copies of the data in memory + // Make sure that we don't leave extra copies of the string data in memory json_str.zeroize(); cases } -// Note: We don't actively zeroize these structs here because we want the code in bitwarden_crypto -// to handle it for us #[derive(serde::Deserialize)] pub struct Cases { - pub symmetric_key: Vec, - - pub master_key: Vec, + pub cases: Vec, } #[derive(serde::Deserialize)] -pub struct SymmetricKeyCases { - pub key: String, +pub struct Case { + pub name: String, + #[serde(flatten)] + pub command: CaseCommand, + pub memory_lookups: Vec, +} - pub decrypted_key_hex: String, - pub decrypted_mac_hex: String, +// We don't actively zeroize this struct because we want the code in bitwarden_crypto +// to handle it for us +#[derive(serde::Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum CaseCommand { + SymmetricKey { + key: String, + }, + MasterKey { + password: String, + email: String, + kdf: Kdf, + }, } #[derive(serde::Deserialize)] -pub struct MasterKeyCases { - pub password: String, +pub struct MemoryLookup { + pub name: String, + + #[serde(flatten)] + pub value: MemoryLookupValue, - pub email: String, - pub kdf: Kdf, + #[serde(default)] + pub allowed_count: Option, +} - pub key_hex: String, - pub hash: String, - pub hash_hex: String, +// We don't actually want these values to be caught by the memory testing, +// so this enum should be always zeroized +#[derive(serde::Deserialize)] +#[serde(untagged)] +pub enum MemoryLookupValue { + String { string: Zeroizing }, + Binary { hex: Zeroizing }, } diff --git a/crates/memory-testing/src/main.rs b/crates/memory-testing/src/main.rs index c50efb6b1..ce120811b 100644 --- a/crates/memory-testing/src/main.rs +++ b/crates/memory-testing/src/main.rs @@ -20,37 +20,40 @@ fn main() { let cases = memory_testing::load_cases(base_dir); let mut symmetric_keys = Vec::new(); - - for case in cases.symmetric_key { - let key = SensitiveString::new(Box::new(case.key)); - let key = SymmetricCryptoKey::try_from(key).unwrap(); - symmetric_keys.push((key.to_vec(), key)); - } - let mut master_keys = Vec::new(); - for case in cases.master_key { - let password: SensitiveVec = SensitiveString::new(Box::new(case.password)).into(); - - let key = MasterKey::derive(&password, case.email.as_bytes(), &case.kdf).unwrap(); - let hash = key - .derive_master_key_hash( - &password, - bitwarden_crypto::HashPurpose::ServerAuthorization, - ) - .unwrap(); - - master_keys.push((key, hash)); + for case in cases.cases { + match case.command { + memory_testing::CaseCommand::SymmetricKey { key } => { + let key = SensitiveString::new(Box::new(key)); + let key = SymmetricCryptoKey::try_from(key).unwrap(); + symmetric_keys.push((key.to_vec(), key)); + } + memory_testing::CaseCommand::MasterKey { + password, + email, + kdf, + } => { + let password: SensitiveVec = SensitiveString::new(Box::new(password)).into(); + let key = MasterKey::derive(&password, email.as_bytes(), &kdf).unwrap(); + let hash = key + .derive_master_key_hash( + &password, + bitwarden_crypto::HashPurpose::ServerAuthorization, + ) + .unwrap(); + + master_keys.push((key, hash)); + } + } } // Make a memory dump before the variables are freed wait_for_dump(); - // Use all the variables so the compiler doesn't decide to remove them - println!("{test_string} {symmetric_keys:?} {master_keys:?} "); - - drop(symmetric_keys); - drop(master_keys); + // Put all the variables through a black box to prevent them from being optimized out before we + // get to this point, and then drop them + let _ = std::hint::black_box((test_string, symmetric_keys, master_keys)); // After the variables are dropped, we want to make another dump wait_for_dump();