fix(scan): stop js-uninitialized-buffer reporting buffers that are filled before use - #156
Merged
Merged
Conversation
…lled before use
Reported by a maintainer reviewing the scan workflow on mac-developer-bridge,
where both of the rule's hits were correct code — a PTY ring buffer that writes
every byte it later hands out:
const out = Buffer.allocUnsafe(length);
if (length === 0) return out;
buf.copy(out, 0, start, start + firstLen);
if (firstLen < length) buf.copy(out, firstLen, 0, length - firstLen);
return out;
The rule was a bare regex on `Buffer.allocUnsafe(`, so it fired on every call.
Filling the buffer yourself is the entire reason that API exists over
`Buffer.alloc`, which made the rule a report on correct code with no way to
silence one occurrence without silencing all of them.
`filledBeforeUseGuard` exonerates an allocation that is written into before it
escapes: copied into as a destination, filled or written through its own
methods, `set` from a typed array, or assigned per index. It is bound to the
*name* the allocation was assigned to rather than to a write appearing nearby,
because a window guard that only asks "is there a `.copy(` around here" would
exonerate the real defect whenever an unrelated buffer is filled below it.
There is a test for exactly that.
Looks forward only, sixteen lines. Code that fills a buffer runs after the
allocation, and ten lines was not enough for the shape that motivated this —
allocation, early return, three or four lines of wrap-around arithmetic, then
the copies.
An allocation with no binding to follow is still reported. `return
Buffer.allocUnsafe(n)` and `socket.write(Buffer.allocUnsafe(n))` hand unzeroed
heap straight out, which is the defect this rule is for.
The trade is deliberate and documented on the field: this cannot prove the
write covers the whole buffer, so a partial fill is now exonerated and still
leaks the remainder. Proving coverage needs range analysis this engine does not
do. The alternative is the status quo, where the rule fires on every correct
use, is read as noise and gets turned off — catching that partial write in
exactly the same number of cases, namely none.
Verified against the reporting repository at 182fd94: its two
js-uninitialized-buffer findings are gone, its other four findings are still
reported, and an unfilled allocation still flags. 302 tests pass in
packages/scan; typecheck clean.
ThreatCrush Security Scan63 finding(s) HIGH/CRITICAL: 4 | MEDIUM: 52 | LOW: 7
…and 13 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL failed the previous commit with three high-severity alerts, all of them
in the guard it added, and all three the same root cause: the name extracted
from the allocation was spliced into `new RegExp`.
js/regex-injection a pattern built from an extracted value
js/incomplete-sanitization the escaper handled `$` and not backslash
js/polynomial-redos `[A-Za-z_$][\w$]*` restarts at every position
inside a run of `$`, so the match is quadratic
in line length
The escaper only looked sufficient because the name came from a character class
that cannot contain a backslash — an argument that depends on a caller two
functions away and stops being true the first time someone reuses the helper.
Fixed patterns that *capture* a name, compared to the binding as a string.
Nothing is spliced, so nothing needs escaping and there is no constructed
pattern to be polynomial. The identifier shapes are pinned with a `(?<![\w$])`
lookbehind so they can only match where a name actually begins, which is what
makes the scan linear rather than quadratic.
A scanner that can be stalled by the file it is reading is a denial of service
in a CI gate, and this repository reports that class as
`redos-nested-quantifier` — shipping one in a false-positive fix would have
been its own kind of answer.
Two regression tests: a binding named `$buf$`, which is the shape that needed
escaping before and needs none now, and a 20,000-character run of `$` that has
to scan in well under a second.
Behaviour is unchanged. Re-verified against mac-developer-bridge at 182fd94:
still 6 findings → 4, both `js-uninitialized-buffer` hits gone, the other four
still reported, unfilled allocations still flagged. 304 tests pass in
packages/scan; typecheck clean.
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
2 participants
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.
Reported by a maintainer reviewing the scan workflow on mac-developer-bridge#7. Both of the rule's hits on that repository were correct code — a PTY ring buffer that writes every byte it later hands out:
The rule was a bare regex on
Buffer.allocUnsafe(, so it fired on every call. Filling the buffer yourself is the entire reason that API exists overBuffer.alloc, which made this a report on correct code with no way to silence one occurrence without silencing all of them.filledBeforeUseGuardexonerates an allocation written into before it escapes — copied into as a destination, filled or written through its own methods,setfrom a typed array, or assigned per index. It is bound to the name the allocation was assigned to, not to a write appearing nearby: a window guard that only asked "is there a.copy(around here" would exonerate the real defect whenever an unrelated buffer is filled below it. There is a test for exactly that.Forward-only, sixteen lines. Ten was not enough for the shape that motivated it (allocation, early return, wrap-around arithmetic, then the copies).
Still reported: an allocation with no binding to follow.
return Buffer.allocUnsafe(n)andsocket.write(Buffer.allocUnsafe(n))hand unzeroed heap straight out, which is the defect this rule is for.The trade, stated plainly (and documented on the field): this cannot prove the write covers the whole buffer, so a partial fill is now exonerated and still leaks the remainder. Proving coverage needs range analysis this engine does not do. The alternative is the status quo, where the rule fires on every correct use, is read as noise and gets turned off — catching that partial write in exactly the same number of cases, namely none.
Verified against the reporting repository at
182fd94, end to end through the built CLI: 6 findings → 4. Bothjs-uninitialized-bufferhits gone, the other four still reported, and an unfilled allocation still flags. 302 tests pass inpackages/scan, typecheck clean, pre-commit green.Note this does not reach pinned consumers until the pack pin is bumped after a release.