Skip to content

Harden rule/binary parsers against malformed input - #21

Open
mnaza wants to merge 2 commits into
marirs:masterfrom
mnaza:fix/malformed-input-robustness
Open

Harden rule/binary parsers against malformed input#21
mnaza wants to merge 2 commits into
marirs:masterfrom
mnaza:fix/malformed-input-robustness

Conversation

@mnaza

@mnaza mnaza commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Closes #20.

Rule YAML and binaries are untrusted input, but several sites panicked or aborted the whole analysis on malformed data. All fixes are small and local; 8 regression tests added (21 pass, was 13).

Rule YAML parser

  • number/ / offset/ bitness suffix validated (src/rules/mod.rs): was parse_int(&suffix[1..]) as u32 — slice-out-of-bounds panic on an empty suffix, silent truncation over u32. Now a strict parse accepting the documented x32/x64 forms (capa-rules doc/format.md) plus bare numbers, erroring otherwise.
  • Bare string: / / /i rejected up front (src/rules/features.rs): passed StringFactory's starts/ends-with-/ check, then &value[1..len-1] panicked in RegexFeature::new. "//" (empty body) stays legal, matching Python.
  • topologically_order_rules returns MatchRuleNotFound instead of panicking on the map index when a dependency is absent from the input — the function is public and can be called on rule subsets.
  • count(...) integer arm validated with u32::try_from: -1 silently wrapped to u32::MAX (the string forms were hardened in 0.4.2 via parse_count_u32; the integer arm was missed).
  • Unbalanced count(mnemonic(mov) errors instead of silently dropping the last byte of the argument (movmo).
  • Inline descriptions split on the first " = " only (split_once), so a description containing the separator keeps its tail.

Binary analysis

  • One malformed instruction no longer aborts whole-file analysis (src/lib.rs): per-instruction feature-extraction errors are logged via the logger and skipped (best-effort) instead of propagating ? through the rayon loop.
  • read_bytes uses checked_sub for offset < base_addr (matches the checked version detect_ascii_len already had).
  • detect_ascii_len accepts a string ending exactly at end-of-buffer (no trailing NUL — common in truncated/packed binaries) instead of reporting a bogus buffer-overflow that aborted the instruction's features; read_bytes already clamps.
  • is_security_cookie and the self-XOR check guard single-operand formatting (operands.len() < 2).
  • .NET extractor (src/extractor/dnfile.rs): get_blocks no longer indexes instructions[0] on methods without a body; resolve_dotnet_token uses checked_sub for rid 0 (ECMA-335 "no reference").

Tests

  • bitness_suffix_is_validated_instead_of_panicking, malformed_counts_error_instead_of_wrapping_or_mangling, inline_description_keeps_tail_after_first_separator, topological_order_missing_dependency_errors_instead_of_panicking (rules)
  • regex_without_body_errors_instead_of_panicking (features)
  • read_bytes_below_image_base_errors_instead_of_underflowing, read_string_at_exact_end_of_buffer (smda extractor)
  • resolve_token_with_zero_rid_errors_instead_of_underflowing (dnfile extractor)

Clippy clean, cargo fmt applied, CHANGELOG entry under [Unreleased]. Based on master; independent of #17 and #19 except for the trivial shared [Unreleased] CHANGELOG heading.

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).
@mnaza
mnaza force-pushed the fix/malformed-input-robustness branch from 7ff1434 to fdc5b74 Compare July 31, 2026 08:57
@marirs

marirs commented Aug 1, 2026

Copy link
Copy Markdown
Owner

conflicts :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Robustness: panics on malformed rule YAML and malformed binaries; one bad instruction aborts whole-file analysis

2 participants