The bra-rs from enet4

reading on uninitialized buffer can cause UB (`impl<R> BufRead for GreedyAccessReader<R>`)

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

bra-rs/src/greedy.rs

Lines 190 to 225 in 2b3c455

    
           impl<R> BufRead for GreedyAccessReader<R> 
        
           where 
        
               R: Read, 
        
           { 
        
               fn fill_buf(&mut self) -> IoResult<&[u8]> { 
        
                   if self.buf.capacity() == self.consumed { 
        
                       self.reserve_up_to(self.buf.capacity() + 16); 
        
                   } 
        
                   let b = self.buf.len(); 
        
                   let buf = unsafe { 
        
                       // safe because it's within the buffer's limits 
        
                       // and we won't be reading uninitialized memory 
        
                       std::slice::from_raw_parts_mut( 
        
                           self.buf.as_mut_ptr().offset(b as isize), 
        
                           self.buf.capacity() - b) 
        
                   }; 
        
                   match self.inner.read(buf) { 
        
                       Ok(o) => { 
        
                           unsafe { 
        
                               // reset the size to include the written portion, 
        
                               // safe because the extra data is initialized 
        
                               self.buf.set_len(b + o); 
        
                           } 
        
                           Ok(&self.buf[self.consumed..]) 
        
                       } 
        
                       Err(e) => Err(e), 
        
                   } 
        
               } 
        
               fn consume(&mut self, amt: usize) { 
        
                   self.consumed += amt; 
        
               } 
        
           }

GreedyAccessReader::fill_buf method creates an uninitialized buffer and passes it to user-provided Read implementation (self.inner.read(buf)). 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(), in order to prevent user-provided Read from reading old contents of the newly allocated heap memory.

The version available on Crates.io seems to be different from the latest master branch of this repo,
but the same issue exists in GreedyBufReader::fill_buf() (bra = "0.1.0").

Thank you for checking out this issue 👍

enet4 / bra-rs Goto Github PK

bra-rs's People

Contributors

Stargazers

Watchers

bra-rs's Issues

reading on uninitialized buffer can cause UB (`impl<R> BufRead for GreedyAccessReader<R>`)

Issue Description

Suggested Fix

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent

	impl<R> BufRead for GreedyAccessReader<R>
	where
	R: Read,
	{
	fn fill_buf(&mut self) -> IoResult<&[u8]> {
	if self.buf.capacity() == self.consumed {
	self.reserve_up_to(self.buf.capacity() + 16);
	}

	let b = self.buf.len();
	let buf = unsafe {
	// safe because it's within the buffer's limits
	// and we won't be reading uninitialized memory
	std::slice::from_raw_parts_mut(
	self.buf.as_mut_ptr().offset(b as isize),
	self.buf.capacity() - b)
	};

	match self.inner.read(buf) {
	Ok(o) => {
	unsafe {
	// reset the size to include the written portion,
	// safe because the extra data is initialized
	self.buf.set_len(b + o);
	}

	Ok(&self.buf[self.consumed..])
	}
	Err(e) => Err(e),
	}
	}

	fn consume(&mut self, amt: usize) {
	self.consumed += amt;
	}
	}