pe: Fix potential out-of-bounds read in unwind/POGO info parser (#498)

* Fix potential out-of-bounds read in unwind info parser
* remove -1 bias in bound check
* Add more offset check; preserve error semantics
* Add test for malformed name

Author: kkent030315

src/pe/debug.rs

@@ -810,35 +810,39 @@ impl<'a> Iterator for POGOEntryIterator<'a> {
             Err(error) => return Some(Err(error.into())),
         };
 
+        // Use >= to avoid empty slice, that we want to emit an error early here for
+        // malformed name in a POGO entry.
         if offset >= self.data.len() {
             return Some(Err(error::Error::Malformed(format!(
-                "Offset {:#x} is too big for containing name field of POGO entry (rva {:#x} and size {:#X})",
-                offset, rva, size
+                "Offset {offset:#x} is too big for containing name field of POGO entry (rva {rva:#x} and size {size:#X})",
             ))));
         }
         let name = match self.data[offset..].iter().position(|&b| b == 0) {
             Some(pos) => {
-                if offset + pos as usize >= self.data.len() {
+                // + 1 nul
+                let Some(name) = self.data.gread_with::<&[u8]>(&mut offset, pos + 1).ok() else {
                     return Some(Err(error::Error::Malformed(format!(
-                        "Null-terminator for POGO entry (rva {:#x} and size {:#X}) found but exceeds iterator buffer",
-                        rva, size
+                        "Null-terminator for POGO entry (rva {rva:#x} and size {size:#X}) found but exceeds iterator buffer",
                     ))));
-                }
-                let name = &self.data[offset..offset + pos + 1];
-                offset = offset + pos + 1;
-                // Align to the next u32 boundary
-                offset = (offset + 3) & !3; // Round up to the nearest multiple of 4
+                };
+                // Round up to the nearest multiple of 4
+                offset = (offset + 3) & !3;
                 name
             }
             None => {
                 return Some(Err(error::Error::Malformed(format!(
-                    "Cannot find null-terimnator for POGO entry (rva {:#x} and size {:#X})",
-                    rva, size
+                    "Cannot find null-terminator for POGO entry (rva {rva:#x} and size {size:#X})",
                 ))
                 .into()));
             }
         };
 
+        if offset > self.data.len() {
+            return Some(Err(error::Error::Malformed(format!(
+                "Offset {offset:#x} exceeds buffer length {:#x}",
+                self.data.len()
+            ))));
+        }
         self.data = &self.data[offset..];
         Some(Ok(POGOInfoEntry { rva, size, name }))
     }
@@ -853,7 +857,8 @@ mod tests {
         IMAGE_DEBUG_TYPE_CODEVIEW, IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS, IMAGE_DEBUG_TYPE_ILTCG,
         IMAGE_DEBUG_TYPE_POGO, IMAGE_DEBUG_TYPE_REPRO, IMAGE_DEBUG_TYPE_VC_FEATURE,
         IMAGE_DLLCHARACTERISTICS_EX_CET_COMPAT, IMAGE_DLLCHARACTERISTICS_EX_CET_COMPAT_STRICT_MODE,
-        ImageDebugDirectory, POGO_SIGNATURE_SIZE, POGOInfoEntry, ReproInfo, VCFeatureInfo,
+        ImageDebugDirectory, POGO_SIGNATURE_SIZE, POGOEntryIterator, POGOInfoEntry, ReproInfo,
+        VCFeatureInfo,
     };
 
     const NO_DEBUG_DIRECTORIES_BIN: &[u8] =
@@ -1100,4 +1105,42 @@ mod tests {
         ];
         assert_eq!(entries, entries_expect);
     }
+
+    #[rustfmt::skip]
+    const MALFORMED_POGO_NAME: &[u8; 83] = &[
+        // Entry 1: .text$mn
+        0x00, 0x10, 0x00, 0x00, // RVA: 0x00001000
+        0x03, 0x00, 0x00, 0x00, // Size: 0x00000003
+        0x2e, 0x74, 0x65, 0x78, // Name: ".text$mn\0"
+        0x74, 0x24, 0x6d, 0x6e,
+        0x00, 0x00, 0x00, 0x00,
+        // Entry 2: .rdata
+        0x00, 0x20, 0x00, 0x00, // RVA: 0x00002000
+        0xa8, 0x00, 0x00, 0x00, // Size: 0x000000A8
+        0x2e, 0x72, 0x64, 0x61, // Name: ".rdata\0"
+        0x74, 0x61, 0x00, 0x00,
+        // Entry 3: .rdata$voltmd
+        0xa8, 0x20, 0x00, 0x00, // RVA: 0x000020A8
+        0x18, 0x00, 0x00, 0x00, // Size: 0x00000018
+        0x2e, 0x72, 0x64, 0x61, // Name: ".rdata$voltmd\0"
+        0x74, 0x61, 0x24, 0x76,
+        0x6f, 0x6c, 0x74, 0x6d,
+        0x64, 0x00, 0x00, 0x00,
+        // Entry 4: .rdata$zzzdbg
+        0xc0, 0x20, 0x00, 0x00, // RVA: 0x000020C0
+        0xcc, 0x00, 0x00, 0x00, // Size: 0x000000CC
+        0x2e, 0x72, 0x64, 0x61, // Name: ".rdata$zzzdbg\0"
+        0x74, 0x61, 0x24, 0x7a,
+        0x7a, 0x7a, 0x64, 0x62,
+        0x67, 0x00, 0x00, /* truncated 0x00 */
+    ];
+
+    #[test]
+    #[should_panic = "Malformed(\"Offset 0x18 exceeds buffer length 0x17\")"]
+    fn parse_debug_pogo_malformed_name() {
+        let entries = POGOEntryIterator {
+            data: MALFORMED_POGO_NAME,
+        };
+        let _ = entries.collect::<Result<Vec<_>, _>>().unwrap();
+    }
 }

src/pe/exception.rs

@@ -710,6 +710,11 @@ impl<'a> UnwindInfo<'a> {
         // handler is called as part of the search for an exception handler or as part of an unwind.
         } else if flags & (UNW_FLAG_EHANDLER | UNW_FLAG_UHANDLER) != 0 {
             let address = bytes.gread_with::<u32>(&mut offset, scroll::LE)?;
+            if offset > bytes.len() {
+                return Err(error::Error::Malformed(format!(
+                    "Offset {offset:#x} is too big to contain unwind handlers",
+                )));
+            }
             let data = &bytes[offset..];
 
             handler = Some(if flags & UNW_FLAG_EHANDLER != 0 {

src/pe/resource.rs

@@ -668,6 +668,7 @@ impl<'a> Iterator for ResourceStringIterator<'a> {
                     "Parsed next resource string as size {:#x}: {:#x?}",
                     offset, next?
                 );
+                // Using offset from `ResourceString::parse` so this is infailable.
                 self.data = &self.data[offset..];
                 Ok(next?)
             }