diff --git a/examples/protect/demo-rules.json b/examples/protect/demo-rules.json index 9a466dfa..cfeec61c 100644 --- a/examples/protect/demo-rules.json +++ b/examples/protect/demo-rules.json @@ -119,7 +119,7 @@ "category": "pii-exposure", "action": "redact", "rule_v2": [ - { "parameter": "response.body", "match": { "type": "regex", "value": "/[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\\.[A-Za-z]{2,}/" } } + { "parameter": "response.body", "match": { "type": "regex", "value": "/[A-Za-z0-9._%+-]+@[A-Za-z0-9-]+(?:\\.[A-Za-z0-9-]+){0,8}\\.[A-Za-z]{2,}/" } } ], "_demo": { "desc": "An endpoint returns a user's email; the address is masked", diff --git a/rule-contract.json b/rule-contract.json index 1d6631c1..e136e48d 100644 --- a/rule-contract.json +++ b/rule-contract.json @@ -1,6 +1,6 @@ { "$comment": "Generated from src/protect/rules/contract.js by scripts/emit-rule-contract.mjs. Do not edit.", - "version": "2.9", + "version": "2.10", "sources": { "raw": { "keyed": false @@ -433,6 +433,8 @@ "maxRules": 5000, "maxWhitelists": 2000, "maxConditionsPerRule": 250, + "maxConditionNodesPerRule": 1000, + "maxConditionNodesPerBundle": 25000, "maxNestingDepth": 12, "maxRegexLength": 1000, "maxValueLength": 8192 diff --git a/src/protect/defaults.js b/src/protect/defaults.js index c78ac062..c94bb165 100644 --- a/src/protect/defaults.js +++ b/src/protect/defaults.js @@ -170,7 +170,7 @@ export const DEFAULT_RESPONSE_RULES = [ // // Accepts a real newline and a JSON-escaped one. Most traces reach a client inside a JSON error // body, where the newline is the two characters `\` and `n`. - rule_v2: [{ parameter: 'response.body', match: { type: 'regex', value: '/(?:\\n|\\\\n)\\s*at\\s+.+\\(.+:\\d+:\\d+\\)/' } }] + rule_v2: [{ parameter: 'response.body', match: { type: 'regex', value: '/(?:\\n|\\\\n)\\s*at\\s+\\S[^\\r\\n(]*\\((?:node:|[A-Za-z]:[\\\\/])?[^:\\r\\n]+:\\d+:\\d+\\)/' } }] }, { id: 'resp-sql-error', diff --git a/src/protect/engine/client.js b/src/protect/engine/client.js index 6c66ef19..7bfb810c 100644 --- a/src/protect/engine/client.js +++ b/src/protect/engine/client.js @@ -1,4 +1,5 @@ import { safeBaseUrl } from '../safe-origin.js'; +import { readBoundedJson } from './response-json.js'; const DEFAULT_BASE_URL = 'https://api.patchstack.com'; const DEFAULT_CACHE_TTL = 300_000; @@ -71,7 +72,7 @@ export class PatchstackRuleClient { }; } - const data = await response.json(); + const data = await readBoundedJson(response); // A 200 that isn't a genuine rule envelope must not be treated as "no rules". Report failure // so the caller falls back to the cache / bundled rules rather than caching an empty bundle. diff --git a/src/protect/engine/engine.js b/src/protect/engine/engine.js index be9d75b4..6b628045 100644 --- a/src/protect/engine/engine.js +++ b/src/protect/engine/engine.js @@ -18,6 +18,92 @@ const REDOS_PATTERNS = [ new RegExp('\\(' + GRP + '\\|' + GRP + '\\)\\s*[+*]') // nested alternation under a quantifier ]; +// Adjacent unbounded atoms can still make rejection polynomial even without a nested group (`a+b+c`). +// A literal delimiter resets the run only when the preceding atom cannot consume that delimiter. This +// keeps structured patterns expressible without letting a harmless prefix conceal a risky suffix. +function hasAdjacentUnbounded(body, flags) { + let previousUnboundedAtom = null; + for (let i = 0; i < body.length;) { + const start = i; + const ch = body[i]; + let assertion = false; + let literal = null; + let atom = ch; + + if (ch === '\\') { + const escaped = body[i + 1] ?? ''; + assertion = /[bBAZz]/.test(escaped); + if (!/[bBAZzdswDSWnrtvf0-9pPkKxuc]/.test(escaped)) literal = escaped; + i += Math.min(2, body.length - i); + atom = body.slice(start, i); + } else if (ch === '[') { + i++; + while (i < body.length) { + if (body[i] === '\\') i += 2; + else if (body[i++] === ']') break; + } + atom = body.slice(start, i); + } else if (ch === ')') { + i++; + atom = '.'; // Conservatively treat a quantified group as capable of consuming a delimiter. + } else if ('(|'.includes(ch)) { + previousUnboundedAtom = null; + i++; + continue; + } else if ('^$'.includes(ch)) { + i++; + continue; + } else if ('?*+{}'.includes(ch)) { + previousUnboundedAtom = null; + i++; + continue; + } else { + if (ch !== '.') literal = ch; + i++; + } + + if (assertion) continue; + + let unbounded = false; + if (body[i] === '*' || body[i] === '+') { + unbounded = true; + i++; + } else if (body[i] === '{') { + const quantifier = /^\{\d*,\}/.exec(body.slice(i)); + if (quantifier) { + unbounded = true; + i += quantifier[0].length; + } + } + if (unbounded && (body[i] === '?' || body[i] === '+')) i++; + + if (unbounded && previousUnboundedAtom !== null) return true; + if (unbounded) { + previousUnboundedAtom = atom; + } else if (previousUnboundedAtom !== null) { + try { + const atomFlags = flags.replace(/[^iu]/g, ''); + const complementaryClass = + (previousUnboundedAtom === '\\s' && atom === '\\S') || + (previousUnboundedAtom === '\\S' && atom === '\\s') || + (previousUnboundedAtom === '\\d' && atom === '\\D') || + (previousUnboundedAtom === '\\D' && atom === '\\d') || + (previousUnboundedAtom === '\\w' && atom === '\\W') || + (previousUnboundedAtom === '\\W' && atom === '\\w'); + const excludedLiteral = + literal !== null && !new RegExp(`^(?:${previousUnboundedAtom})$`, atomFlags).test(literal); + if (complementaryClass || excludedLiteral) { + previousUnboundedAtom = null; + } + } catch { + // If exclusion cannot be proved, retain the preceding atom as a conservative backstop. + } + } + if (i === start) i++; + } + return false; +} + // Report once when a rule's regex is rejected (ReDoS-shaped or unparseable). Unlike an unknown match // type, a rejected regex used to fail silently — so a delivered rule protected nothing and nobody knew. const warnedRejectedPatterns = new Set(); @@ -44,17 +130,16 @@ export function safeRegExp(pattern) { return null; } - for (const dangerous of REDOS_PATTERNS) { - if (dangerous.test(pattern)) { - return null; - } - } - const match = pattern.match(/^\/(.+?)\/([gimsuy]*)$/s); if (!match) { return null; } + for (const dangerous of REDOS_PATTERNS) { + if (dangerous.test(match[1])) return null; + } + if (hasAdjacentUnbounded(match[1], match[2])) return null; + try { return new RegExp(match[1], match[2]); } catch { diff --git a/src/protect/engine/pulse-client.js b/src/protect/engine/pulse-client.js index 36e97d68..219220cc 100644 --- a/src/protect/engine/pulse-client.js +++ b/src/protect/engine/pulse-client.js @@ -1,6 +1,7 @@ import { safeBaseUrl } from '../safe-origin.js'; import { pulseAuthHeader } from '../../pulse-token.js'; import { canonicalBuildId } from '../../build-id.js'; +import { readBoundedJson } from './response-json.js'; const DEFAULT_BASE_URL = 'https://api.patchstack.com/monitor/pulse'; const DEFAULT_CACHE_TTL = 300_000; @@ -123,7 +124,7 @@ export class PulseRuleClient { if (!response.ok) { return { success: false, error: `API returned ${response.status}`, firewall: [], whitelists: [], whitelist_keys: {} }; } - const data = await response.json(); + const data = await readBoundedJson(response); // A 200 that isn't a genuine rule envelope (schema drift, a proxy/interstitial page, an // {error} body) must not be treated as "no rules". Report failure so the caller falls back to // the cache / bundled rules rather than caching an empty bundle. diff --git a/src/protect/engine/response-json.js b/src/protect/engine/response-json.js new file mode 100644 index 00000000..543675cb --- /dev/null +++ b/src/protect/engine/response-json.js @@ -0,0 +1,47 @@ +export const MAX_RULE_RESPONSE_BYTES = 5 * 1024 * 1024; + +/** Parse a JSON response without allowing an endpoint to make the runtime buffer an unbounded body. */ +export async function readBoundedJson(response, maxBytes = MAX_RULE_RESPONSE_BYTES) { + const stated = Number(response.headers?.get?.('content-length')); + if (Number.isFinite(stated) && stated > maxBytes) { + throw new Error(`rule response exceeds ${maxBytes} bytes`); + } + + const stream = response.body; + if (!stream || typeof stream.getReader !== 'function') { + if (typeof response.text === 'function') { + const text = await response.text(); + if (new TextEncoder().encode(text).byteLength > maxBytes) { + throw new Error(`rule response exceeds ${maxBytes} bytes`); + } + return JSON.parse(text); + } + throw new Error('rule response body cannot be read with a size bound'); + } + + const reader = stream.getReader(); + const chunks = []; + let total = 0; + try { + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + total += value.byteLength; + if (total > maxBytes) { + void reader.cancel().catch(() => {}); + throw new Error(`rule response exceeds ${maxBytes} bytes`); + } + chunks.push(value); + } + } finally { + reader.releaseLock(); + } + + const bytes = new Uint8Array(total); + let offset = 0; + for (const chunk of chunks) { + bytes.set(chunk, offset); + offset += chunk.byteLength; + } + return JSON.parse(new TextDecoder().decode(bytes)); +} diff --git a/src/protect/rules/contract.js b/src/protect/rules/contract.js index ba440e5d..4a8579f1 100644 --- a/src/protect/rules/contract.js +++ b/src/protect/rules/contract.js @@ -12,7 +12,7 @@ // `rule-contract.json` is the published form. `tests/protect/rule-contract.test.ts` reads the engine's own // source and asserts these descriptions match what it implements. -export const CONTRACT_VERSION = '2.9'; +export const CONTRACT_VERSION = '2.10'; /** * Every parameter source, and what it accepts after the dot. @@ -482,6 +482,8 @@ export const LIMITS = Object.freeze({ maxRules: 5000, maxWhitelists: 2000, maxConditionsPerRule: 250, + maxConditionNodesPerRule: 1000, + maxConditionNodesPerBundle: 25_000, maxNestingDepth: 12, maxRegexLength: 1000, maxValueLength: 8192, diff --git a/src/protect/rules/validate.js b/src/protect/rules/validate.js index 460e7005..fb85a452 100644 --- a/src/protect/rules/validate.js +++ b/src/protect/rules/validate.js @@ -29,6 +29,7 @@ import { nullPropertyProblem, rulePropertyProblem, } from './contract.js'; +import { safeRegExp } from '../engine/engine.js'; export const LIMITS = CONTRACT_LIMITS; @@ -45,6 +46,7 @@ export function validateBundle(bundle, opts = {}) { const rejected = []; const inFirewall = Array.isArray(bundle?.firewall) ? bundle.firewall : []; const inWhitelists = Array.isArray(bundle?.whitelists) ? bundle.whitelists : []; + const bundleBudget = { count: 0 }; const firewall = []; for (const rule of inFirewall) { @@ -52,7 +54,7 @@ export function validateBundle(bundle, opts = {}) { rejected.push({ id: idOf(rule), reason: `bundle exceeds maxRules (${LIMITS.maxRules})` }); continue; } - const reason = enforceableRuleProblem(rule); + const reason = enforceableRuleProblem(rule, bundleBudget); if (reason) rejected.push({ id: idOf(rule), reason }); else firewall.push(rule); } @@ -77,7 +79,7 @@ export function validateBundle(bundle, opts = {}) { rejected.push({ id: idOf(wl), reason: `whitelist may not carry ${BUILD_SCOPE_PROPERTY}` }); continue; } - const reason = conditionsProblem(wl?.rule_v2); + const reason = conditionsProblem(wl?.rule_v2, 0, { count: 0 }, bundleBudget); if (reason) rejected.push({ id: idOf(wl), reason: `whitelist: ${reason}` }); else whitelists.push(wl); } @@ -102,7 +104,7 @@ function idOf(rule) { * * @returns {string|null} a reason the rule must be dropped, or null when it's acceptable. */ -export function enforceableRuleProblem(rule) { +export function enforceableRuleProblem(rule, bundleBudget = null) { if (!rule || typeof rule !== 'object') return 'not an object'; // Before anything reads a property: a property that is PRESENT and null is not an omission. Every layer @@ -129,10 +131,10 @@ export function enforceableRuleProblem(rule) { const propertyReason = rulePropertyProblem(rule); if (propertyReason) return propertyReason; - return conditionsProblem(rule.rule_v2); + return conditionsProblem(rule.rule_v2, 0, { count: 0 }, bundleBudget); } -function conditionsProblem(conditions, depth = 0) { +function conditionsProblem(conditions, depth = 0, ruleBudget = { count: 0 }, bundleBudget = null) { if (!Array.isArray(conditions)) return 'rule_v2 must be an array of conditions'; if (conditions.length === 0) return 'rule_v2 is empty (would never match)'; if (depth > LIMITS.maxNestingDepth) return `nesting deeper than ${LIMITS.maxNestingDepth}`; @@ -140,11 +142,21 @@ function conditionsProblem(conditions, depth = 0) { return `more than ${LIMITS.maxConditionsPerRule} conditions`; } for (const c of conditions) { + ruleBudget.count++; + if (ruleBudget.count > LIMITS.maxConditionNodesPerRule) { + return `more than ${LIMITS.maxConditionNodesPerRule} total condition nodes`; + } + if (bundleBudget) { + bundleBudget.count++; + if (bundleBudget.count > LIMITS.maxConditionNodesPerBundle) { + return `bundle exceeds ${LIMITS.maxConditionNodesPerBundle} total condition nodes`; + } + } const shapeReason = conditionShapeProblem(c); if (shapeReason) return shapeReason; if (isGroup(c)) { - const nested = conditionsProblem(c.rules, depth + 1); + const nested = conditionsProblem(c.rules, depth + 1, ruleBudget, bundleBudget); if (nested) return nested; continue; // a group carries no match of its own } @@ -164,6 +176,7 @@ function conditionsProblem(conditions, depth = 0) { if (m.type === 'regex') { if (typeof m.value !== 'string') return 'regex match.value must be a string'; if (m.value.length > LIMITS.maxRegexLength) return `regex longer than ${LIMITS.maxRegexLength} chars`; + if (safeRegExp(m.value) === null) return 'regex is invalid or has an unsafe repetition shape'; } else if (typeof m.value === 'string' && m.value.length > LIMITS.maxValueLength) { return `match.value longer than ${LIMITS.maxValueLength} chars`; } @@ -173,7 +186,12 @@ function conditionsProblem(conditions, depth = 0) { // The PARENT's parameter is carried down. The sub-match has none of its own — it applies to whatever // the key path navigated to — so validating it in isolation reported every one of them as a match // type missing a parameter, which would have rejected a shipped capability as malformed. - const nested = conditionsProblem([{ parameter: c.parameter, match: m.match }], depth + 1); + const nested = conditionsProblem( + [{ parameter: c.parameter, match: m.match }], + depth + 1, + ruleBudget, + bundleBudget, + ); if (nested) return nested; } } diff --git a/src/protect/templates/demo-rules.json b/src/protect/templates/demo-rules.json index 1ecd885f..5c8d5488 100644 --- a/src/protect/templates/demo-rules.json +++ b/src/protect/templates/demo-rules.json @@ -195,7 +195,7 @@ "parameter": "response.body", "match": { "type": "regex", - "value": "/[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\\.[A-Za-z]{2,}/" + "value": "/[A-Za-z0-9._%+-]+@[A-Za-z0-9-]+(?:\\.[A-Za-z0-9-]+){0,8}\\.[A-Za-z]{2,}/" } } ] diff --git a/tests/protect/middleware.test.ts b/tests/protect/middleware.test.ts index 6e5123cf..bf105c26 100644 --- a/tests/protect/middleware.test.ts +++ b/tests/protect/middleware.test.ts @@ -24,6 +24,13 @@ function restoreFetch() { } } +function rulesResponse() { + return new Response(JSON.stringify(fixtureRules), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); +} + function createReq(overrides = {}) { return { method: 'GET', @@ -175,10 +182,7 @@ describe('Middleware', () => { }); it('should fetch rules and create middleware', async () => { - mockFetch(async () => ({ - ok: true, - json: async () => fixtureRules - })); + mockFetch(async () => rulesResponse()); const mw = await protect({ token: 'test-token' }); @@ -188,10 +192,7 @@ describe('Middleware', () => { }); it('should block requests with fetched rules', async () => { - mockFetch(async () => ({ - ok: true, - json: async () => fixtureRules - })); + mockFetch(async () => rulesResponse()); const mw = await protect({ token: 'test-token', logging: false }); const req = createReq({ query: { search: '1 UNION SELECT *' } }); @@ -235,10 +236,7 @@ describe('Middleware', () => { }); it('should call onScan callback', async () => { - mockFetch(async () => ({ - ok: true, - json: async () => fixtureRules - })); + mockFetch(async () => rulesResponse()); let scanData = null; await protect({ @@ -260,10 +258,7 @@ describe('Middleware', () => { }); it('should lazy-initialize on first request', async () => { - mockFetch(async () => ({ - ok: true, - json: async () => fixtureRules - })); + mockFetch(async () => rulesResponse()); const mw = protectSync({ token: 'test-token' }); const req = createReq(); diff --git a/tests/protect/pulse-client.test.ts b/tests/protect/pulse-client.test.ts index dbdaf3ad..c17ef4a6 100644 --- a/tests/protect/pulse-client.test.ts +++ b/tests/protect/pulse-client.test.ts @@ -83,6 +83,16 @@ describe('PulseRuleClient', () => { expect(res.firewall).toEqual([]); }); + it('refuses a rule response whose declared size exceeds the buffer bound', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async () => new Response('{}', { status: 200, headers: { 'content-length': '6000000' } })), + ); + const res = await new PulseRuleClient({ siteUuid: 'x' }).getRules(); + expect(res.success).toBe(false); + expect(res.error).toMatch(/exceeds/); + }); + it('caches within the TTL (one fetch for two calls)', async () => { const fetchMock = vi.fn(async () => new Response(JSON.stringify(RULES), { status: 200 })); vi.stubGlobal('fetch', fetchMock); diff --git a/tests/protect/regex-cost.test.ts b/tests/protect/regex-cost.test.ts index 4739d7e3..09b58c54 100644 --- a/tests/protect/regex-cost.test.ts +++ b/tests/protect/regex-cost.test.ts @@ -14,9 +14,9 @@ import { * Regression coverage for the rules this package compiles in, and nothing wider. It does not screen a * rule delivered from the rules service, and does not stand between an authored rule and being served. * - * `safeRegExp()` refuses exponential shapes and accepts the polynomial one — two sibling quantified - * atoms separated by a literal both admit. The shape cannot be refused by reading a pattern, since a - * check strict enough to catch it refuses linear patterns too, so it is measured here instead. + * `safeRegExp()` refuses exponential shapes and adjacent unbounded atoms. The measurement remains a + * corpus gate for more subtle polynomial interactions that a structural check cannot classify without + * rejecting useful linear patterns too. * * Rejection, not matching: a match returns at its first success and pays none of the backtracking. * Every candidate is therefore built so the pattern cannot match it, and a candidate that does match @@ -27,7 +27,7 @@ import { const OVERLAPPING = "/\\bpostgres:\\/\\/[A-Za-z0-9:._-]+:[A-Za-z0-9:._-]+@/i"; const DISJOINT = "/\\bpostgres:\\/\\/[A-Za-z0-9._-]+:[A-Za-z0-9:._-]+@/i"; -/** Two sibling quantifiers and no group: accepted by `safeRegExp`, and does not finish at this size. */ +/** Two sibling quantifiers and no group: a reference shape that does not finish at this size. */ const UNBOUNDED = '/a+b+c/'; /** The default response screening cap. `max_bytes` raises it and `bypass_limit` removes it. */ diff --git a/tests/protect/rule-contract.test.ts b/tests/protect/rule-contract.test.ts index bc7c829e..80bd5d6c 100644 --- a/tests/protect/rule-contract.test.ts +++ b/tests/protect/rule-contract.test.ts @@ -118,7 +118,7 @@ describe('the build scope the contract publishes', () => { it('states the firewall-only, detect-only-on-unusable contract', () => { const contract = ruleContract(); - expect(CONTRACT_VERSION).toBe('2.9'); + expect(CONTRACT_VERSION).toBe('2.10'); expect(contract.build_scope).toEqual({ applies_to: ['firewall'], usable: { diff --git a/tests/protect/rule-validation.test.ts b/tests/protect/rule-validation.test.ts index b4c91d51..84e1cf94 100644 --- a/tests/protect/rule-validation.test.ts +++ b/tests/protect/rule-validation.test.ts @@ -52,6 +52,35 @@ describe('validateBundle', () => { expect(rejected[0].reason).toMatch(/maxRules/); }); + it('caps total condition nodes even when every nested array is individually small', () => { + const leaves = Array.from( + { length: LIMITS.maxConditionsPerRule }, + () => ({ parameter: 'raw', match: { type: 'contains', value: 'x' } }), + ); + const groups = Array.from( + { length: Math.ceil((LIMITS.maxConditionNodesPerRule + 1) / (leaves.length + 1)) }, + () => ({ parameter: 'rules', rules: leaves }), + ); + const result = validateBundle({ firewall: [ok({ rule_v2: groups })], whitelists: [] }); + expect(result.bundle.firewall).toHaveLength(0); + expect(result.rejected[0].reason).toMatch(/total condition nodes/); + }); + + it('caps condition nodes across the complete bundle', () => { + const leaves = Array.from( + { length: 249 }, + () => ({ parameter: 'raw', match: { type: 'contains', value: 'x' } }), + ); + const conditions = Array.from({ length: 4 }, () => ({ parameter: 'rules', rules: leaves })); + const count = Math.floor(LIMITS.maxConditionNodesPerBundle / 1000) + 1; + const result = validateBundle({ + firewall: Array.from({ length: count }, (_, id) => ok({ id: `r${id}`, rule_v2: conditions })), + whitelists: [], + }); + expect(result.bundle.firewall).toHaveLength(count - 1); + expect(result.rejected.at(-1)?.reason).toMatch(/bundle exceeds.*condition nodes/); + }); + it('validates whitelists too (a malformed one would suppress real rules)', () => { const { bundle, rejected } = validateBundle({ firewall: [], whitelists: [{ rule_id: 'r1', rule_v2: [] } as any] }); expect(bundle.whitelists).toHaveLength(0); @@ -77,6 +106,22 @@ describe('regex pattern length backstop', () => { expect(safeRegExp('/' + 'a'.repeat(2000) + '/')).toBeNull(); expect(safeRegExp('/AKIA[0-9A-Z]{16}/')).not.toBeNull(); }); + + it('refuses adjacent unbounded atoms before a delivered rule reaches evaluation', () => { + const { safeRegExp } = _testExports as any; + expect(safeRegExp('/a+b+c/')).toBeNull(); + expect(safeRegExp('/prefix-a+b+c/')).toBeNull(); + expect(safeRegExp('/[a:]+:[a]+/')).toBeNull(); + expect(safeRegExp('/[a]+:[a]+/')).not.toBeNull(); + expect(safeRegExp('/postgres:\\/\\/[A-Za-z0-9:._-]+:[A-Za-z0-9:._-]+@/i')).toBeNull(); + expect(safeRegExp('/postgres:\\/\\/[A-Za-z0-9._-]+:[A-Za-z0-9:._-]+@/i')).not.toBeNull(); + const result = validateBundle({ + firewall: [ok({ rule_v2: [{ parameter: 'raw', match: { type: 'regex', value: '/a+b+c/' } }] })], + whitelists: [], + }); + expect(result.bundle.firewall).toHaveLength(0); + expect(result.rejected[0].reason).toMatch(/unsafe repetition/); + }); }); describe('telemetry API origin', () => {