Harden rule/binary parsers against malformed input

CHANGELOG.md

@@ -43,6 +43,35 @@ This project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   hottest operation of the rule engine. All sites now do one
   `HashMap::get`, matching the pattern `RangeStatement` already used.
 
+### Fixed — robustness on malformed input (closes [#20](https://github.com/marirs/capa-rs/issues/20))
+
+Rule YAML and binaries are untrusted input; these sites panicked or
+aborted whole-file analysis on malformed data:
+
+- **Rule parser panics** `number/` / `offset/` with an empty bitness
+  suffix sliced out of bounds (`&suffix[1..]`); the suffix is now
+  validated (`x32`/`x64` per capa-rules `doc/format.md`, bare numbers
+  accepted, out-of-range rejected). A bare `string: /` or `/i` panicked
+  in `RegexFeature::new` — now rejected up front. `topologically_order_rules`
+  panicked on a dependency missing from the input — now returns
+  `MatchRuleNotFound`.
+- **Silent wrap-around and mangling in `count(...)`** an integer count
+  of `-1` wrapped to `u32::MAX` (the string forms were hardened in
+  0.4.2, the integer arm was missed), and an unbalanced
+  `count(mnemonic(mov)` silently parsed the argument as `mo`. Both now
+  raise `InvalidRule`. Inline descriptions split on the first `" = "`
+  only, keeping a tail that itself contains the separator.
+- **One malformed instruction no longer aborts whole-file analysis**
+  per-instruction feature extraction errors are logged and skipped
+  (best-effort) instead of propagating through the rayon loop.
+- **Extractor panics / false errors** `read_bytes` underflowed on an
+  offset below the image base; `detect_ascii_len` reported a printable
+  string ending exactly at end-of-buffer (no NUL) as an overflow error,
+  aborting the instruction's features; `is_security_cookie` and the
+  self-XOR check indexed `operands[1]` on single-operand formatting;
+  the .NET extractor panicked on methods without a body
+  (`instructions[0]`) and underflowed on tokens with rid 0.
+
 ## [0.5.2] — xor-zero number(0), regex /i fast path, rule pre-pruning
 
 ### Fixed — feature extraction parity

src/extractor/dnfile.rs

@@ -103,7 +103,11 @@ impl super::Function for Function {
         for i in &self.f.instructions {
             insts.push(Box::new(Instruction { i: i.clone() }));
         }
-        res.insert(self.f.instructions[0].offset as u64, insts);
+        // Methods without a body (abstract / P/Invoke stubs) have no
+        // instructions — indexing `[0]` would panic (#20).
+        if let Some(first) = self.f.instructions.first() {
+            res.insert(first.offset as u64, insts);
+        }
         Ok(res)
     }
     fn as_any(&self) -> &dyn std::any::Any {
@@ -1282,7 +1286,13 @@ pub fn resolve_dotnet_token<'a>(
     //    }
     if let cil::instruction::Operand::Token(t) = token {
         let table = pe.net()?.md_table_by_index(&t.table())?;
-        return Ok(table.get_row(t.rid() - 1)?.get_row().as_any());
+        // rid 0 ("no reference" in ECMA-335) would underflow the `- 1`
+        // (#20); row indices are 1-based.
+        let row_index = t
+            .rid()
+            .checked_sub(1)
+            .ok_or_else(|| crate::Error::InvalidToken(format!("{:?}", token)))?;
+        return Ok(table.get_row(row_index)?.get_row().as_any());
     }
     Err(crate::Error::InvalidToken(format!("{:?}", token)))
 }
@@ -1356,4 +1366,17 @@ mod tests {
             total_fields
         );
     }
+
+    /// #20: a token with rid == 0 (ECMA-335 "no reference") must error,
+    /// not underflow `rid - 1`.
+    #[test]
+    fn resolve_token_with_zero_rid_errors_instead_of_underflowing() {
+        let path = "data/dotnet_1c444ebeba24dcba8628b7dfe5fec7c6.exe_";
+        let bytes =
+            std::fs::read(path).unwrap_or_else(|e| panic!("test fixture missing: {path}: {e}"));
+        let pe = DnPe::parse(&bytes).expect("dnfile parse dotnet fixture");
+        // table 0x06 (MethodDef), rid 0
+        let token = cil::instruction::Operand::Token(clr::token::Token::new(0x0600_0000));
+        assert!(resolve_dotnet_token(&pe, &token).is_err());
+    }
 }

src/extractor/smda.rs

@@ -1063,7 +1063,7 @@ impl<'data> Extractor<'data> {
         }
         if let Some(o) = insn.format_operands() {
             let operands: Vec<String> = o.split(',').map(|s| s.trim().to_string()).collect();
-            if operands[0] == operands[1] {
+            if operands.len() >= 2 && operands[0] == operands[1] {
                 // 0.5.2 (upstream parity #2997): `xor eax, eax` (and the SSE
                 // / packed variants Xorpd/Xorps/Pxor) zero the destination
                 // register. Emit Number(0) for rules matching the produced
@@ -1638,7 +1638,11 @@ pub fn read_bytes<'a>(
     num_bytes: usize,
 ) -> Result<&'a [u8]> {
     let raw = report.binary_info.raw_data;
-    let rva = offset - report.base_addr;
+    // checked_sub: an offset below the image base is invalid input,
+    // not a huge RVA (#20; matches detect_ascii_len's checked version).
+    let rva = offset
+        .checked_sub(report.base_addr)
+        .ok_or(Error::BufferOverflowError)?;
     let buffer_end = raw.len();
     let mut end_of_string = rva + num_bytes as u64;
 
@@ -1671,7 +1675,6 @@ pub fn read_string(report: &DisassemblyReport<'_>, offset: &u64) -> Result<Strin
 
 pub fn detect_ascii_len(report: &DisassemblyReport<'_>, offset: &u64) -> Result<usize> {
     let raw = report.binary_info.raw_data;
-    let buffer_len = raw.len() as u64;
     let rva = offset.checked_sub(report.base_addr).ok_or_else(|| {
         std::io::Error::other("Offset is out of bounds relative to the base address")
     })?;
@@ -1686,12 +1689,9 @@ pub fn detect_ascii_len(report: &DisassemblyReport<'_>, offset: &u64) -> Result<
         .take_while(|&&ch| b"0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!\"#$%&'()*+, -./:;<=>?@[\\]^_`{|}~ \r\n".contains(&ch))
         .count();
 
-    if rva + ascii_len as u64 >= buffer_len {
-        Err(std::io::Error::other(
-            "Buffer overflow detected while detecting ASCII length",
-        ))?;
-    }
-
+    // A string running right up to the end of the buffer with no
+    // trailing NUL is valid (common in truncated/packed binaries);
+    // `read_bytes` already clamps the final read (#20).
     Ok(ascii_len)
 }
 
@@ -1735,7 +1735,9 @@ pub fn is_security_cookie(f: &Function, insn: &Instruction) -> Result<bool> {
     //# security cookie check should use SP or BP
     if let Some(o) = insn.format_operands() {
         let operands: Vec<String> = o.split(',').map(|s| s.trim().to_string()).collect();
-        if !["esp", "ebp", "rsp", "rbp"].contains(&&operands[1][..]) {
+        // Malformed/single-operand formatting must not panic on
+        // `operands[1]` — just treat it as "not a security cookie" (#20).
+        if operands.len() < 2 || !["esp", "ebp", "rsp", "rbp"].contains(&&operands[1][..]) {
             return Ok(false);
         }
         for (index, block) in f.get_blocks()?.iter().enumerate() {
@@ -2124,4 +2126,27 @@ mod tests {
             "UTF-16BE string not decoded at offset 8: {strings:?}"
         );
     }
+
+    /// #20: `read_bytes` on an offset below the image base must return
+    /// an error, not underflow `offset - base_addr`.
+    #[test]
+    fn read_bytes_below_image_base_errors_instead_of_underflowing() {
+        let data = vec![0u8; 0x100];
+        let extractor =
+            Extractor::from_buffer(&data, 0x1000, 64, false, false).expect("parse buffer");
+        assert!(read_bytes(extractor.report(), &0x800, 4).is_err());
+    }
+
+    /// #20: a printable string ending exactly at the end of the buffer
+    /// (no trailing NUL — common in truncated/packed binaries) must be
+    /// returned, not reported as a buffer-overflow error.
+    #[test]
+    fn read_string_at_exact_end_of_buffer() {
+        let mut data = vec![0u8; 0x10];
+        data.extend_from_slice(b"WXYZ"); // runs right up to EOF, no NUL
+        let extractor =
+            Extractor::from_buffer(&data, 0x1000, 64, false, false).expect("parse buffer");
+        let s = read_string(extractor.report(), &(0x1000 + 0x10)).expect("read_string at EOF");
+        assert_eq!(s, "WXYZ");
+    }
 }

src/lib.rs

@@ -1038,7 +1038,17 @@ fn find_function_capabilities<'a>(
 
         let insns = extractor.get_instructions(f, &bb)?;
         for insn in insns.iter() {
-            for (feature, va) in extractor.extract_insn_features(f, insn)? {
+            // Best-effort (#20): a single malformed instruction (bad
+            // operand encoding, truncated read, …) must not abort
+            // analysis of the whole file — log and continue.
+            let insn_features = match extractor.extract_insn_features(f, insn) {
+                Ok(features) => features,
+                Err(e) => {
+                    logger(&format!("skipping instruction feature extraction: {e}"));
+                    continue;
+                }
+            };
+            for (feature, va) in insn_features {
                 if features_dump {
                     map_features_by_scope
                         .entry("instruction")

src/rules/features.rs

@@ -1315,6 +1315,22 @@ fn unicode_safe_byte_escapes(pat: &str) -> String {
 
 impl RegexFeature {
     pub fn new(value: &str, description: &str) -> Result<RegexFeature> {
+        // A bare "/" (or "/i") passes StringFactory's starts/ends-with-'/'
+        // check but has no body — the `&value[1..len-1]` slice below
+        // would be out of bounds (#20). "//" (empty body) stays legal,
+        // matching Python's `value[1:-1]`.
+        let case_insensitive = value.ends_with("/i");
+        let min_len = if case_insensitive {
+            "//i".len()
+        } else {
+            "//".len()
+        };
+        if value.len() < min_len {
+            return Err(Error::InvalidRule(
+                line!(),
+                format!("malformed regex feature: {value}"),
+            ));
+        }
         let body = &value["/".len()..value.len() - "/".len()];
         // 0.3.21: pre-0.3.21 we prepended `(?-u)` to put the regex into
         // byte mode (matched the `mnaza/fancy-regex` fork's behaviour).
@@ -1786,7 +1802,7 @@ impl FeatureT for FormatFeature {
 
 #[cfg(test)]
 mod tests {
-    use super::unicode_safe_byte_escapes;
+    use super::{RegexFeature, unicode_safe_byte_escapes};
 
     #[test]
     fn passes_ascii_byte_escapes_through_unchanged() {
@@ -1847,4 +1863,14 @@ mod tests {
         let expected = "中文\\u{80}";
         assert_eq!(unicode_safe_byte_escapes(pat), expected);
     }
+
+    /// #20: a bare "/" or "/i" passes StringFactory's starts/ends-with-'/'
+    /// check but has no body — pre-#20 `&value[1..len-1]` sliced out of
+    /// bounds. "//" (empty body) stays legal, matching Python.
+    #[test]
+    fn regex_without_body_errors_instead_of_panicking() {
+        assert!(RegexFeature::new("/", "").is_err());
+        assert!(RegexFeature::new("/i", "").is_err());
+        assert!(RegexFeature::new("//", "").is_ok());
+    }
 }