Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Reading on uninitialized buffer may cause UB ( read_entry() ) #10

Closed
JOE1994 opened this issue Jan 8, 2021 · 0 comments · Fixed by #12
Closed

Reading on uninitialized buffer may cause UB ( read_entry() ) #10

JOE1994 opened this issue Jan 8, 2021 · 0 comments · Fixed by #12

Comments

@JOE1994
Copy link

JOE1994 commented Jan 8, 2021

Hello 🦀,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Issue Description

fn read_entry<F>(frame: &Frame, read_at: &mut F) -> Result<ReadResult, Error>
where
F: FnMut(&mut [u8], u64) -> io::Result<usize>,
{
// Entry is [payload size: u64, payload ]
let mut buf = Vec::with_capacity(frame.data_size);
unsafe { buf.set_len(frame.data_size) };
let n = read_at(&mut buf, frame.data_start())?;

fn read_entry<ByteType, F>(frame: &Frame, read_at: &mut F) -> Result<ReadResult, Error>
where
F: FnMut(&mut [u8], u64) -> io::Result<usize>,
{
// Entry is [payload size: u32, payload, payload_size: u32, next_offset: ByteType]
let tail_size = size_of_frame_tail::<ByteType>();
let to_read = frame.data_size + tail_size;
let mut buf = Vec::with_capacity(to_read);
unsafe { buf.set_len(to_read) };
let n = read_at(&mut buf, frame.data_start())?;

Methods go_offset_log::read_entry() & offset_log::read_entry() create an uninitialized buffer and passes it to user-provided F . This is unsound, because it allows safe Rust code to exhibit an undefined behavior (read from uninitialized memory).

This part from the Read trait documentation explains the issue:

It is your responsibility to make sure that buf is initialized before calling read. Calling read with an uninitialized buf (of the kind one obtains via MaybeUninit<T>) is not safe, and can lead to undefined behavior.

Suggested Fix

It is safe to zero-initialize the newly allocated u8 buffer before read_at(), in order to prevent user-provided code from accessing old contents of the newly allocated heap memory.

Also, there are two nightly features for handling such cases.

Thank you for checking out this issue 👍

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant