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

Reading uninitialized memory can cause UB (Deserializer::read_vec) #10

Closed
JOE1994 opened this issue Jan 2, 2021 · 2 comments · Fixed by #11
Closed

Reading uninitialized memory can cause UB (Deserializer::read_vec) #10

JOE1994 opened this issue Jan 2, 2021 · 2 comments · Fixed by #11

Comments

@JOE1994
Copy link

JOE1994 commented Jan 2, 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

cdr-rs/src/de.rs

Lines 70 to 77 in 880a281

fn read_vec(&mut self) -> Result<Vec<u8>> {
let len: u32 = de::Deserialize::deserialize(&mut *self)?;
let mut buf = Vec::with_capacity(len as usize);
unsafe { buf.set_len(len as usize) }
self.read_size(u64::from(len))?;
self.reader.read_exact(&mut buf[..])?;
Ok(buf)
}

Deserializer::read_vec method creates an uninitialized buffer and passes it to user-provided Read implementation (self.reader.read_exact). This is unsound, because it allows safe Rust code to exhibit an undefined behavior (read from uninitialized memory).

Suggested Fix

It is safe to zero-initialize the newly allocated part of u8 buffer before read(), in order to prevent user-provided Read from getting access to the old contents from the newly allocated heap memory.

Thank you for checking out this issue 👍

@JOE1994
Copy link
Author

JOE1994 commented Jan 23, 2021

@hrektts Thank you for the fix! Would you also mind publishing a new release containing the fix to crates.io ?

@hrektts
Copy link
Owner

hrektts commented Jan 24, 2021

@JOE1994 It is published. Thank you for your follow-up.

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

Successfully merging a pull request may close this issue.

2 participants