fix(llm-security): compileRules coerces a non-string pattern instead of dropping the rule
new RegExp(pattern, 'i') never throws when pattern is a truthy non-string
(e.g. an object) — it ToString-coerces it first. The truthy-only guard
(`!rule.pattern`) let such a rule through as a real, compiled RegExp,
bypassing the try/catch meant to drop malformed rules. Worse than a silent
drop: `new RegExp("[object Object]", "i")` is parsed as a character class
over o/b/j/e/c/t/space, so the "dropped" rule instead becomes a
near-universal false-positive matcher. Same path for the built-in
commons-backed ruleset and the operator's sig.custom_rules_path (both
route through compileRules).
Fix: require typeof rule.pattern === 'string' before compiling. Verified
the golden dump pins no rule that exists only because of this coercion —
it regenerates byte-identically after the fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iWrdLSVgRhgPzTB29rQGD
This commit is contained in:
parent
eceb71bbb3
commit
f926071348
3 changed files with 63 additions and 5 deletions
|
|
@ -51,10 +51,15 @@ import { loadArtifact } from './commons-loader.mjs';
|
||||||
* this defaulting logic would drift, and the custom path is the one an
|
* this defaulting logic would drift, and the custom path is the one an
|
||||||
* operator can get wrong.
|
* operator can get wrong.
|
||||||
*
|
*
|
||||||
* Each rule's `pattern` is compiled case-insensitively; rules lacking `id` or
|
* Each rule's `pattern` is compiled case-insensitively; rules lacking `id`,
|
||||||
* `pattern`, and rules whose pattern fails to compile, are dropped. The
|
* lacking a non-empty string `pattern`, or whose pattern fails to compile, are
|
||||||
* defaults below are loader tolerance, recorded in the artifact under
|
* dropped. The string check matters on its own: `new RegExp` does not throw on
|
||||||
* `missing-field-defaults` — not an optional-field contract.
|
* a non-string `pattern` (e.g. `{source: 'x'}`) — it coerces the value via
|
||||||
|
* ToString first, so a vendored or operator-supplied rule with a malformed
|
||||||
|
* (non-string) pattern would otherwise compile into a real, unintended regex
|
||||||
|
* instead of being caught by the try/catch below. The defaults below are
|
||||||
|
* loader tolerance, recorded in the artifact under `missing-field-defaults` —
|
||||||
|
* not an optional-field contract.
|
||||||
*
|
*
|
||||||
* @param {object} parsed
|
* @param {object} parsed
|
||||||
* @returns {Array<{id: string, family: string, severity: string, re: RegExp,
|
* @returns {Array<{id: string, family: string, severity: string, re: RegExp,
|
||||||
|
|
@ -63,7 +68,7 @@ import { loadArtifact } from './commons-loader.mjs';
|
||||||
export function compileRules(parsed) {
|
export function compileRules(parsed) {
|
||||||
const compiled = [];
|
const compiled = [];
|
||||||
for (const rule of parsed?.rules || []) {
|
for (const rule of parsed?.rules || []) {
|
||||||
if (!rule || !rule.id || !rule.pattern) continue;
|
if (!rule || !rule.id || typeof rule.pattern !== 'string' || !rule.pattern) continue;
|
||||||
let re;
|
let re;
|
||||||
try {
|
try {
|
||||||
re = new RegExp(rule.pattern, 'i');
|
re = new RegExp(rule.pattern, 'i');
|
||||||
|
|
|
||||||
|
|
@ -159,5 +159,21 @@ describe('malware-signatures (commons known-bad-identity ruleset)', () => {
|
||||||
it('returns an empty array for a ruleset with no rules array', () => {
|
it('returns an empty array for a ruleset with no rules array', () => {
|
||||||
assert.deepEqual(compileRules({}), []);
|
assert.deepEqual(compileRules({}), []);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('drops a rule whose pattern is not a string, instead of stringifying it', () => {
|
||||||
|
// `new RegExp(pattern, 'i')` does not throw when `pattern` is a non-string
|
||||||
|
// object: the RegExp constructor coerces it via ToString, so
|
||||||
|
// `{source: 'x'}` silently becomes the literal pattern "[object Object]"
|
||||||
|
// rather than raising the compile error the `try/catch` below is there to
|
||||||
|
// catch. A truthy-only guard (`!rule.pattern`) lets that coerced rule
|
||||||
|
// through as a real, matching RegExp instead of dropping it.
|
||||||
|
const compiled = compileRules({
|
||||||
|
rules: [
|
||||||
|
{ id: 'OBJ-PATTERN-001', pattern: { source: 'x' } },
|
||||||
|
{ id: 'A', pattern: 'a' },
|
||||||
|
],
|
||||||
|
});
|
||||||
|
assert.deepEqual(compiled.map((r) => r.id), ['A']);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -99,6 +99,43 @@ describe('signature-scanner: custom_rules_path (#36)', () => {
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('drops a custom rule whose pattern is not a string, instead of coercing it', async () => {
|
||||||
|
// Same coercion bug as tests/lib/malware-signatures.test.mjs, exercised
|
||||||
|
// through the operator-facing path: JSON.parse of a JSON object pattern
|
||||||
|
// yields a JS object, and `new RegExp(pattern, 'i')` does not throw on it —
|
||||||
|
// it stringifies via ToString to the literal "[object Object]" and
|
||||||
|
// compiles THAT as a regex. The leading/trailing brackets make it a
|
||||||
|
// character class, not a literal match: `[object Object]` matches any
|
||||||
|
// single occurrence of o/b/j/e/c/t/space, which is nearly every file. So
|
||||||
|
// the rule must be dropped by compileRules's type check, not left for the
|
||||||
|
// compile try/catch, which never sees an error here.
|
||||||
|
const dir = mkdtempSync(join(tmpdir(), 'sig-custom-'));
|
||||||
|
try {
|
||||||
|
writePolicy(dir, { sig: { custom_rules_path: 'custom-sigs.json' } });
|
||||||
|
writeFileSync(join(dir, 'custom-sigs.json'), JSON.stringify({
|
||||||
|
rules: [{
|
||||||
|
id: 'CUSTOM-OBJ-001',
|
||||||
|
family: 'webshell',
|
||||||
|
severity: 'high',
|
||||||
|
pattern: { source: 'EVILCUSTOMMARKER_[0-9]+' },
|
||||||
|
description: 'Malformed: pattern is an object, not a string',
|
||||||
|
}],
|
||||||
|
}));
|
||||||
|
|
||||||
|
resetCounter();
|
||||||
|
const discovery = await discoverFiles(dir);
|
||||||
|
const result = await scan(dir, discovery);
|
||||||
|
assert.equal(result.status, 'ok');
|
||||||
|
const bad = result.findings.find(f => f.evidence && f.evidence.includes('CUSTOM-OBJ-001'));
|
||||||
|
assert.equal(
|
||||||
|
bad, undefined,
|
||||||
|
'a rule with a non-string pattern must be dropped, not coerced into a matching regex',
|
||||||
|
);
|
||||||
|
} finally {
|
||||||
|
rmSync(dir, { recursive: true, force: true });
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
it('fails gracefully when the custom ruleset is invalid JSON', async () => {
|
it('fails gracefully when the custom ruleset is invalid JSON', async () => {
|
||||||
const dir = mkdtempSync(join(tmpdir(), 'sig-custom-'));
|
const dir = mkdtempSync(join(tmpdir(), 'sig-custom-'));
|
||||||
try {
|
try {
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue