Skip to content
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

Deserialisation of a Decimal128 allows user provided readers to view uninitalised data #251

Closed
5225225 opened this issue Apr 21, 2021 · 1 comment
Labels
tracked-in-jira Ticket filed in Mongo's Jira system

Comments

@5225225
Copy link

5225225 commented Apr 21, 2021

The following lines are unsound when given a user defined reader who is allowed to read from the buffer local_buf.

bson-rust/src/de/mod.rs

Lines 164 to 173 in 0ebb96c

#[cfg(feature = "decimal128")]
#[inline]
fn read_f128<R: Read + ?Sized>(reader: &mut R) -> Result<Decimal128> {
use std::mem;
let mut local_buf: [u8; 16] = unsafe { mem::MaybeUninit::uninit().assume_init() };
reader.read_exact(&mut local_buf)?;
let val = unsafe { Decimal128::from_raw_bytes_le(local_buf) };
Ok(val)
}

I can't run a test case in miri, but the following fuzzing harness with --sanitizer=memory given will find it.

#![no_main]
#[macro_use] extern crate libfuzzer_sys;
extern crate bson;

use bson::Document;
use std::io::Cursor;

fuzz_target!(|buf: &[u8]| {
    struct HostileReader<R> {
        reader: R,
    }

    impl<R: std::io::Read> std::io::Read for HostileReader<R> {
        fn read(&mut self, buf: &mut [u8]) -> Result<usize, std::io::Error> {
            let v: usize = buf.iter().copied().map(|x| x as usize).sum();

            self.reader.read(buf)
        }
    }

    let mut r = HostileReader { reader: Cursor::new(&buf[..]) };

    let _ = Document::from_reader(&mut r);
});

An implementation of std::io::Read which reads from the buffer given instead of purely writing to it is discouraged, but Read is a safe trait, so it's not unsafe to do this.

There's the https://doc.rust-lang.org/std/io/trait.Read.html#method.initializer method, but it's unstable, tracking issue: rust-lang/rust#42788

Failing that, I think the only reasonable thing to do is to just zero out the buffer (just like what's done above for the case where decimal128 is not enabled).


As a side note, does anyone know why decimal::d128::from_raw_bytes is unsafe? We're calling it with completely unchecked values from the user, so if there's any values for which it can cause UB, that doesn't sound safe. I might open an issue on their end asking for clarification as to why it's unsafe.

I had a look at the documentation there there's no indication as to what you need to do to be safe calling it. Looks like from_hex does basically the same thing as from_raw_bytes, so it shouldn't be unsafe?

@Alexander-L-G Alexander-L-G added the tracked-in-jira Ticket filed in Mongo's Jira system label Apr 26, 2021
@abr-egn
Copy link
Contributor

abr-egn commented Jun 10, 2021

Thanks for the report! This has been fixed in commit a9eba30.

@abr-egn abr-egn closed this as completed Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tracked-in-jira Ticket filed in Mongo's Jira system
Projects
None yet
Development

No branches or pull requests

3 participants