Skip to content

Commit

Permalink
[PM-6100] Test for memory leaks of secrets (#641)
Browse files Browse the repository at this point in the history
## 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)
  • Loading branch information
dani-garcia authored Mar 11, 2024
1 parent b115e26 commit 9d6fa34
Show file tree
Hide file tree
Showing 14 changed files with 410 additions and 1 deletion.
1 change: 1 addition & 0 deletions .github/codecov.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
ignore:
- "crates/sdk-schemas" # Tool
- "crates/uniffi-bindgen" # Tool
- "crates/memory-testing" # Testing
43 changes: 43 additions & 0 deletions .github/workflows/memory-testing.yml
Original file line number Diff line number Diff line change
@@ -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
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ x64/
x86/
build/
bld/
[Bb]in/
[Oo]bj/
*.wasm

Expand Down
18 changes: 18 additions & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions crates/memory-testing/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
output
13 changes: 13 additions & 0 deletions crates/memory-testing/Cargo.toml
Original file line number Diff line number Diff line change
@@ -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"
26 changes: 26 additions & 0 deletions crates/memory-testing/Dockerfile
Original file line number Diff line number Diff line change
@@ -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" ]
4 changes: 4 additions & 0 deletions crates/memory-testing/Dockerfile.dockerignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
*
!crates/*
!Cargo.toml
!Cargo.lock
9 changes: 9 additions & 0 deletions crates/memory-testing/cases.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"symmetric_key": [
{
"key": "FfhVVP8fmFIZY1WmRszPmRmVCxXNWVcJffPrbkywTPtBNkgfhYGT+D9sVGizYXrPffuj2yoyWqMwF9iF5aMQhQ==",
"decrypted_key_hex": "15f85554ff1f9852196355a646cccf9919950b15cd5957097df3eb6e4cb04cfb",
"decrypted_mac_hex": "4136481f858193f83f6c5468b3617acf7dfba3db2a325aa33017d885e5a31085"
}
]
}
20 changes: 20 additions & 0 deletions crates/memory-testing/run_test.sh
Original file line number Diff line number Diff line change
@@ -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
132 changes: 132 additions & 0 deletions crates/memory-testing/src/bin/analyze-dumps.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
use std::{env, fmt::Display, io, path::Path, process};

use memory_testing::*;

fn find_subarrays(needle: &[u8], haystack: &[u8]) -> Vec<usize> {
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::<Vec<String>>()
.join(", ")
}

fn add_row<N: Display>(
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<String> = env::args().collect();
if args.len() < 2 {
println!("Usage: ./analyze-dumps <base_dir>");
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<u8> = 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<u8> = hex::decode(&case.decrypted_key_hex).unwrap();
let mac_part: Vec<u8> = hex::decode(&case.decrypted_mac_hex).unwrap();
let key_in_b64: Vec<u8> = 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 });
}
70 changes: 70 additions & 0 deletions crates/memory-testing/src/bin/capture-dumps.rs
Original file line number Diff line number Diff line change
@@ -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<u64> {
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<String> = std::env::args().collect();
if args.len() < 3 {
println!("Usage: ./capture_dumps <binary_path> <base_dir>");
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));
}
Loading

0 comments on commit 9d6fa34

Please sign in to comment.