feat(binary): detect memcpy/memmove with a non-constant length - #151
Open
lyubomir-bozhinov wants to merge 4 commits into
Open
feat(binary): detect memcpy/memmove with a non-constant length#151lyubomir-bozhinov wants to merge 4 commits into
lyubomir-bozhinov wants to merge 4 commits into
Conversation
…ngth Adds two corpus fixtures + direct assertions for a memcpy/memmove sink: - memcpy-wire-length (bad): a wire-decoded length into a fixed buffer, expect B-MEMCPY. Fails on main — the ruleset has no memcpy rule. - memcpy-bounded-control (clean): two safe memcpys (sizeof-bounded + a constant length). This forces a DISCRIMINATING rule: a naive /memcpy/ match false-positives here and fails the zero-false-positive gate. Refs elder-plinius#150
Adds B-MEMCPY to the decompiled-vuln ruleset — the classic unbounded-copy overflow. The framework's own reasoning layer treats this as its textbook target (src/orchestration/prompts.ts:60,80 — 'a wire-controlled length flowing into an unchecked memcpy'; orchestration/index.ts:11), but the static pre-filter had no rule for it: strcpy was covered, its memcpy(dst, src, len) sibling was not. The rule inspects the length (3rd) argument and fires only when it is non-constant — a variable, struct field, deref, or computed/decoded value — staying silent on a sizeof(...) or numeric-literal length. Validated against a 22-case dangerous/safe matrix (perfect discrimination). Corpus 6+2 -> 7+3; the new sizeof/constant-bounded control keeps the rule honest — a bare /memcpy/ match would false-positive on it. Closes elder-plinius#150
…se limits Adversarial review found a real false positive: a comma inside a nested call in arg 1/2 (memcpy(dst, get_src(a,b), sizeof(dst))) mis-split the naive [^,]+,[^,]+, arg list, so a sizeof-BOUNDED call fired. Restricting args 1-2 to [^,()]+ (paren-free) removes it — verified the bounded nested-call form is now silent while every common non-constant length still fires. The trade is a memcpy whose 1st/2nd arg is itself a call is not matched (an acceptable miss for a directional pre-filter, now disclosed). Rule comment rewritten to honestly enumerate the directional misses (leading- digit computed length, sizeof-plus-term, nested-call args, inline comments, __memcpy_chk/memcpy_s/wmemcpy variants). Test expanded into an in-tree discrimination matrix (fires / silent / pinned disclosed-limit cases) so the scope boundary is auditable and any future tightening surfaces deliberately. Refs elder-plinius#150
…limits A second adversarial pass on the shipped rule caught that the previous paren-free tightening ([^,()]+) over-corrected: it also stopped matching pointer CASTS on args 1-2 — memcpy((void *)dst, (void *)src, len) — which are the most common memcpy shape in real decompiled output. The rule was silently missing the main case. Replace the arg matcher with one level of BALANCED parens, (?:[^,()]|\([^()]*\))+, so a cast (void *)dst and a comma-bearing call get(a,b) are each consumed as ONE arg. This fires on casts again, still fires on a call-bearing arg with a non-constant length (now a true positive, previously a disclosed miss), and keeps the original nested-call-with-bounded-length false positive silent. Verified no catastrophic backtracking on pathological input. Also corrects the disclosed-limits comment: the previous 'arg is itself a call' miss is now only DOUBLY-nested parens, and the inline-comment note was backwards (a comment obscuring a sizeof/literal false-fires, it does not cause a miss) — restated accurately, with decompiled output not emitting inline comments noted. Tests pin the cast fires and the variant exclusions. Refs elder-plinius#150
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #150.
Adds
B-MEMCPYto the Binary/RE decompiled-vuln detector (scripts/binary-vuln-bench.mjs) — flagsmemcpy/memmovecalls whose length argument is non-constant (a variable, struct field, deref, or decoded value), the classic unbounded-copy overflow.strcpywas covered; itsmemcpy(dst, src, len)sibling was the gap.Why this one
The reasoning layer already treats this sink as its textbook target:
src/orchestration/prompts.ts:60— refuses on "a wire-controlled length flowing into an unchecked memcpy"src/orchestration/prompts.ts:80— "the smoking-gun line (e.g. the unchecked memcpy of a wire length)"src/orchestration/index.ts:11— example question: "Does this function validate the length parameter before the memcpy?"So the LLM adjudication layer is built around this pattern, but the fast static pre-filter couldn't surface it as a candidate at all.
The rule
It inspects the third argument: fires on
len/hdr->len/*p/ntohl(hdr->len)/(size_t)len, stays silent onsizeof(dst)/16/0x40. Two subtleties are load-bearing and documented in-code: the[^)\s]after the lookahead stops\s*from backtracking onto whitespace (which would leak every bounded form through), and args 1–2 match a single level of balanced parens ((?:[^,()]|\([^()]*\))+) so a pointer cast(void *)dstand a comma-bearing callget(a,b)are each consumed as one arg — neither mis-splits the list into a false positive on a bounded length. (Pointer casts on args 1–2 are the common decompiled shape; the first cut used a paren-free arg matcher and missed them — caught by a second adversarial review pass.)Honest scope
This is a directional static pre-filter over decompiled output — a candidate sink, matching the existing corpus's stated posture. It is not taint analysis and does not prove the length is attacker-reachable (that is the reasoning layer's job). Disclosed misses, enumerated in the rule comment and pinned as tests so the boundary is auditable:
8*count) or withsizeofplus a term (sizeof(x)+n)wrap(get(a,b)))__memcpy_chk/memcpy_s/wmemcpyvariants (the_chkform is usually the bounded, safe one)Each disclosed limit is pinned by a test so the boundary is auditable. Mid-argument
/* ... */comments aren't stripped (a comment obscuring asizeof/literal would false-fire), but decompiled output does not emit inline comments.Out of scope for this rule, as separate follow-ups:
strcat, exec-family injection, additive/shift overflow precursors.Tests — RED-first, discriminating
a0b7198): a bad fixture (memcpywith a wire-decoded length) and a clean control with two safe memcpys (sizeof-bounded + a constant length) go into the corpus, plus direct assertions. The clean control is what forces a real regex — a bare/memcpy/rule false-positives on it and fails the zero-false-positive gate, so the test isn't a tautology. Both fail onmain.bf7b7d6): the singleRULESentry.12022c5): a first adversarial review found a false positive on nested-call args (memcpy(dst, get_src(a,b), sizeof(dst))— a bounded call that fired); expanded the test into an in-tree fires/silent/disclosed-limit matrix.966a46f): a second adversarial review (on the shipped state) found the paren-free tightening had over-corrected and stopped matching pointer casts — the common decompiled shape. Switched args 1–2 to balanced-paren matching, which fixes the original FP and the cast miss together. Validated against a 31-case fuzz oracle + a ReDoS sanity check.Corpus goes 6+2 → 7+3; the
>=6floors stay satisfied.Verification (macOS, Apple Silicon)
node scripts/binary-vuln-bench.mjsself-test: 7/7 seeded sinks caught, 3/3 controls clean, 0 false-positives.npm test: 755 vitest tests green (+ ops-preflight 11/11, model-matrix, refusal-frontier 19/19).npm run typecheck: clean.eslinton the changed TS test file: clean. (The.mjs's pre-existing node-globalsno-undeflint is unchanged by this diff and outside CI'seslint src/**/*.tsscope.)macOS-neutral: the rule is a regex over committed text fixtures — no platform-specific behavior. I can only verify on macOS, so I'm not claiming Linux/Windows runs I didn't do.