Skip to content

Commit

Permalink
Improve memory testing (#766)
Browse files Browse the repository at this point in the history
## Type of change
```
- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other
```

## Objective
Improve how the memory-testing crate does some things:
- Compile and run the process in release mode, to better match what our
final artifacts will do.
- Improve the structure of `Cases` struct, now the values that we want
to look up in memory are generic, which greatly simplifies the
`analyze-dumps` binary.
- Removed the test string from the table output, if we don't find the
test string in the dump we assume the dump is invalid and return
directly.
- Instead of waiting a predefined amount of time, the dump program now
waits for the program to notify that it's ready, which speeds up the
operations a bit.
- Use `black_box` instead of just printing the values, to ensure the
compiler doesn't optimize them out.
  • Loading branch information
dani-garcia authored May 3, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent b36cf6f commit 7e57f19
Showing 9 changed files with 194 additions and 227 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

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

2 changes: 1 addition & 1 deletion crates/memory-testing/Cargo.toml
Original file line number Diff line number Diff line change
@@ -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"] }
6 changes: 3 additions & 3 deletions crates/memory-testing/Dockerfile
Original file line number Diff line number Diff line change
@@ -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" ]
91 changes: 64 additions & 27 deletions crates/memory-testing/cases.json
Original file line number Diff line number Diff line change
@@ -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": "[email protected]",
"name": "Master Key PBKDF2",
"master_key": {
"password": "123412341234",
"email": "[email protected]",

"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": "[email protected]",
"name": "Master Key Argon2",
"master_key": {
"password": "asdfasdfasdf",
"email": "[email protected]",

"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"
}
]
}
]
}
6 changes: 3 additions & 3 deletions crates/memory-testing/run_test.sh
Original file line number Diff line number Diff line change
@@ -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
149 changes: 25 additions & 124 deletions crates/memory-testing/src/bin/analyze-dumps.rs
Original file line number Diff line number Diff line change
@@ -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<usize> {
@@ -31,26 +30,6 @@ fn comma_sep(nums: &[usize]) -> 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 {
@@ -72,114 +51,36 @@ fn main() -> io::Result<()> {

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(),
);

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}");
Loading

0 comments on commit 7e57f19

Please sign in to comment.