From ad71cd4a5e4aa9a195636baa2406277f298bcffc Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Fri, 4 Feb 2022 14:07:04 +0100 Subject: [PATCH 1/5] fix: Avoid arithmetic overflow and panics --- src/mach/fat.rs | 6 ++++-- src/mach/imports.rs | 5 ++++- tests/macho.rs | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/src/mach/fat.rs b/src/mach/fat.rs index e5430fa04..30657b1f1 100644 --- a/src/mach/fat.rs +++ b/src/mach/fat.rs @@ -89,9 +89,11 @@ 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] + let end = start.saturating_add(self.size as usize); + bytes.get(start..end).unwrap_or_default() } /// Returns the cpu type diff --git a/src/mach/imports.rs b/src/mach/imports.rs index 7044cd582..3c31329a9 100644 --- a/src/mach/imports.rs +++ b/src/mach/imports.rs @@ -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 { off as usize..(off + size) as usize }; + let get_pos = |off: u32, size: u32| -> Range { + 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 { diff --git a/tests/macho.rs b/tests/macho.rs index d3af91208..7e0387f9b 100644 --- a/tests/macho.rs +++ b/tests/macho.rs @@ -577,3 +577,39 @@ fn relocations() { assert_eq!(reloc.is_pic(), true); 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()); +} From 0860a44c3d006141629bd6ba197d723b85c077e5 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Fri, 4 Feb 2022 14:21:29 +0100 Subject: [PATCH 2/5] avoid slice panic in load cmd --- src/mach/load_command.rs | 20 +++++++++++--------- tests/macho.rs | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/mach/load_command.rs b/src/mach/load_command.rs index 0496fb2c6..063c2c19c 100644 --- a/src/mach/load_command.rs +++ b/src/mach/load_command.rs @@ -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 { 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) { diff --git a/tests/macho.rs b/tests/macho.rs index 7e0387f9b..8dad20a1a 100644 --- a/tests/macho.rs +++ b/tests/macho.rs @@ -613,3 +613,17 @@ fn imports_overflow() { ]; 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()); +} From 5b33de274a6121d37c424d45887e771017253509 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Fri, 4 Feb 2022 15:12:52 +0100 Subject: [PATCH 3/5] avoid underflow in symbol table parsing --- src/mach/symbols.rs | 9 ++++++--- tests/macho.rs | 13 +++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/mach/symbols.rs b/src/mach/symbols.rs index a5ed33e3e..7f2425d68 100644 --- a/src/mach/symbols.rs +++ b/src/mach/symbols.rs @@ -428,15 +428,18 @@ impl<'a> Symbols<'a> { ctx: container::Ctx, ) -> error::Result> { // 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> { diff --git a/tests/macho.rs b/tests/macho.rs index 8dad20a1a..40ed51ebd 100644 --- a/tests/macho.rs +++ b/tests/macho.rs @@ -627,3 +627,16 @@ fn panic_load_cmd() { ]; 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()); +} From fe1298fd63b9a8a94395bc958776a9dc435b8c7f Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Fri, 4 Feb 2022 16:08:47 +0100 Subject: [PATCH 4/5] validate export trie load command --- src/mach/exports.rs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/mach/exports.rs b/src/mach/exports.rs index 54ace9758..36302a676 100644 --- a/src/mach/exports.rs +++ b/src/mach/exports.rs @@ -274,10 +274,16 @@ 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; + let end = start.saturating_add(command.export_size as usize); + // FIXME: Ideally, this should validate `command`, but the best we can + // do for now is return an empty `Range`. + let mut location = start..end; + if bytes.get(location.clone()).is_none() { + location = 0..0; + } ExportTrie { data: bytes, - location: start..end, + location, } } } @@ -316,4 +322,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); + } } From 123d68cc9ae8ec6b2890c2c5a9d50dd8c19d8f7c Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 7 Feb 2022 11:19:39 +0100 Subject: [PATCH 5/5] Warn on invalid offsets --- src/mach/exports.rs | 15 ++++++++++----- src/mach/fat.rs | 12 ++++++++++-- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/mach/exports.rs b/src/mach/exports.rs index 36302a676..9687f61e0 100644 --- a/src/mach/exports.rs +++ b/src/mach/exports.rs @@ -274,13 +274,18 @@ 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 = start.saturating_add(command.export_size as usize); // FIXME: Ideally, this should validate `command`, but the best we can // do for now is return an empty `Range`. - let mut location = start..end; - if bytes.get(location.clone()).is_none() { - location = 0..0; - } + 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, diff --git a/src/mach/fat.rs b/src/mach/fat.rs index 30657b1f1..33d945748 100644 --- a/src/mach/fat.rs +++ b/src/mach/fat.rs @@ -92,8 +92,16 @@ impl FatArch { // 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 = start.saturating_add(self.size as usize); - bytes.get(start..end).unwrap_or_default() + 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