From 9bfe4b71d43af9d98480a23cca986b76e00a0ca3 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 17 Aug 2026 05:26:13 +0000 Subject: [PATCH 1/2] fix(scan): stop js-uninitialized-buffer reporting buffers that are filled before use MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../scan/src/__tests__/node-rules.test.ts | 59 +++++++++ packages/scan/src/code-rules.ts | 112 ++++++++++++++++++ packages/scan/src/node-rules.ts | 4 + 3 files changed, 175 insertions(+) diff --git a/packages/scan/src/__tests__/node-rules.test.ts b/packages/scan/src/__tests__/node-rules.test.ts index b7928fe..f2ac657 100644 --- a/packages/scan/src/__tests__/node-rules.test.ts +++ b/packages/scan/src/__tests__/node-rules.test.ts @@ -301,6 +301,65 @@ describe('hardening and resource limits', () => { expect(ruleIds('a.js', 'const buf = Buffer.alloc(1024);')).not.toContain('js-uninitialized-buffer'); }); + it('stays silent when the allocation is copied into before it escapes', () => { + // The shape this rule was reported wrong on: a ring-buffer read that + // allocates at the exact length it is about to write, wrap-around and all. + const source = [ + 'function copy(fromAbsolute, length) {', + ' const out = Buffer.allocUnsafe(length);', + ' if (length === 0) return out;', + ' const retained = Math.min(total, capacity);', + ' const rel = fromAbsolute - (total - retained);', + ' const start = (writePos - retained + rel + capacity * 2) % capacity;', + ' const firstLen = Math.min(length, capacity - start);', + ' buf.copy(out, 0, start, start + firstLen);', + ' if (firstLen < length) buf.copy(out, firstLen, 0, length - firstLen);', + ' return out;', + '}', + ].join('\n'); + expect(ruleIds('a.js', source)).not.toContain('js-uninitialized-buffer'); + }); + + it('stays silent on an allocation filled in the same expression', () => { + expect(ruleIds('a.js', 'const buf = Buffer.allocUnsafe(1024).fill(0);')).not.toContain( + 'js-uninitialized-buffer', + ); + }); + + it('stays silent on an allocation filled through its own methods', () => { + const source = ['const header = Buffer.allocUnsafe(8);', 'header.writeUInt32BE(len, 0);', 'header.writeUInt32BE(crc, 4);'].join('\n'); + expect(ruleIds('a.js', source)).not.toContain('js-uninitialized-buffer'); + }); + + it('still flags when the write lands in a different buffer', () => { + // The guard is bound to the name that was allocated. A fill of something + // else nearby is not evidence about this one, and reading it as evidence + // is how a name-blind window guard exonerates the real defect. + const source = [ + 'const leaked = Buffer.allocUnsafe(1024);', + 'const other = Buffer.alloc(1024);', + 'other.fill(0);', + 'socket.write(leaked);', + ].join('\n'); + expect(ruleIds('a.js', source)).toContain('js-uninitialized-buffer'); + }); + + it('still flags an allocation that escapes with nothing bound to fill', () => { + // No binding to follow, so nothing can be shown to write into it — and + // handing unzeroed heap straight to a caller is the defect itself. + expect(ruleIds('a.js', 'return Buffer.allocUnsafe(size);')).toContain( + 'js-uninitialized-buffer', + ); + expect(ruleIds('a.js', 'socket.write(Buffer.allocUnsafe(size));')).toContain( + 'js-uninitialized-buffer', + ); + }); + + it('still flags an allocation that is never written to', () => { + const source = ['const buf = Buffer.allocUnsafe(1024);', 'res.end(buf);'].join('\n'); + expect(ruleIds('a.js', source)).toContain('js-uninitialized-buffer'); + }); + it('flags an ineffective body limit', () => { expect(ruleIds('a.js', `app.use(express.json({ limit: '50mb' }));`)).toContain( 'js-oversized-request-body-limit', diff --git a/packages/scan/src/code-rules.ts b/packages/scan/src/code-rules.ts index 2596ee3..9988440 100644 --- a/packages/scan/src/code-rules.ts +++ b/packages/scan/src/code-rules.ts @@ -145,6 +145,37 @@ export interface CodeRule { * interpolation just moves the question somewhere this cannot follow. */ constantInterpolationGuard?: boolean; + /** + * Exonerate an uninitialised buffer that is written into before it escapes. + * + * const out = Buffer.allocUnsafe(length); + * src.copy(out, 0, start, start + firstLen); + * return out; + * + * `allocUnsafe` hands back unzeroed heap and the leak it enables is real, but + * "I will fill this myself" is the entire reason the API exists. A rule that + * fires on every call reports correct code as a vulnerability, and both hits + * on the repository this was found against were a PTY ring that writes every + * byte it later hands out. Neither could be silenced without silencing the + * rule everywhere. + * + * Bound to the *name* the allocation is assigned to, not to a write merely + * appearing nearby: an unrelated `.copy(` below an escaping `allocUnsafe` + * must not exonerate it. + * + * The trade is deliberate and worth stating plainly. This cannot prove the + * write covers the whole buffer, so a partial fill — `src.copy(out, 0, 0, 5)` + * into a hundred-byte buffer — is exonerated and still leaks the rest. + * 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 switched off — which catches that partial write + * in exactly the same number of cases, namely none. + * + * An allocation with no binding to follow is never exonerated: + * `return Buffer.allocUnsafe(n)` and `send(Buffer.allocUnsafe(n))` hand the + * unzeroed memory straight out, which is the shape of the actual defect. + */ + filledBeforeUseGuard?: boolean; /** Lines of context searched backwards for guards and required evidence. */ guardBack?: number; /** @@ -1787,6 +1818,85 @@ export function interpolationsAreConstant(line: string, fileText: string): boole ); } +/** + * How far below an allocation a fill is still credibly *the* fill for it. + * + * Sixteen because the shape that motivated this is a ring-buffer read: the + * allocation, an early return for the empty case, three or four lines working + * out the wrap-around offsets, then the copies. Ten lines was not enough for + * it. Past this the write is more likely to belong to something else, and the + * finding should stand. + */ +const FILL_LOOKAHEAD = 16; + +/** `$` is the only name character that also means something to a regex. */ +const escapeName = (name: string): string => name.replace(/\$/g, '\\$&'); + +/** + * The name an uninitialised allocation on this line is bound to, if any. + * + * Declarations and plain assignments both. A property target such as + * `this.buf = Buffer.allocUnsafe(n)` reduces to `buf`, which is how the writes + * that follow will spell it. + */ +function allocationBinding(line: string): string | null { + const bound = + /([A-Za-z_$][\w$]*)\s*=\s*(?:new\s+Buffer\s*\(|Buffer\s*\.\s*allocUnsafe(?:Slow)?\s*\()/.exec( + line, + ); + return bound?.[1] ?? null; +} + +/** + * Does anything write into `name` within the lookahead? + * + * Four spellings, which between them cover how a Node buffer is filled: + * copied into as a destination, filled or written through its own methods, + * `set` from a typed array, or assigned per index. + */ +function writesInto(name: string): RegExp { + const n = escapeName(name); + return new RegExp( + // `src.copy(name, …)` — name is the destination. + `\\.\\s*copy\\s*\\(\\s*${n}\\s*[,)]` + + // `name.fill(…)`, `name.write*(…)`, `name.set(…)`. + `|\\b${n}\\s*\\.\\s*(?:fill|set|write[A-Za-z0-9]*)\\s*\\(` + + // `name[i] = …`, but not `name[i] === …`. + `|\\b${n}\\s*\\[[^\\]\\n]*\\]\\s*=(?!=)`, + ); +} + +/** + * True when the buffer allocated on this line is filled before it escapes. + * + * Looks forward only. Code that fills a buffer runs after the allocation, by + * definition — there is nothing above it to find. + */ +export function bufferFilledBeforeUse(ctx: MatchContext): boolean { + const line = ctx.lines[ctx.index] ?? ''; + + // `Buffer.allocUnsafe(n).fill(0)` — filled in the same breath, and there is + // no binding to follow because none is needed. + if (/\ballocUnsafe(?:Slow)?\s*\([^)\n]*\)\s*\.\s*fill\s*\(/.test(line)) return true; + + const name = allocationBinding(line); + if (!name) return false; + + const written = writesInto(name); + // The allocation line itself first: `const b = Buffer.allocUnsafe(n); b.fill(0);` + // is one line, and a rule that missed it would be answering a question about + // formatting rather than about the code. + if (written.test(line.slice(line.indexOf('=') + 1))) return true; + + const last = Math.min(ctx.lines.length - 1, ctx.index + FILL_LOOKAHEAD); + for (let i = ctx.index + 1; i <= last; i += 1) { + const next = ctx.lines[i] ?? ''; + if (skippable(next, i, ctx.prose)) continue; + if (written.test(next)) return true; + } + return false; +} + export function evaluateRule(rule: CodeRule, ctx: MatchContext): RuleMatch | null { if (rule.languages && !rule.languages.includes(ctx.language)) return null; @@ -1815,6 +1925,8 @@ export function evaluateRule(rule: CodeRule, ctx: MatchContext): RuleMatch | nul return null; } + if (rule.filledBeforeUseGuard && bufferFilledBeforeUse(ctx)) return null; + const guard = rule.guard === undefined ? GENERIC_GUARD : rule.guard; if (guard && (guard.test(line) || guard.test(context))) return null; diff --git a/packages/scan/src/node-rules.ts b/packages/scan/src/node-rules.ts index 8a47658..9c21ad6 100644 --- a/packages/scan/src/node-rules.ts +++ b/packages/scan/src/node-rules.ts @@ -459,6 +459,10 @@ export const NODE_RULES: readonly CodeRule[] = [ languages: ['javascript', 'typescript'], pattern: /\bBuffer\s*\.\s*allocUnsafe(?:Slow)?\s*\(|\bnew\s+Buffer\s*\(\s*(?![`'"])[a-zA-Z_$0-9]/, inherent: true, + // Filling the buffer yourself is the whole reason to call `allocUnsafe`, + // so reporting every call reports correct code. What is left reported is + // an allocation whose bytes are never written before it escapes. + filledBeforeUseGuard: true, }, { id: 'js-oversized-request-body-limit', From cb4b725f7654d9a89a829d933c0efb8c9bed1e6a Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 17 Aug 2026 05:33:08 +0000 Subject: [PATCH 2/2] fix(scan): stop building the fill guard's regex from the binding name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `(? { expect(ruleIds('a.js', source)).toContain('js-uninitialized-buffer'); }); + it('handles a binding whose name contains regex metacharacters', () => { + // `$` is legal in an identifier and meaningful in a pattern. The guard used + // to splice the name into `new RegExp`, so this was the shape that needed + // escaping; it now compares names as strings and needs none. + const source = ['const $buf$ = Buffer.allocUnsafe(8);', '$buf$.fill(0);'].join('\n'); + expect(ruleIds('a.js', source)).not.toContain('js-uninitialized-buffer'); + }); + + it('does not degrade on a long run of identifier characters', () => { + // `[A-Za-z_$][\w$]*` can begin at every position inside a run of `$`, which + // is quadratic without a lookbehind pinning it to where a name can start. + // A scanner that can be stalled by the file it is reading is a denial of + // service in a CI gate. + const source = `const buf = Buffer.allocUnsafe(8);\n${'$'.repeat(20000)}\n`; + const started = Date.now(); + ruleIds('a.js', source); + expect(Date.now() - started).toBeLessThan(2000); + }); + it('flags an ineffective body limit', () => { expect(ruleIds('a.js', `app.use(express.json({ limit: '50mb' }));`)).toContain( 'js-oversized-request-body-limit', diff --git a/packages/scan/src/code-rules.ts b/packages/scan/src/code-rules.ts index 9988440..7fd4661 100644 --- a/packages/scan/src/code-rules.ts +++ b/packages/scan/src/code-rules.ts @@ -1829,8 +1829,20 @@ export function interpolationsAreConstant(line: string, fileText: string): boole */ const FILL_LOOKAHEAD = 16; -/** `$` is the only name character that also means something to a regex. */ -const escapeName = (name: string): string => name.replace(/\$/g, '\\$&'); +/** + * An identifier, matched only where one can actually begin. + * + * The lookbehind is load-bearing rather than decoration. `[A-Za-z_$][\w$]*` + * can start at *every* position inside a run of `$`, so a long run costs a + * restart per character and the match is quadratic in line length. This + * repository reports that class as `redos-nested-quantifier` and CodeQL + * reports it as `js/polynomial-redos`; it should not ship it. + */ +const NAME = String.raw`(? name.replace(/\$/g, '\\$&'); * that follow will spell it. */ function allocationBinding(line: string): string | null { - const bound = - /([A-Za-z_$][\w$]*)\s*=\s*(?:new\s+Buffer\s*\(|Buffer\s*\.\s*allocUnsafe(?:Slow)?\s*\()/.exec( - line, - ); - return bound?.[1] ?? null; + return ALLOCATION_BINDING.exec(line)?.[1] ?? null; } /** - * Does anything write into `name` within the lookahead? + * The spellings that write into a buffer: copied into as a destination, filled + * or written through its own methods, `set` from a typed array, or assigned + * per index. * - * Four spellings, which between them cover how a Node buffer is filled: - * copied into as a destination, filled or written through its own methods, - * `set` from a typed array, or assigned per index. + * Fixed patterns that *capture* a name, rather than a pattern built by + * interpolating the binding into `new RegExp`. The first version did the + * latter and went wrong three ways at once — the name had to be escaped, the + * escaper missed backslashes, and the constructed pattern was itself + * polynomial on a name of many `$`. All three stop existing once the name is + * compared as a string instead of spliced into a regex. */ -function writesInto(name: string): RegExp { - const n = escapeName(name); - return new RegExp( - // `src.copy(name, …)` — name is the destination. - `\\.\\s*copy\\s*\\(\\s*${n}\\s*[,)]` + - // `name.fill(…)`, `name.write*(…)`, `name.set(…)`. - `|\\b${n}\\s*\\.\\s*(?:fill|set|write[A-Za-z0-9]*)\\s*\\(` + - // `name[i] = …`, but not `name[i] === …`. - `|\\b${n}\\s*\\[[^\\]\\n]*\\]\\s*=(?!=)`, - ); +const WRITE_SHAPES: readonly RegExp[] = [ + // `src.copy(name, …)` — name is the destination. + /\.\s*copy\s*\(\s*([A-Za-z_$][\w$]*)\s*[,)]/g, + // `name.fill(…)`, `name.write*(…)`, `name.set(…)`. + new RegExp(`${NAME}\\s*\\.\\s*(?:fill|set|write[A-Za-z0-9]*)\\s*\\(`, 'g'), + // `name[i] = …`, but not `name[i] === …`. + new RegExp(`${NAME}\\s*\\[[^\\]\\n]*\\]\\s*=(?!=)`, 'g'), +]; + +/** Does anything in `text` write into the buffer bound to `name`? */ +function writesInto(text: string, name: string): boolean { + for (const shape of WRITE_SHAPES) { + // Module-level and `g`, so the cursor from the previous call is still on + // it. Reset before use rather than allocating a regex per line. + shape.lastIndex = 0; + for (let found = shape.exec(text); found !== null; found = shape.exec(text)) { + if (found[1] === name) return true; + } + } + return false; } /** @@ -1882,17 +1905,17 @@ export function bufferFilledBeforeUse(ctx: MatchContext): boolean { const name = allocationBinding(line); if (!name) return false; - const written = writesInto(name); // The allocation line itself first: `const b = Buffer.allocUnsafe(n); b.fill(0);` // is one line, and a rule that missed it would be answering a question about - // formatting rather than about the code. - if (written.test(line.slice(line.indexOf('=') + 1))) return true; + // formatting rather than about the code. Only the part after the `=`, so the + // binding on the left is not read as a write to itself. + if (writesInto(line.slice(line.indexOf('=') + 1), name)) return true; const last = Math.min(ctx.lines.length - 1, ctx.index + FILL_LOOKAHEAD); for (let i = ctx.index + 1; i <= last; i += 1) { const next = ctx.lines[i] ?? ''; if (skippable(next, i, ctx.prose)) continue; - if (written.test(next)) return true; + if (writesInto(next, name)) return true; } return false; }