From 9d6fa34dc48e8aec50f5725c5d857bee9f3f6b0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa?= Date: Mon, 11 Mar 2024 11:36:42 +0100 Subject: [PATCH] [PM-6100] Test for memory leaks of secrets (#641) ## Type of change ``` - [ ] Bug fix - [x] New feature development - [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc) - [ ] Build/deploy pipeline (DevOps) - [ ] Other ``` ## Objective Added a small test framework to test for secret leaks in memory. This consists of a few parts: - Binary crate `memory-testing`, this compiles to a binary that uses `bitwarden_crypto` to create some keys in memory and then frees them. The execution of this program goes like this: - Starts, keys get defined in memory - Waits for input (This is where we create an initial core dump) - Program frees the keys - Waits for input (This is where we create a final core dump) - Program exits normally - A `capture_dumps.py` Python script, it's purpose is starting the program and orchestrating the core dumps and sending inputs to the program to continue. - A `Dockerfile` that will compile the program and run the `capture_dumps.py` script, this is needed because the dumps only work on a Linux environment. - A `test.py` script that analyzes the memory dumps for secrets in memory - A `run_tests.sh` script that builds and runs the docker container and the test script in one invocation I've tried other tools to run it natively on other operating systems like osxpmem on mac and they either don't work on ARM Macs or they require running as root and disabling System Integrity Protection. I've also added a small workflow to run these tests, as that runs on a linux environment, it's run directly without docker. The results are printed to a table now: ![image](https://github.com/bitwarden/sdk/assets/725423/699b25bb-b184-4d46-aec1-e785df53cc88) --- .github/codecov.yml | 1 + .github/workflows/memory-testing.yml | 43 ++++++ .gitignore | 1 - Cargo.lock | 18 +++ crates/memory-testing/.gitignore | 1 + crates/memory-testing/Cargo.toml | 13 ++ crates/memory-testing/Dockerfile | 26 ++++ crates/memory-testing/Dockerfile.dockerignore | 4 + crates/memory-testing/cases.json | 9 ++ crates/memory-testing/run_test.sh | 20 +++ .../memory-testing/src/bin/analyze-dumps.rs | 132 ++++++++++++++++++ .../memory-testing/src/bin/capture-dumps.rs | 70 ++++++++++ crates/memory-testing/src/lib.rs | 29 ++++ crates/memory-testing/src/main.rs | 44 ++++++ 14 files changed, 410 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/memory-testing.yml create mode 100644 crates/memory-testing/.gitignore create mode 100644 crates/memory-testing/Cargo.toml create mode 100644 crates/memory-testing/Dockerfile create mode 100644 crates/memory-testing/Dockerfile.dockerignore create mode 100644 crates/memory-testing/cases.json create mode 100755 crates/memory-testing/run_test.sh create mode 100644 crates/memory-testing/src/bin/analyze-dumps.rs create mode 100644 crates/memory-testing/src/bin/capture-dumps.rs create mode 100644 crates/memory-testing/src/lib.rs create mode 100644 crates/memory-testing/src/main.rs 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!") +}