fix(scan): stop js-uninitialized-buffer reporting buffers that are filled before use

packages/scan/src/__tests__/node-rules.test.ts

@@ -301,6 +301,84 @@ 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('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',

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,108 @@ 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;
+
+/**
+ * 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`(?<![\w$])([A-Za-z_$][\w$]*)`;
+
+const ALLOCATION_BINDING = new RegExp(
+  `${NAME}\\s*=\\s*(?:new\\s+Buffer\\s*\\(|Buffer\\s*\\.\\s*allocUnsafe(?:Slow)?\\s*\\()`,
+);
+
+/**
+ * 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 {
+  return ALLOCATION_BINDING.exec(line)?.[1] ?? null;
+}
+
+/**
+ * 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.
+ *
+ * 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.
+ */
+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;
+}
+
+/**
+ * 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;
+
+  // 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. 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 (writesInto(next, name)) 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 +1948,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;
 

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',