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.

Sign up for GitHub

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

Jump to bottom

fix: Avoid arithmetic overflow and panics #302

Merged
merged 6 commits into from
Feb 14, 2022
Merged

fix: Avoid arithmetic overflow and panics #302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ad71cd4
fix: Avoid arithmetic overflow and panics
Swatinem Feb 4, 2022
0860a44
avoid slice panic in load cmd
Swatinem Feb 4, 2022
5b33de2
avoid underflow in symbol table parsing
Swatinem Feb 4, 2022
fe1298f
validate export trie load command
Swatinem Feb 4, 2022
123d68c
Warn on invalid offsets
Swatinem Feb 7, 2022
7ebd8bd
Merge branch 'master' into fix/overflow
Swatinem Feb 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions src/mach/exports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,10 +277,21 @@ impl<'a> ExportTrie<'a> {
/// Create a new, lazy, zero-copy export trie from the `DyldInfo` `command`
pub fn new(bytes: &'a [u8], command: &load_command::DyldInfoCommand) -> Self {
let start = command.export_off as usize;
let end = (command.export_size + command.export_off) as usize;
// FIXME: Ideally, this should validate `command`, but the best we can
// do for now is return an empty `Range`.
let location = match start
.checked_add(command.export_size as usize)
.and_then(|end| bytes.get(start..end).map(|_| start..end))
{
Some(location) => location,
None => {
log::warn!("Invalid `DyldInfo` `command`.");
0..0
}
};
ExportTrie {
data: bytes,
location: start..end,
location,
}
}
}
Expand Down Expand Up @@ -319,4 +330,15 @@ mod tests {
println!("len: {} exports: {:#?}", exports.len(), &exports);
assert_eq!(exports.len() as usize, 3usize)
}

#[test]
fn invalid_range() {
let mut command = load_command::DyldInfoCommand::default();
command.export_off = 0xffff_ff00;
command.export_size = 0x00ff_ff00;
let trie = ExportTrie::new(&[], &command);
// FIXME: it would have been nice if this were an `Err`.
let exports = trie.exports(&[]).unwrap();
assert_eq!(exports.len(), 0);
}
}
14 changes: 12 additions & 2 deletions src/mach/fat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,19 @@ impl fmt::Debug for FatArch {
impl FatArch {
/// Get the slice of bytes this header describes from `bytes`
pub fn slice<'a>(&self, bytes: &'a [u8]) -> &'a [u8] {
// FIXME: This function should ideally validate the inputs and return a `Result`.
// Best we can do for now without `panic`ing is return an empty slice.
let start = self.offset as usize;
let end = (self.offset + self.size) as usize;
&bytes[start..end]
match start
.checked_add(self.size as usize)
.and_then(|end| bytes.get(start..end))
{
Some(slice) => slice,
None => {
log::warn!("invalid `FatArch` offset");
&[]
}
}
}

/// Returns the cpu type
Expand Down
5 changes: 4 additions & 1 deletion src/mach/imports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,10 @@ impl<'a> Debug for BindInterpreter<'a> {
impl<'a> BindInterpreter<'a> {
/// Construct a new import binding interpreter from `bytes` and the load `command`
pub fn new(bytes: &'a [u8], command: &load_command::DyldInfoCommand) -> Self {
let get_pos = |off: u32, size: u32| -> Range<usize> { off as usize..(off + size) as usize };
let get_pos = |off: u32, size: u32| -> Range<usize> {
let start = off as usize;
start..start.saturating_add(size as usize)
};
let location = get_pos(command.bind_off, command.bind_size);
let lazy_location = get_pos(command.lazy_bind_off, command.lazy_bind_size);
BindInterpreter {
Expand Down
20 changes: 11 additions & 9 deletions src/mach/load_command.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,25 +507,27 @@ impl<'a> ctx::TryFromCtx<'a, Endian> for ThreadCommand {
let flavor: u32 = bytes.pread_with(8, le)?;
let count: u32 = bytes.pread_with(12, le)?;

if count > 70 {
return Err(error::Error::Malformed(format!(
"thread command specifies {} longs for thread state but we handle only 70",
count
)));
}

// get a byte slice of the thread state
let thread_state_byte_length = count as usize * 4;
let thread_state_bytes = &bytes[16..16 + thread_state_byte_length];

// check the length
if thread_state_bytes.len() < thread_state_byte_length {
if bytes.len() < 16 + thread_state_byte_length {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i know you didn't introduce 16 but it might be nice to turn this into a const since it's used again below and will be puzzling to future reviewers

return Err(error::Error::Malformed(format!(
"thread command specifies {} bytes for thread state but has only {}",
thread_state_byte_length,
thread_state_bytes.len()
)));
}
if count > 70 {
return Err(error::Error::Malformed(format!(
"thread command specifies {} longs for thread state but we handle only 70",
count
bytes.len()
)));
}

let thread_state_bytes = &bytes[16..16 + thread_state_byte_length];

// read the thread state
let mut thread_state: [u32; 70] = [0; 70];
for (i, state) in thread_state.iter_mut().enumerate().take(count as usize) {
Expand Down
9 changes: 6 additions & 3 deletions src/mach/symbols.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,15 +428,18 @@ impl<'a> Symbols<'a> {
ctx: container::Ctx,
) -> error::Result<Symbols<'a>> {
// we need to normalize the strtab offset before we receive the truncated bytes in pread_with
let strtab = symtab.stroff - symtab.symoff;
Ok(bytes.pread_with(
let strtab = symtab
.stroff
.checked_sub(symtab.symoff)
.ok_or_else(|| error::Error::Malformed("invalid symbol table offset".into()))?;
bytes.pread_with(
symtab.symoff as usize,
SymbolsCtx {
nsyms: symtab.nsyms as usize,
strtab: strtab as usize,
ctx,
},
)?)
)
}

pub fn iter(&self) -> SymbolIterator<'a> {
Expand Down
63 changes: 63 additions & 0 deletions tests/macho.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,69 @@ fn relocations() {
assert_eq!(reloc.is_extern(), true);
}

#[test]
fn fat_mach_overflow() {
// See https://github.com/getsentry/symbolic/pull/497
let data = [
0xbe, 0xba, 0xfe, 0xca, // magic cafebabe
0x00, 0x00, 0x00, 0x01, // num arches = 1
0x00, 0x00, 0x00, 0x00, // cpu type
0x00, 0x00, 0x00, 0x00, // cpu subtype
0x00, 0xff, 0xff, 0xff, // offset
0x00, 0x00, 0xff, 0xff, // size
0x00, 0x00, 0x00, 0x00, // align
];

let fat = MultiArch::new(&data).unwrap();
assert!(fat.get(0).is_err());

for arch in fat.iter_arches() {
let _ = arch.unwrap().slice(&data);
}
}

#[test]
fn imports_overflow() {
let data = [
0xFE, 0xED, 0xFA, 0xCF, 0x4C, 0xA8, 0x45, 0x4C, 0x42, 0x00, 0xFF, 0xFF, 0xFF, 0xFF, 0x73,
0x6F, 0x00, 0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x4D, 0x4F, 0x44, 0x55, 0x4C, 0x40, 0x20,
0x00, 0x00, 0x00, 0x00, 0x00, 0x22, 0x00, 0x00, 0x00, 0x56, 0x50, 0xC2, 0xC2, 0xC2, 0xC2,
0xC2, 0xC2, 0xC2, 0xC2, 0xC2, 0xC2, 0xC2, 0x54, 0xC2, 0x4D, 0x4F, 0x44, 0x55, 0x4C, 0x40,
0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x5B, 0x78, 0x00, 0x00, 0x00, 0x00, 0x2B, 0xC2, 0x4D,
0x4F, 0x44, 0x55, 0x4C, 0x45, 0x20, 0xC2, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x1F,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x1F, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x1F, 0x00, 0x00, 0x00, 0x00, 0x00, 0x1F, 0x1F,
];
assert!(MachO::parse(&data, 0).is_err());
}

#[test]
fn panic_load_cmd() {
let data = [
0xfe, 0xed, 0xfa, 0xce, 0x4c, 0x45, 0x20, 0x0, 0x0, 0x0, 0x0, 0x0, 0x33, 0x5b, 0x0, 0x0,
0x0, 0x0, 0x0, 0x7, 0x0, 0x0, 0x0, 0x4d, 0x4f, 0x44, 0x55, 0x4c, 0x45, 0x20, 0x0, 0x0, 0x0,
0x0, 0x0, 0x4, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x12, 0xd, 0x1a, 0x44, 0x53, 0x0, 0x0,
0x0, 0x0, 0x0, 0x7, 0x0, 0x0, 0x0, 0x0, 0x61, 0x73, 0x6d, 0x6f, 0x73, 0x6f, 0x0, 0x0, 0x0,
0x0, 0x0, 0x2a, 0x50, 0x4b, 0x5, 0x6, 0x0, 0xe0, 0x73, 0x2f, 0x43, 0x3, 0x74, 0x53, 0x1f,
0x1f, 0x1f, 0x1f, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18,
0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18,
];
assert!(Mach::parse(&data).is_err());
}

#[test]
fn invalid_sym_offset() {
let data = [
0xFE, 0xED, 0xFA, 0xCF, 0x4C, 0x45, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x33, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x4D, 0x4F, 0x44, 0x55, 0x4C, 0x45, 0x20,
0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xB7, 0x01, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x09,
0x00, 0x00, 0x00, 0x00, 0x2E, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x66, 0x20, 0xFF,
0xFF, 0xFF, 0x84, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x1F, 0x1F, 0x1E, 0x5D,
];
assert!(Mach::parse(&data).is_err());
}

// See https://github.com/getsentry/symbolic/issues/479
#[test]
fn fuzzed_memory_growth() {
Expand Down