diff --git a/src/protect/engine/engine.js b/src/protect/engine/engine.js index a025330..f8f1b86 100644 --- a/src/protect/engine/engine.js +++ b/src/protect/engine/engine.js @@ -171,11 +171,58 @@ function warnUnsupportedMatchType(type) { ); } -// Internal / private / loopback / link-local / cloud-metadata host check, used by the -// `internal_host` match type for SSRF egress rules. It CANONICALIZES the host before classifying — +// Internal / private / loopback / link-local / cloud-metadata host check behind the `internal_host` +// match type. It CANONICALIZES the host before classifying — // a textual/prefix check is bypassable by alternate encodings (decimal/hex/octal IPv4, expanded or // IPv4-mapped IPv6), which is a classic SSRF evasion. Handles localhost / *.local / GCP metadata // names, every IPv4 spelling inet_aton accepts, and IPv6 loopback/link-local/unique-local/mapped. +/** + * The host to classify out of a rule parameter's value. + * + * `internal_host` was written for the egress phase, where the value IS the destination host. On the + * request phase the same question arrives as an application parameter, and there the value is almost + * always a full URL (`?url=http://169.254.169.254/latest/meta-data/`) or a `host:port` pair — neither of + * which is a hostname, so classifying the raw string answered "not internal" for every one of them. A + * request-phase SSRF rule was therefore expressible, servable and permanently inert: the exact failure + * this engine has been repeatedly hardened against, in the one match type meant to prevent it. + * + * Only the host is extracted; the classification itself is unchanged, so every canonicalization defence + * (decimal/hex IPv4, expanded and IPv4-mapped IPv6, trailing dots) still applies to what comes out. A + * value that is already a bare host passes through untouched, which is what keeps the egress path and + * the built-in default rule behaving exactly as before. + */ +function hostFromValue(value) { + const raw = String(value ?? '').trim(); + if (raw === '') return ''; + + // A scheme (`http://`, and deliberately any other) or a protocol-relative URL. Parsing rather than + // string-slicing is what makes the userinfo evasion (`http://trusted@169.254.169.254/`) resolve to the + // host actually contacted, and keeps `http://evil.com#@127.0.0.1` resolving to evil.com. + if (/^[a-z][a-z0-9+.-]*:\/\//i.test(raw) || raw.startsWith('//')) { + try { + return new URL(raw.startsWith('//') ? `http:${raw}` : raw).hostname; + } catch { + // Unparseable: hand the raw value on, where the host check rejects it rather than guessing. + return raw; + } + } + + // `[::1]:8080` — bracketed IPv6 with or without a port. + if (raw.startsWith('[')) { + const end = raw.indexOf(']'); + if (end > 0) return raw.slice(1, end); + } + + // `169.254.169.254:80`. Only a single colon followed by digits: a bare IPv6 address has several, and + // must not have its last group mistaken for a port. + const colon = raw.indexOf(':'); + if (colon > 0 && raw.indexOf(':', colon + 1) === -1 && /^\d+$/.test(raw.slice(colon + 1))) { + return raw.slice(0, colon); + } + + return raw; +} + function isInternalHost(hostname) { if (!hostname) return false; let host = String(hostname).toLowerCase().replace(/^\[|\]$/g, ''); @@ -282,10 +329,28 @@ function expandIPv6(host) { return out; } +// Report a `when` block that names nothing this engine understands. Fail-open is correct for a scope that +// cannot be EVALUATED, but a scope that cannot be UNDERSTOOD is an authoring mistake with the opposite +// consequence: the rule silently applies to every request instead of one route, which for a blocking rule +// is a false-positive surface across the whole app. Warned once so it is discoverable in a log. +const warnedScopes = new Set(); +function warnUnrecognisedScope(when) { + const key = Object.keys(when).sort().join(','); + if (warnedScopes.has(key)) return; + warnedScopes.add(key); + console.warn( + `[patchstack] Rule scope \`when: { ${key} }\` names no supported key — the engine understands ` + + `\`method\` and \`path\`. The scope is IGNORED and the rule applies to every request.` + ); +} + // Route/method scope for a rule's optional `when: { method, path }`. Fail-open: if the scope can't // be evaluated, the rule still applies (never silently suppress a rule). function ruleAppliesTo(when, resolver) { try { + if (when.method === undefined && when.path === undefined && Object.keys(when).length > 0) { + warnUnrecognisedScope(when); + } if (when.method) { const methods = (Array.isArray(when.method) ? when.method : [when.method]).map((m) => String(m).toUpperCase()); const actual = String(resolver.resolve('server.REQUEST_METHOD')[0] ?? 'GET').toUpperCase(); @@ -502,8 +567,9 @@ export function matchValue(type, value, matchVal, matchObj) { } case 'internal_host': - // SSRF egress: private / loopback / link-local / cloud-metadata destinations. - return isInternalHost(strValue); + // SSRF: private / loopback / link-local / cloud-metadata destinations. The value may be a bare + // host (egress) or a URL / host:port in an application parameter (request) — see `hostFromValue`. + return isInternalHost(hostFromValue(strValue)); case 'quotes': // engine-php exposes `inline_js_xss` as an alias of `quotes`. diff --git a/tests/protect/engine.test.ts b/tests/protect/engine.test.ts index c416804..942d155 100644 --- a/tests/protect/engine.test.ts +++ b/tests/protect/engine.test.ts @@ -123,6 +123,52 @@ describe('RuleEngine', () => { } }); + it('should match internal_host when the value is a URL, not a bare host', () => { + // The gap this closes: `internal_host` was written for the egress phase, where the value IS the + // destination host. On the request phase the same question arrives as an application parameter and + // the value is a full URL — so a served, correctly-pinned SSRF rule matched nothing at all. + for (const value of [ + 'http://169.254.169.254/latest/meta-data/', + 'http://localhost:3000/admin', + 'https://127.0.0.1/x', + 'http://[::1]/x', + 'http://metadata.google.internal/computeMetadata/v1/', + '//10.0.0.5/x', + '169.254.169.254:80', + '[::1]:8080', + ]) { + assert.strictEqual(matchValue('internal_host', value, null), true, `${value} should be internal`); + } + + for (const value of [ + 'https://api.stripe.example/v1/charges', + 'http://8.8.8.8/resolve', + 'how to use localhost in docker', + 'https://example.com/?next=/admin', + ]) { + assert.strictEqual(matchValue('internal_host', value, null), false, `${value} should be external`); + } + }); + + it('should read the host a URL actually contacts, not the one it advertises', () => { + // Why the host is parsed rather than sliced out of the string. Userinfo puts a trusted-looking name + // before the real host, and a fragment puts one after it; a substring check reads the wrong one in + // both directions, which is a bypass in the first case and a false positive in the second. + assert.strictEqual(matchValue('internal_host', 'http://api.stripe.example@169.254.169.254/', null), true); + assert.strictEqual(matchValue('internal_host', 'http://evil.example/#@127.0.0.1', null), false); + assert.strictEqual(matchValue('internal_host', 'http://evil.example/?next=http://127.0.0.1/', null), false); + }); + + it('should leave a bare host classified exactly as before', () => { + // The egress path and the built-in default rule pass `egress.host`, which is already a hostname. + // Extraction must be a no-op for those, or this change would alter what a live guard blocks today. + assert.strictEqual(matchValue('internal_host', '169.254.169.254', null), true); + assert.strictEqual(matchValue('internal_host', '::1', null), true); + assert.strictEqual(matchValue('internal_host', '2130706433', null), true); // decimal 127.0.0.1 + assert.strictEqual(matchValue('internal_host', 'example.com', null), false); + assert.strictEqual(matchValue('internal_host', '', null), false); + }); + it('should match quotes (and the inline_js_xss alias)', () => { assert.strictEqual(matchValue('quotes', `x' OR 1=1`, null), true); assert.strictEqual(matchValue('quotes', 'no quotes here', null), false); diff --git a/tests/protect/fixtures/generated-pinned-rule.json b/tests/protect/fixtures/generated-pinned-rule.json new file mode 100644 index 0000000..02f6359 --- /dev/null +++ b/tests/protect/fixtures/generated-pinned-rule.json @@ -0,0 +1,44 @@ +{ + "note": "Captured from a platform `GET pulse/rules/{uuid}` response for an app whose attack-surface map proved request input reaching an HTTP client. This is the SHAPE a coordinate-pinned rule arrives in — the parameter and route are the app's own, bound from a template at generation time. No advisory identifier: which advisories are shielded is not a public list.", + "shape": { + "routeScopedInRuleV2": "The platform scopes the route with an ANDed `server.REQUEST_URI contains` condition rather than a `when` block. Both forms are exercised by the suite, because the scope moving into `when` must not silently drop the scope.", + "enforcement": "A generated rule arrives dry-run regardless of the site's mode, and only promotion flips it. The per-rule value overriding a site-wide `block` is the safety property that keeps an unproven pinned rule from blocking traffic.", + "boundPlaceholders": "`` and `` are substituted before serving. A rule that reached an app with them intact would load and never match — `` is not a parameter source — which is indistinguishable from protection until someone fires an exploit at it." + }, + "served": { + "id": "pulse-1", + "title": "Block internal-host URLs reaching the vulnerable HTTP client", + "rule_v2": [ + { + "match": { "type": "contains", "value": "/api/preview" }, + "inclusive": true, + "mutations": ["urldecode"], + "parameter": "server.REQUEST_URI" + }, + { + "match": { "type": "internal_host" }, + "inclusive": true, + "mutations": ["urldecode"], + "parameter": "get.url" + } + ], + "enforcement": "dry-run" + }, + "template": { + "note": "The same rule before binding, as authored. Kept beside the served copy so the substitution is visible rather than described, and so a test can prove the unbound form is inert.", + "rule_v2": [ + { + "match": { "type": "contains", "value": "" }, + "inclusive": true, + "mutations": ["urldecode"], + "parameter": "server.REQUEST_URI" + }, + { + "match": { "type": "internal_host" }, + "inclusive": true, + "mutations": ["urldecode"], + "parameter": "" + } + ] + } +} diff --git a/tests/protect/generated-rule-chain.test.ts b/tests/protect/generated-rule-chain.test.ts new file mode 100644 index 0000000..f92075d --- /dev/null +++ b/tests/protect/generated-rule-chain.test.ts @@ -0,0 +1,211 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { createProtection } from '../../src/protect/runtime.js'; + +/** + * The COORDINATE-PINNED rule chain: a rule generated from an app's own attack-surface map, served by + * Pulse, and enforced through the HTTP guard. + * + * Why this exists as its own file. The defect it guards against was not a broken matcher — it was a + * composition: a rule bound to the right parameter, scoped to the right route, carrying the right + * per-rule enforcement, arriving at the runtime intact, and never firing. Every part was individually + * correct and every unit test passed. `pulse-chain.test.ts` covers the same transport with a STATIC + * lodash rule whose conditions read `raw`, so it cannot see a failure in the pinned shape: different + * parameter sources, a route scope, and a match type that was only ever exercised on the egress path. + * + * What broke, concretely: `internal_host` classified its value as a hostname, which is what the egress + * phase hands it. In an application parameter the value is a full URL, so the rule matched nothing for + * every request-phase SSRF rule the platform could generate. It was found by firing an exploit at a + * served rule, not by a test — hence this file. + * + * The four assertions are the ones that distinguish "protecting" from "present": + * detected in dry-run · 403 once promoted · external URL allowed · internal URL on another route allowed. + */ +const FIXTURE = JSON.parse( + readFileSync(join(dirname(fileURLToPath(import.meta.url)), 'fixtures', 'generated-pinned-rule.json'), 'utf8'), +); + +/** The rule exactly as the platform serves it — route scope inside `rule_v2`, enforcement `dry-run`. */ +const servedRule = FIXTURE.served; + +/** + * The same coverage with the scope expressed as `when`, the other form the engine supports. + * + * The key is `path`, not `route` — worth stating, because writing `route` here is not an error: the scope + * is simply ignored and the rule applies to every request. That is how the first draft of this test + * passed on the exploit and then also detected on a route it was scoped away from. + */ +const whenScopedRule = { + id: 'pulse-1-when', + title: servedRule.title, + when: { path: '/api/preview' }, + enforcement: 'dry-run', + rule_v2: [servedRule.rule_v2[1]], +}; + +/** A scope nobody can honour: the key is not one the engine knows, so the rule is unscoped. */ +const misspelledScopeRule = { + id: 'pulse-1-misspelled-scope', + title: servedRule.title, + when: { route: '/api/preview' }, + enforcement: 'dry-run', + rule_v2: [servedRule.rule_v2[1]], +}; + +/** The template before binding. Must be inert: `` is not a parameter source. */ +const unboundTemplate = { id: 'pulse-1-unbound', title: servedRule.title, rule_v2: FIXTURE.template.rule_v2 }; + +/** + * A mock Pulse endpoint whose per-rule enforcement can be flipped, which is how promotion reaches a + * running guard. The site stays in `block` throughout: the point is that a generated rule's own + * `dry-run` overrides it until promotion, so a site-wide mode change cannot promote a rule by accident. + */ +function mockPulse(rule: Record) { + const state = { enforcement: 'dry-run', etag: '"v1"' }; + const fetchMock = vi.fn(async (_url: string, init?: RequestInit) => { + const inm = (init?.headers as Record | undefined)?.['If-None-Match']; + if (inm === state.etag) return new Response(null, { status: 304, headers: { ETag: state.etag } }); + return new Response( + JSON.stringify({ + firewall: [{ ...rule, enforcement: state.enforcement }], + whitelists: [], + whitelist_keys: {}, + enforcement: 'block', + }), + { status: 200, headers: { 'Content-Type': 'application/json', ETag: state.etag } }, + ); + }); + return { state, fetchMock }; +} + +const req = (url: string) => new Request(url, { method: 'GET' }); +const appHandler = async () => new Response(JSON.stringify({ ok: true }), { status: 200 }); + +/** The exploit: an internal metadata address in the parameter the map proved reaches the HTTP client. */ +const SSRF = 'https://app.demo/api/preview?url=http://169.254.169.254/latest/meta-data/'; +/** The same route, a legitimate third-party destination. */ +const THIRD_PARTY = 'https://app.demo/api/preview?url=https://api.stripe.example/v1/charges'; +/** An internal destination on a route this rule is NOT scoped to. */ +const OTHER_ROUTE = 'https://app.demo/api/orders?url=http://169.254.169.254/'; + +describe('generated coordinate-pinned rule, through Pulse and the HTTP guard', () => { + const prevMode = process.env.PATCHSTACK_MODE; + afterEach(() => { + if (prevMode === undefined) delete process.env.PATCHSTACK_MODE; + else process.env.PATCHSTACK_MODE = prevMode; + vi.restoreAllMocks(); + }); + + it.each([ + ['as the platform serves it (route scoped in rule_v2)', servedRule], + ['with the scope expressed as when.path', whenScopedRule], + ])('detects in dry-run and blocks once promoted — %s', async (_label, rule) => { + delete process.env.PATCHSTACK_MODE; + const { state, fetchMock } = mockPulse(rule as Record); + vi.stubGlobal('fetch', fetchMock); + const detections: Array<{ rule?: { id?: string } }> = []; + + const p = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + onDetect: (d: { rule?: { id?: string } }) => detections.push(d), + }); + + // The site is in block mode, and the rule is not. A generated rule that inherited the site's mode + // would start blocking traffic on evidence that has not been corroborated against the running build. + expect(p.mode).toBe('block'); + + // 1. DETECTED, not blocked. This is the assertion the whole file exists for: the composition fires. + const dry = await p.fetch(appHandler)(req(SSRF)); + expect(dry.status).toBe(200); + expect(detections.some((d) => d.rule?.id === (rule as { id: string }).id), 'the pinned rule must fire').toBe(true); + + // 2. A legitimate destination on the same route is untouched — the rule screens the DESTINATION, and a + // rule that blocked this would be withdrawn before it ever reached a customer. + expect((await p.fetch(appHandler)(req(THIRD_PARTY))).status).toBe(200); + expect(detections.length, 'a third-party destination must not detect').toBe(1); + + // 3. The route scope holds: same exploit, different route, no detection at all. + expect((await p.fetch(appHandler)(req(OTHER_ROUTE))).status).toBe(200); + expect(detections.length, 'the route scope must exclude other endpoints').toBe(1); + + // 4. Promotion — the platform flips this rule's own enforcement, the guard picks it up on refresh. + state.enforcement = 'block'; + state.etag = '"v2"'; + await p.refresh(); + + expect((await p.fetch(appHandler)(req(SSRF))).status).toBe(403); + // Still no false positive after promotion, which is the state that actually reaches traffic. + expect((await p.fetch(appHandler)(req(THIRD_PARTY))).status).toBe(200); + expect((await p.fetch(appHandler)(req(OTHER_ROUTE))).status).toBe(200); + + p.stopRefresh?.(); + }); + + it('warns, and applies everywhere, when a scope names no key the engine knows', async () => { + // Not hypothetical: this is the shape the first draft of this test used. `when: { route }` is silently + // unscoped — the rule then applies to every request, which for a promoted rule is a false-positive + // surface across the whole app rather than one endpoint. Fail-open is right for a scope that cannot be + // EVALUATED; a scope that cannot be UNDERSTOOD is an authoring mistake, so the engine now says so once. + delete process.env.PATCHSTACK_MODE; + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const { fetchMock } = mockPulse(misspelledScopeRule); + vi.stubGlobal('fetch', fetchMock); + const detections: unknown[] = []; + + const p = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + onDetect: (d: unknown) => detections.push(d), + }); + + // The route it was meant to be scoped to, and one it was not: both detect. + await p.fetch(appHandler)(req(SSRF)); + await p.fetch(appHandler)(req(OTHER_ROUTE)); + expect(detections.length, 'an unrecognised scope key leaves the rule unscoped').toBe(2); + + expect(warn.mock.calls.flat().join(' ')).toMatch(/scope|when/i); + + p.stopRefresh?.(); + }); + + it('is inert if the template reaches the app with its placeholders unbound', async () => { + // The other half of the same failure. `` is not a parameter source, so an unbound template + // loads, reports as a shipped rule, and can never match — and the only way to tell it apart from a + // working rule is to fire an exploit at it. Asserted so that "generation bound the coordinates" is a + // property of the chain rather than an assumption about it. + delete process.env.PATCHSTACK_MODE; + const { state, fetchMock } = mockPulse(unboundTemplate); + vi.stubGlobal('fetch', fetchMock); + const detections: unknown[] = []; + + const p = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + onDetect: (d: unknown) => detections.push(d), + }); + + state.enforcement = 'block'; + state.etag = '"v2"'; + await p.refresh(); + + expect((await p.fetch(appHandler)(req(SSRF))).status).toBe(200); + expect(detections.length, 'an unbound template cannot match anything').toBe(0); + + p.stopRefresh?.(); + }); + + it('binds the fixture from a real serve, with no placeholder left in it', () => { + // Guards the fixture itself: if someone regenerates it from a template rather than from a served + // response, every assertion above would still pass while testing the wrong shape. + const json = JSON.stringify(servedRule); + expect(json).not.toMatch(/|/); + expect(json).toContain('get.url'); + expect(json).toContain('/api/preview'); + expect(servedRule.enforcement).toBe('dry-run'); + // And the template half must still carry them, or the inertness test above proves nothing. + expect(JSON.stringify(FIXTURE.template)).toMatch(//); + }); +});