diff --git a/.github/codecov.yml b/.github/codecov.yml index 3228d009c..eb34abf9c 100644 --- a/.github/codecov.yml +++ b/.github/codecov.yml @@ -1,3 +1,4 @@ ignore: - "crates/sdk-schemas" # Tool - "crates/uniffi-bindgen" # Tool + - "crates/memory-testing" # Testing diff --git a/.github/workflows/memory-testing.yml b/.github/workflows/memory-testing.yml new file mode 100644 index 000000000..af5ef6b7c --- /dev/null +++ b/.github/workflows/memory-testing.yml @@ -0,0 +1,43 @@ +--- +name: Test for memory leaks + +on: + pull_request: + paths: + - "crates/bitwarden-crypto/**" + - "crates/memory-testing/**" + push: + paths: + - "crates/bitwarden-crypto/**" + - "crates/memory-testing/**" + branches: + - "main" + - "rc" + - "hotfix-rc" + +jobs: + memory-test: + name: Testing + runs-on: ubuntu-22.04 + + steps: + - name: Check out repo + uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 + + - name: Set up gdb + run: | + sudo apt update + sudo apt -y install gdb + + - name: Install rust + uses: dtolnay/rust-toolchain@be73d7920c329f220ce78e0234b8f96b7ae60248 # stable + with: + toolchain: stable + + - name: Cache cargo registry + uses: Swatinem/rust-cache@23bce251a8cd2ffc3c1075eaa2367cf899916d84 # v2.7.3 + with: + key: memtest-cargo + + - name: Test + run: ./crates/memory-testing/run_test.sh no-docker diff --git a/.gitignore b/.gitignore index 63c5875a3..b13651d19 100644 --- a/.gitignore +++ b/.gitignore @@ -12,7 +12,6 @@ x64/ x86/ build/ bld/ -[Bb]in/ [Oo]bj/ *.wasm diff --git a/Cargo.lock b/Cargo.lock index 5c9d0fb58..b9f383413 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1544,6 +1544,12 @@ version = "0.3.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "bd5256b483761cd23699d0da46cc6fd2ee3be420bbe6d020ae4a091e70b7e9fd" +[[package]] +name = "hex" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7f24254aa9a54b5c858eaee2f5bccdb46aaf0e486a595ed5fd8f86ba55232a70" + [[package]] name = "hkdf" version = "0.12.4" @@ -1970,6 +1976,18 @@ dependencies = [ "autocfg", ] +[[package]] +name = "memory-testing" +version = "0.1.0" +dependencies = [ + "bitwarden-crypto", + "comfy-table", + "hex", + "serde", + "serde_json", + "zeroize", +] + [[package]] name = "mime" version = "0.3.17" diff --git a/crates/memory-testing/.gitignore b/crates/memory-testing/.gitignore new file mode 100644 index 000000000..53752db25 --- /dev/null +++ b/crates/memory-testing/.gitignore @@ -0,0 +1 @@ +output diff --git a/crates/memory-testing/Cargo.toml b/crates/memory-testing/Cargo.toml new file mode 100644 index 000000000..c1ecbbf54 --- /dev/null +++ b/crates/memory-testing/Cargo.toml @@ -0,0 +1,13 @@ +[package] +name = "memory-testing" +version = "0.1.0" +edition = "2021" +publish = false + +[dependencies] +bitwarden-crypto = { path = "../bitwarden-crypto", version = "=0.1.0" } +comfy-table = "7.1.0" +hex = "0.4.3" +serde = "1.0.196" +serde_json = "1.0.113" +zeroize = "1.7.0" diff --git a/crates/memory-testing/Dockerfile b/crates/memory-testing/Dockerfile new file mode 100644 index 000000000..fdcf5de00 --- /dev/null +++ b/crates/memory-testing/Dockerfile @@ -0,0 +1,26 @@ +############################################### +# Build stage # +############################################### +FROM rust:1.76 AS build + +# Copy required project files +COPY . /app + +# Build project +WORKDIR /app +RUN cargo build -p memory-testing + +############################################### +# App stage # +############################################### +FROM debian:bookworm-slim + +# This specifically needs to run as root to be able to capture core dumps +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 +COPY --from=build /app/target/debug/memory-testing /app/target/debug/capture-dumps /app/crates/memory-testing/cases.json ./ + +CMD [ "/capture-dumps", "./memory-testing", "/output" ] diff --git a/crates/memory-testing/Dockerfile.dockerignore b/crates/memory-testing/Dockerfile.dockerignore new file mode 100644 index 000000000..50f4b1230 --- /dev/null +++ b/crates/memory-testing/Dockerfile.dockerignore @@ -0,0 +1,4 @@ +* +!crates/* +!Cargo.toml +!Cargo.lock diff --git a/crates/memory-testing/cases.json b/crates/memory-testing/cases.json new file mode 100644 index 000000000..eb85e2aca --- /dev/null +++ b/crates/memory-testing/cases.json @@ -0,0 +1,9 @@ +{ + "symmetric_key": [ + { + "key": "FfhVVP8fmFIZY1WmRszPmRmVCxXNWVcJffPrbkywTPtBNkgfhYGT+D9sVGizYXrPffuj2yoyWqMwF9iF5aMQhQ==", + "decrypted_key_hex": "15f85554ff1f9852196355a646cccf9919950b15cd5957097df3eb6e4cb04cfb", + "decrypted_mac_hex": "4136481f858193f83f6c5468b3617acf7dfba3db2a325aa33017d885e5a31085" + } + ] +} diff --git a/crates/memory-testing/run_test.sh b/crates/memory-testing/run_test.sh new file mode 100755 index 000000000..627e5dacd --- /dev/null +++ b/crates/memory-testing/run_test.sh @@ -0,0 +1,20 @@ +# Move to the root of the repository +cd "$(dirname "$0")" +cd ../../ + +BASE_DIR="./crates/memory-testing" + +mkdir -p $BASE_DIR/output +rm $BASE_DIR/output/* + +cargo build -p memory-testing + +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 +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 diff --git a/crates/memory-testing/src/bin/analyze-dumps.rs b/crates/memory-testing/src/bin/analyze-dumps.rs new file mode 100644 index 000000000..fee72f2e5 --- /dev/null +++ b/crates/memory-testing/src/bin/analyze-dumps.rs @@ -0,0 +1,132 @@ +use std::{env, fmt::Display, io, path::Path, process}; + +use memory_testing::*; + +fn find_subarrays(needle: &[u8], haystack: &[u8]) -> Vec { + let needle_len = needle.len(); + let haystack_len = haystack.len(); + let mut subarrays = vec![]; + + if needle_len == 0 || haystack_len == 0 || needle_len > haystack_len { + return vec![]; + } + + for i in 0..=(haystack_len - needle_len) { + if &haystack[i..i + needle_len] == needle { + subarrays.push(i); + } + } + + subarrays +} + +const OK: &str = "✅"; +const FAIL: &str = "❌"; + +fn comma_sep(nums: &[usize]) -> String { + nums.iter() + .map(ToString::to_string) + .collect::>() + .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 { + println!("Usage: ./analyze-dumps "); + process::exit(1); + } + let base_dir: &Path = args[1].as_ref(); + + println!("Memory testing script started"); + + let initial_core = std::fs::read(base_dir.join("output/initial_dump.bin"))?; + let final_core = std::fs::read(base_dir.join("output/final_dump.bin"))?; + + let mut error = false; + let mut table = comfy_table::Table::new(); + table.set_header(vec!["Name", "Initial", "Final", "OK"]); + + let cases = memory_testing::load_cases(base_dir); + + 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(), + ); + + // TODO: At the moment we are not zeroizing the base64 key in from_str, so this test is + // ignored + add_row( + &mut table, + format!("Symm. Key in Base64, case {}", idx), + &b64_initial_pos, + &b64_final_pos, + b64_final_pos.is_empty(), + ); + } + + println!("{table}"); + + process::exit(if error { 1 } else { 0 }); +} diff --git a/crates/memory-testing/src/bin/capture-dumps.rs b/crates/memory-testing/src/bin/capture-dumps.rs new file mode 100644 index 000000000..f43905867 --- /dev/null +++ b/crates/memory-testing/src/bin/capture-dumps.rs @@ -0,0 +1,70 @@ +use std::{ + fs, + io::{self, prelude::*}, + path::Path, + process::{Command, Stdio}, + thread::sleep, + time::Duration, +}; + +fn dump_process_to_bytearray(pid: u32, output_dir: &Path, output_name: &Path) -> io::Result { + Command::new("gcore") + .args(["-a", &pid.to_string()]) + .output()?; + + let core_path = format!("core.{}", pid); + let output_path = output_dir.join(output_name); + let len = fs::copy(&core_path, output_path)?; + fs::remove_file(&core_path)?; + Ok(len) +} + +fn main() -> io::Result<()> { + let args: Vec = std::env::args().collect(); + if args.len() < 3 { + println!("Usage: ./capture_dumps "); + std::process::exit(1); + } + + 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()) + .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_secs(3)); + + // 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_secs(1)); + + // 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); + + stdin.write_all(b".")?; + stdin.flush()?; + + // Wait for the process to finish and print the output + let output = proc.wait()?; + println!("Return code: {}", output); + + std::process::exit(output.code().unwrap_or(1)); +} diff --git a/crates/memory-testing/src/lib.rs b/crates/memory-testing/src/lib.rs new file mode 100644 index 000000000..e633756d1 --- /dev/null +++ b/crates/memory-testing/src/lib.rs @@ -0,0 +1,29 @@ +use std::path::Path; + +use zeroize::Zeroize; + +pub const TEST_STRING: &str = "THIS IS USED TO CHECK THAT THE MEMORY IS DUMPED CORRECTLY"; + +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 + 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, +} + +#[derive(serde::Deserialize)] +pub struct SymmetricKeyCases { + pub key: String, + + pub decrypted_key_hex: String, + pub decrypted_mac_hex: String, +} diff --git a/crates/memory-testing/src/main.rs b/crates/memory-testing/src/main.rs new file mode 100644 index 000000000..96e4ae175 --- /dev/null +++ b/crates/memory-testing/src/main.rs @@ -0,0 +1,44 @@ +use std::{env, io::Read, path::Path, process, str::FromStr}; + +use bitwarden_crypto::SymmetricCryptoKey; + +fn wait_for_dump() { + println!("Waiting for dump..."); + std::io::stdin().read_exact(&mut [1u8]).unwrap(); +} + +fn main() { + let args: Vec = env::args().collect(); + if args.len() < 2 { + println!("Usage: ./memory-testing "); + process::exit(1); + } + let base_dir: &Path = args[1].as_ref(); + + let test_string = String::from(memory_testing::TEST_STRING); + + let cases = memory_testing::load_cases(base_dir); + + let mut symmetric_keys = Vec::new(); + let mut symmetric_keys_as_vecs = Vec::new(); + + for case in &cases.symmetric_key { + let key = SymmetricCryptoKey::from_str(&case.key).unwrap(); + symmetric_keys_as_vecs.push(key.to_vec()); + symmetric_keys.push(key); + } + + // 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:?} {symmetric_keys_as_vecs:?}"); + + drop(symmetric_keys); + drop(symmetric_keys_as_vecs); + + // After the variables are dropped, we want to make another dump + wait_for_dump(); + + println!("Done!") +}