mach: Avoid arithmetic overflow and panics (#302)

* avoid arithmetic overflow and panics * avoid slice panic in load cmd * avoid underflow in symbol table parsing * validate export trie load command * Warn on invalid offsets
m4b · Feb 14, 2022 · 52a563b · 52a563b
1 parent 8df6e03
commit 52a563b
Show file tree

Hide file tree

Showing 6 changed files with 120 additions and 17 deletions.
diff --git a/src/mach/exports.rs b/src/mach/exports.rs
@@ -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,
         }
     }
 }
@@ -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);
+    }
 }
diff --git a/src/mach/fat.rs b/src/mach/fat.rs
@@ -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

diff --git 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<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 {

diff --git 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/src/mach/symbols.rs b/src/mach/symbols.rs
@@ -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> {

diff --git a/tests/macho.rs b/tests/macho.rs
@@ -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() {