From fdc5b74c75bbdb32d743ec5d91cba94fc4265dab Mon Sep 17 00:00:00 2001 From: Andrey Mnatsakanov Date: Mon, 27 Jul 2026 17:33:55 +0200 Subject: [PATCH] Harden parsers against malformed input (closes marirs/capa-rs#20) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rule YAML and binaries are untrusted input; these sites panicked or aborted whole-file analysis on malformed data: - rule parser: validate number//offset/ bitness suffix (was &suffix[1..] — slice panic on 'number/', silent truncation over u32); reject bare '/' or '/i' in RegexFeature::new (slice panic); topologically_order_rules returns MatchRuleNotFound instead of panicking on a missing dependency. - count(...): integer arm validated with u32::try_from (was *i as u32, wrapping -1 to u32::MAX); unbalanced 'count(mnemonic(mov' errors instead of silently parsing 'mo'; inline descriptions split on the first ' = ' only. - analysis: per-instruction feature extraction is best-effort — one malformed instruction is logged and skipped instead of aborting the whole file through the rayon loop. - extractors: read_bytes uses checked_sub for offset < base_addr; detect_ascii_len accepts a string ending exactly at EOF (no NUL) instead of erroring out the instruction; is_security_cookie and the self-XOR check guard single-operand formatting; the .NET extractor handles methods without a body and tokens with rid 0. 8 regression tests added (21 total, all passing). --- CHANGELOG.md | 29 +++++++ src/extractor/dnfile.rs | 27 ++++++- src/extractor/smda.rs | 45 ++++++++--- src/lib.rs | 12 ++- src/rules/features.rs | 28 ++++++- src/rules/mod.rs | 163 ++++++++++++++++++++++++++++++++++------ 6 files changed, 269 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index df35aa4..a91080f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,35 @@ This project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html). `u16::from_be_bytes` received the chunk bytes in reverse order, turning every UTF-16BE string into non-ASCII garbage that was then dropped. +### 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 diff --git a/src/extractor/dnfile.rs b/src/extractor/dnfile.rs index 9ec9679..f3a6d79 100644 --- a/src/extractor/dnfile.rs +++ b/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()); + } } diff --git a/src/extractor/smda.rs b/src/extractor/smda.rs index 8edc668..71a6b84 100644 --- a/src/extractor/smda.rs +++ b/src/extractor/smda.rs @@ -1041,7 +1041,7 @@ impl<'data> Extractor<'data> { } if let Some(o) = insn.format_operands() { let operands: Vec = 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 @@ -1616,7 +1616,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; @@ -1649,7 +1653,6 @@ pub fn read_string(report: &DisassemblyReport<'_>, offset: &u64) -> Result, offset: &u64) -> Result { 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") })?; @@ -1664,12 +1667,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) } @@ -1713,7 +1713,9 @@ pub fn is_security_cookie(f: &Function, insn: &Instruction) -> Result { //# security cookie check should use SP or BP if let Some(o) = insn.format_operands() { let operands: Vec = 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() { @@ -2102,4 +2104,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"); + } } diff --git a/src/lib.rs b/src/lib.rs index a953913..d708033 100755 --- a/src/lib.rs +++ b/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") diff --git a/src/rules/features.rs b/src/rules/features.rs index 34bb1fc..c98cecf 100755 --- a/src/rules/features.rs +++ b/src/rules/features.rs @@ -1324,6 +1324,22 @@ fn unicode_safe_byte_escapes(pat: &str) -> String { impl RegexFeature { pub fn new(value: &str, description: &str) -> Result { + // 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). @@ -1795,7 +1811,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() { @@ -1856,4 +1872,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()); + } } diff --git a/src/rules/mod.rs b/src/rules/mod.rs index ae93736..317c44d 100644 --- a/src/rules/mod.rs +++ b/src/rules/mod.rs @@ -396,6 +396,19 @@ impl Rule { } } + /// Bitness suffix of `number/…` / `offset/…` feature keys. The + /// documented form (capa-rules `doc/format.md`) is `x32` / `x64`; + /// a bare number is accepted too. Pre-#20 this was + /// `parse_int(&suffix[1..]) as u32`, which panicked on an empty + /// suffix (`number/`) and silently truncated out-of-range values. + fn parse_bitness_suffix(suffix: &str, key: &str) -> Result { + let digits = suffix.trim(); + let digits = digits.strip_prefix('x').unwrap_or(digits); + digits + .parse::() + .map_err(|_| Error::InvalidRule(line!(), key.to_string())) + } + /// 0.4.2: parse an `N or more` / `N or fewer` count operand and /// validate that the value fits in `u32`. Pre-0.4.2 the code did /// `value as u32` after parsing via `i64`, which silently @@ -473,15 +486,15 @@ impl Rule { "class" => Ok(RuleFeatureType::Class), "arch" => Ok(RuleFeatureType::Arch), _ => { - if key.starts_with("number/") { - let parts: Vec<&str> = key.split('/').collect(); - let bitness = Rule::parse_int(&parts[1].trim()[1..])? as u32; - return Ok(RuleFeatureType::Number(bitness)); + if let Some(suffix) = key.strip_prefix("number/") { + return Ok(RuleFeatureType::Number(Rule::parse_bitness_suffix( + suffix, key, + )?)); } - if key.starts_with("offset/") { - let parts: Vec<&str> = key.split('/').collect(); - let bitness = Rule::parse_int(&parts[1].trim()[1..])? as u32; - return Ok(RuleFeatureType::Offset(bitness)); + if let Some(suffix) = key.strip_prefix("offset/") { + return Ok(RuleFeatureType::Offset(Rule::parse_bitness_suffix( + suffix, key, + )?)); } if let Some(rest) = key .strip_prefix("operand[") @@ -532,7 +545,7 @@ impl Rule { let v; // = ""; //other features can have inline descriptions, like `number: 10 = CONST_FOO`. //in this case, the RHS will be like `10 = CONST_FOO` or some other string - if s.contains(" = ") { + if let Some((value_part, ddd)) = s.split_once(" = ") { if description.is_some() { // there is already a description passed in as a sub node, like: // @@ -540,9 +553,9 @@ impl Rule { // description: CONST_FOO return Err(Error::InvalidRule(line!(), s.to_string())); } - let parts: Vec<&str> = s.split(" = ").collect(); - v = parts[0].trim(); - let ddd = parts[1]; + // split on the FIRST " = " only — the description + // itself may contain the separator (#20). + v = value_part.trim(); if ddd.is_empty() { //# sanity check: //# there is an empty description, like `number: 10 =` @@ -1030,7 +1043,12 @@ impl Rule { let arg = if parts.len() > 1 { parts[1] } else { "" }; let feature_type = Rule::parse_feature_type(term)?; let arg = if !arg.is_empty() { - &arg[..arg.len() - ")".len()] + // `count(mnemonic(mov)` — the whole key still + // ends with ')' so we get here, but the arg + // itself doesn't; blindly stripping the last + // byte would mangle "mov" to "mo" (#20). + arg.strip_suffix(')') + .ok_or_else(|| Error::InvalidRule(line!(), kkey.to_string()))? } else { "" }; @@ -1054,11 +1072,21 @@ impl Rule { // format!("{:?} must be string", vval)))?; match vval { Yaml::Integer(i) => { + // The string count forms are validated by + // `parse_count_u32` (0.4.2); the integer + // arm still did `*i as u32`, silently + // wrapping e.g. -1 to u32::MAX (#20). + let count = u32::try_from(*i).map_err(|_| { + Error::InvalidRule( + line!(), + format!("count value {i} out of range for u32"), + ) + })?; return Ok(StatementElement::Statement(Box::new( Statement::Range(RangeStatement::new( StatementElement::Feature(Box::new(feature)), - *i as u32, - *i as u32, + count, + count, "", )?), ))); @@ -1837,12 +1865,13 @@ pub fn topologically_order_rules(rules: Vec<&Rule>) -> Result> { } let mut rett = vec![]; for dep in rule.get_dependencies(namespaces)? { - rett.append(&mut rec( - rules_by_name[&dep], - seen, - rules_by_name, - namespaces, - )?); + // Public entry point: a caller can pass a rule subset whose + // dependencies aren't all present — return an error instead + // of panicking on the map index (#20). + let dep_rule = rules_by_name + .get(&dep) + .ok_or_else(|| Error::MatchRuleNotFound(dep.clone()))?; + rett.append(&mut rec(dep_rule, seen, rules_by_name, namespaces)?); } rett.push(rule); @@ -2095,4 +2124,96 @@ rule: "unsatisfiable Some-count rule was not pruned: {ns:?}" ); } + + // ————— #20: malformed-rule hardening ————— + + /// `number/` / `offset/` bitness suffixes: the documented + /// `x32` / `x64` forms parse, empty or out-of-range suffixes error + /// (pre-#20 the parser panicked on `number/` — `&suffix[1..]` was + /// out of bounds — and silently truncated over-u32 values). + #[test] + fn bitness_suffix_is_validated_instead_of_panicking() { + assert!(matches!( + Rule::parse_feature_type("number/x32"), + Ok(RuleFeatureType::Number(32)) + )); + assert!(matches!( + Rule::parse_feature_type("offset/x64"), + Ok(RuleFeatureType::Offset(64)) + )); + assert!(Rule::parse_feature_type("number/").is_err()); + assert!(Rule::parse_feature_type("offset/").is_err()); + assert!(Rule::parse_feature_type("number/x18446744073709551616").is_err()); + } + + /// A negative integer count must error (pre-#20 `-1 as u32` + /// silently wrapped to `u32::MAX`), and an unbalanced `count(` + /// argument must error instead of silently dropping its last + /// character (`count(mnemonic(mov)` parsed "mo"). + #[test] + fn malformed_counts_error_instead_of_wrapping_or_mangling() { + let negative = r#" +rule: + meta: + name: bad count + scopes: + static: function + dynamic: process + features: + - and: + - count(mnemonic(mov)): -1 +"#; + assert!( + Rule::from_yaml(negative).is_err(), + "negative count must be rejected" + ); + + let unbalanced = r#" +rule: + meta: + name: bad count paren + scopes: + static: function + dynamic: process + features: + - and: + - count(mnemonic(mov): 2 +"#; + assert!( + Rule::from_yaml(unbalanced).is_err(), + "unbalanced count( argument must be rejected" + ); + } + + /// Inline descriptions split on the FIRST " = " only — the + /// description itself may contain the separator (pre-#20 + /// `split(" = ")` silently dropped the tail). + #[test] + fn inline_description_keeps_tail_after_first_separator() { + let (_value, description) = + Rule::parse_description("CreateFile = opens = files", &RuleFeatureType::Api, &None) + .expect("parse_description"); + assert_eq!(description.as_deref(), Some("opens = files")); + } + + /// A rule whose `match:` dependency is absent from the input must + /// produce an error, not a HashMap-index panic (#20). + #[test] + fn topological_order_missing_dependency_errors_instead_of_panicking() { + let rule = Rule::from_yaml( + r#" +rule: + meta: + name: depends on missing + scopes: + static: function + dynamic: process + features: + - and: + - match: no such rule +"#, + ) + .expect("from_yaml"); + assert!(topologically_order_rules(vec![&rule]).is_err()); + } }