Skip to content

Commit 801ab5f

Browse files
committed
fix: fix UB in the scan mem blocks API
The iterators on memory blocks is unsafe as it uses pointers on freed objects. The `mem_block_iterator_first` and `mem_block_iterator_next` both have this code: ``` let mut mem_block = ...; match mem_block.as_mut() { Some(mem_block) { context.mem_block.write(mem_block.as_yara()); context.mem_block.as_mut_ptr() } ... } ``` This is unsafe because the "as_yara" method creates an object that keeps a pointer to the mem_block object, which is freed just afterwards. This can be seen easily by running the tests in valgrind, where the tests using the mem blocks api have this error: ``` ==23549== Invalid read of size 8 ==23549== at 0x1CAAD4: yara::internals::iterator::mem_block_fetch_data (iterator.rs:160) ==23549== by 0x1E8A06: yr_scanner_scan_mem_blocks (scanner.c:479) ==23549== by 0x152458: yara::internals::scan::scanner_scan_mem_blocks_inner (scan.rs:269) ==23549== by 0x1521BF: yara::internals::scan::scanner_scan_mem_blocks (scan.rs:248) ... ``` The `mem_block_fetch_data` dereferences the pointer to the freed object, leading to an invalid read. This is fixed in this commit by avoiding to keep this pointer, and instead directly storing the pointer to the slice data, since this is the value we want to return in `mem_block_fetch_data`. I won't pretend that this makes this API **safe**, since this has to use the yara memory block iterator API, which is very unsafe in how it is declared, and its safety depends on knowledge on how those objects are used internally in YARA. But at least valgrind no longer complains :)
1 parent 14ba7c9 commit 801ab5f

File tree

1 file changed

+14
-21
lines changed

1 file changed

+14
-21
lines changed

src/internals/iterator.rs

Lines changed: 14 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -16,18 +16,16 @@ impl<'a> MemoryBlock<'a> {
1616
Self { base, data }
1717
}
1818

19-
fn as_yara(&mut self) -> YR_MEMORY_BLOCK {
20-
let fetch_data = if self.data.is_empty() {
21-
mem_block_fetch_data_null
22-
} else {
23-
mem_block_fetch_data
24-
};
25-
19+
fn into_yara(self) -> YR_MEMORY_BLOCK {
2620
YR_MEMORY_BLOCK {
2721
base: self.base,
2822
size: self.data.len() as _,
29-
context: self as *mut MemoryBlock as *mut std::os::raw::c_void,
30-
fetch_data: Some(fetch_data),
23+
context: if self.data.is_empty() {
24+
std::ptr::null_mut()
25+
} else {
26+
self.data.as_ptr() as *const _ as *mut _
27+
},
28+
fetch_data: Some(mem_block_fetch_data),
3129
}
3230
}
3331
}
@@ -119,10 +117,10 @@ unsafe extern "C" fn mem_block_iterator_first<T: MemoryBlockIterator>(
119117
iter: *mut YR_MEMORY_BLOCK_ITERATOR,
120118
) -> *mut YR_MEMORY_BLOCK {
121119
let context = &mut *((*iter).context as *mut WrapperMemoryBlockIterator<T>);
122-
let mut mem_block = context.iter.first();
123-
match mem_block.as_mut() {
120+
let mem_block = context.iter.first();
121+
match mem_block {
124122
Some(mem_block) => {
125-
context.mem_block.write(mem_block.as_yara());
123+
context.mem_block.write(mem_block.into_yara());
126124
context.mem_block.as_mut_ptr()
127125
}
128126
None => ptr::null_mut(),
@@ -134,10 +132,10 @@ unsafe extern "C" fn mem_block_iterator_next<T: MemoryBlockIterator>(
134132
) -> *mut YR_MEMORY_BLOCK {
135133
let context = &mut *((*iter).context as *mut WrapperMemoryBlockIterator<T>);
136134
let _ = context.mem_block.assume_init();
137-
let mut mem_block = context.iter.next();
138-
match mem_block.as_mut() {
135+
let mem_block = context.iter.next();
136+
match mem_block {
139137
Some(mem_block) => {
140-
context.mem_block.write(mem_block.as_yara());
138+
context.mem_block.write(mem_block.into_yara());
141139
context.mem_block.as_mut_ptr()
142140
}
143141
None => ptr::null_mut(),
@@ -151,11 +149,6 @@ unsafe extern "C" fn mem_block_iterator_file_size<T: MemoryBlockIteratorSized>(
151149
context.iter.file_size()
152150
}
153151

154-
unsafe extern "C" fn mem_block_fetch_data_null(_: *mut YR_MEMORY_BLOCK) -> *const u8 {
155-
ptr::null()
156-
}
157-
158152
unsafe extern "C" fn mem_block_fetch_data(mem_block: *mut YR_MEMORY_BLOCK) -> *const u8 {
159-
let mem_block = &mut *((*mem_block).context as *mut MemoryBlock);
160-
mem_block.data.as_ptr()
153+
(*mem_block).context as *const u8
161154
}

0 commit comments

Comments
 (0)