feat(binary): detect memcpy/memmove with a non-constant length (#151)
* test(binary): RED — no rule for memcpy/memmove with a non-constant length 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 #150 * feat(binary): detect memcpy/memmove with a non-constant length 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 #150 * fix(binary): stop B-MEMCPY false-positive on nested-call args; disclose 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 #150 * fix(binary): match pointer casts in B-MEMCPY args; correct disclosed 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 #150
This commit is contained in:
1 parent
afc9dad1b2
commit
6d4e01756a
3 files changed
+73
No files matched your search
@@ -32,9 +32,50 @@ describe('binary/RE decompiled-vuln benchmark', () => {
|
||||
expect(detect('printf(user);').has('B-FORMAT-STRING')).toBe(true);
|
||||
});
|
||||
|
||||
it('flags memcpy/memmove with a non-constant length, not a bounded one', () => {
|
||||
const fires = (c: string) => expect(detect(c).has('B-MEMCPY'), c).toBe(true);
|
||||
const silent = (c: string) => expect(detect(c).has('B-MEMCPY'), c).toBe(false);
|
||||
|
||||
// fires on a variable / field / deref / wire-decoded / cast length
|
||||
fires('memcpy(buf, pkt + 4, len);');
|
||||
fires('memcpy(dst, src, hdr->len);');
|
||||
fires('memcpy(dst, src, ntohl(hdr->len));');
|
||||
fires('memmove(d, s, n);');
|
||||
fires('memcpy(&d, s, (size_t)len);');
|
||||
fires('memcpy(&local_38, param_1, size);');
|
||||
// pointer casts on args 1-2 are the common decompiled shape — must still fire
|
||||
fires('memcpy((void *)dst, src, len);');
|
||||
fires('memmove((uint8_t *)dst, (uint8_t *)src, len);');
|
||||
fires('memcpy((char *)dst + 4, src, len);');
|
||||
// a comma-bearing call in an arg is consumed as one arg, so a non-constant
|
||||
// length after it is still caught
|
||||
fires('memcpy(get_dst(a,b), src, len);');
|
||||
|
||||
// silent when the length is sizeof-bounded or a numeric literal — this is what
|
||||
// forces a discriminating rule rather than a bare /memcpy/ match
|
||||
silent('memcpy(dst, src, sizeof(dst));');
|
||||
silent('memcpy(hdr, src, 4);');
|
||||
silent('memcpy(dst, src, 0x40);');
|
||||
silent('memcpy(dst, src, sizeof(struct hdr));');
|
||||
// a comma inside a nested call must NOT mis-split the arg list and false-fire
|
||||
// on a bounded sizeof length; casts on args 1-2 must not either
|
||||
silent('memcpy(dst, get_src(a,b), sizeof(dst));');
|
||||
silent('memcpy((void *)dst, (void *)src, sizeof(dst));');
|
||||
|
||||
// Disclosed directional limits (see the rule comment): these non-constant
|
||||
// lengths are NOT flagged — pinned here so the scope boundary is auditable and
|
||||
// any future tightening surfaces as a deliberate change, not a silent one.
|
||||
silent('memcpy(d, s, 8 * count);'); // leading-digit computed length
|
||||
silent('memcpy(d, s, sizeof(x) + n);'); // sizeof plus a variable term
|
||||
silent('memcpy(dst, wrap(get(a,b)), len);'); // doubly-nested paren in an arg
|
||||
silent('__memcpy_chk(dst, src, len, dn);'); // _chk variant (usually bounded)
|
||||
silent('wmemcpy(dst, src, len);'); // wide-char variant
|
||||
});
|
||||
|
||||
it('ruleset spans memory-safety + injection + integer-overflow', () => {
|
||||
expect(RULES.length).toBeGreaterThanOrEqual(6);
|
||||
expect(RULES.some((r) => r.id === 'B-CMD-INJECTION')).toBe(true);
|
||||
expect(RULES.some((r) => r.id === 'B-INT-OVERFLOW')).toBe(true);
|
||||
expect(RULES.some((r) => r.id === 'B-MEMCPY')).toBe(true);
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user