-
Notifications
You must be signed in to change notification settings - Fork 169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add fuzz test to handle length math issues with Messages and Headers #1273
Changes from 12 commits
a118e91
041093f
91cf2ed
e2fc684
6d92b56
72407e1
421ef81
a9acfdb
f98c827
82b8183
eac43d4
2a128de
9cafae2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
#!/bin/bash | ||
|
||
set -o errexit | ||
|
||
source ./.evergreen/env.sh | ||
|
||
cargo install cargo-fuzz |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
#!/bin/bash | ||
|
||
set -o errexit | ||
|
||
source ./.evergreen/env.sh | ||
|
||
mkdir -p artifacts | ||
|
||
# Function to run fuzzer and collect crashes | ||
run_fuzzer() { | ||
target=$1 | ||
echo "Running fuzzer for $target" | ||
# Run fuzzer and redirect crashes to artifacts directory | ||
RUST_BACKTRACE=1 cargo +nightly fuzz run $target -- \ | ||
-rss_limit_mb=4096 \ | ||
-max_total_time=360 \ | ||
-artifact_prefix=artifacts/ \ | ||
-print_final_stats=1 | ||
} | ||
|
||
run_fuzzer header_length |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
target | ||
corpus | ||
artifacts | ||
coverage |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
[package] | ||
name = "mongodb-fuzz" | ||
version = "0.0.0" | ||
publish = false | ||
edition = "2021" | ||
|
||
[package.metadata] | ||
cargo-fuzz = true | ||
|
||
[dependencies] | ||
libfuzzer-sys = "0.4" | ||
mongodb = { path = "..", features = ["fuzzing"] } | ||
|
||
[[bin]] | ||
name = "header_length" | ||
path = "fuzz_targets/header_length.rs" | ||
test = false | ||
doc = false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
#![no_main] | ||
use libfuzzer_sys::fuzz_target; | ||
use mongodb::cmap::conn::wire::{ | ||
header::{Header, OpCode}, | ||
message::Message, | ||
}; | ||
|
||
fuzz_target!(|data: &[u8]| { | ||
if data.len() < Header::LENGTH { | ||
return; | ||
} | ||
if let Ok(mut header) = Header::from_slice(data) { | ||
// read_from_slice will adjust the data for the header length, this first check will | ||
// almost always have length mismatches, but length mismatches are a possible attack | ||
// vector. | ||
// This will also often have the wrong opcode, but that is also a possible attack vector. | ||
if let Ok(message) = Message::read_from_slice(data, header.clone()) { | ||
let _ = message; | ||
} | ||
// try again with the header.length set to the data length and the header.opcode == | ||
// OpCode::Message to catch other attack vectors. | ||
header.length = data.len() as i32 - Header::LENGTH as i32; | ||
header.op_code = OpCode::Message; | ||
if let Ok(message) = Message::read_from_slice(data, header) { | ||
let _ = message; | ||
} | ||
} | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -158,12 +158,14 @@ fn num_decimal_digits(mut n: usize) -> usize { | |
|
||
/// Read a document's raw BSON bytes from the provided reader. | ||
pub(crate) fn read_document_bytes<R: Read>(mut reader: R) -> Result<Vec<u8>> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are the actual bugs found by fuzzing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should be getting a warning when we use risky |
||
let length = reader.read_i32_sync()?; | ||
let length = Checked::new(reader.read_i32_sync()?); | ||
|
||
let mut bytes = Vec::with_capacity(length as usize); | ||
bytes.write_all(&length.to_le_bytes())?; | ||
let mut bytes = Vec::with_capacity(length.try_into()?); | ||
bytes.write_all(&length.try_into::<u32>()?.to_le_bytes())?; | ||
|
||
reader.take(length as u64 - 4).read_to_end(&mut bytes)?; | ||
reader | ||
.take((length - 4).try_into()?) | ||
.read_to_end(&mut bytes)?; | ||
|
||
Ok(bytes) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,6 +80,7 @@ impl Action for ParseConnectionString { | |
"srvMaxHosts and loadBalanced=true cannot both be present", | ||
)); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did a quick pass for any other |
||
// max is u32, so this is safe. | ||
config.hosts = crate::sdam::choose_n(&config.hosts, max as usize) | ||
.cloned() | ||
.collect(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,13 @@ | ||
#[cfg(test)] | ||
pub(crate) mod test; | ||
|
||
#[cfg(feature = "fuzzing")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. allow missing just so we don't see warning when we run the fuzz test |
||
#[allow(missing_docs)] | ||
pub mod conn; | ||
|
||
#[cfg(not(feature = "fuzzing"))] | ||
pub(crate) mod conn; | ||
|
||
mod connection_requester; | ||
pub(crate) mod establish; | ||
mod manager; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,26 @@ | ||
#[cfg(feature = "fuzzing")] | ||
#[allow(missing_docs)] | ||
pub mod header; | ||
#[cfg(not(feature = "fuzzing"))] | ||
mod header; | ||
#[cfg(feature = "fuzzing")] | ||
#[allow(missing_docs)] | ||
pub mod message; | ||
#[cfg(not(feature = "fuzzing"))] | ||
pub(crate) mod message; | ||
#[cfg(feature = "fuzzing")] | ||
#[allow(missing_docs)] | ||
pub mod util; | ||
#[cfg(not(feature = "fuzzing"))] | ||
mod util; | ||
|
||
pub(crate) use self::{ | ||
message::{Message, MessageFlags}, | ||
util::next_request_id, | ||
}; | ||
pub(crate) use self::util::next_request_id; | ||
|
||
#[cfg(feature = "fuzzing")] | ||
pub use self::message::Message; | ||
|
||
#[cfg(feature = "fuzzing")] | ||
pub use crate::fuzz::message_flags::MessageFlags; | ||
|
||
#[cfg(not(feature = "fuzzing"))] | ||
pub use message::{Message, MessageFlags}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to add any of this to the public API, even if it's behind a feature flag. Can we move the contents of
fuzz
to withinsrc/test
and put any fuzzing additions to the message/header code behindcfg(test)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this isn't possible because of the way cargo fuzz works (I wish it were, it's an issue with how it uses LLVM's fuzzing support, I think). However, you did bring to my attention that I was accidentally leaking some of this publicly when not fuzzing, so I have fixed that up.