Skip to content

Commit 4e8b466

Browse files
authored
Merge branch 'main' into mariot/eng-3568-login-agent-friendly
2 parents c77050c + ee23d81 commit 4e8b466

4 files changed

Lines changed: 371 additions & 4 deletions

File tree

‎src/protect/engine/engine.js‎

Lines changed: 70 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -171,11 +171,58 @@ function warnUnsupportedMatchType(type) {
171171
);
172172
}
173173

174-
// Internal / private / loopback / link-local / cloud-metadata host check, used by the
175-
// `internal_host` match type for SSRF egress rules. It CANONICALIZES the host before classifying —
174+
// Internal / private / loopback / link-local / cloud-metadata host check behind the `internal_host`
175+
// match type. It CANONICALIZES the host before classifying —
176176
// a textual/prefix check is bypassable by alternate encodings (decimal/hex/octal IPv4, expanded or
177177
// IPv4-mapped IPv6), which is a classic SSRF evasion. Handles localhost / *.local / GCP metadata
178178
// names, every IPv4 spelling inet_aton accepts, and IPv6 loopback/link-local/unique-local/mapped.
179+
/**
180+
* The host to classify out of a rule parameter's value.
181+
*
182+
* `internal_host` was written for the egress phase, where the value IS the destination host. On the
183+
* request phase the same question arrives as an application parameter, and there the value is almost
184+
* always a full URL (`?url=http://169.254.169.254/latest/meta-data/`) or a `host:port` pair — neither of
185+
* which is a hostname, so classifying the raw string answered "not internal" for every one of them. A
186+
* request-phase SSRF rule was therefore expressible, servable and permanently inert: the exact failure
187+
* this engine has been repeatedly hardened against, in the one match type meant to prevent it.
188+
*
189+
* Only the host is extracted; the classification itself is unchanged, so every canonicalization defence
190+
* (decimal/hex IPv4, expanded and IPv4-mapped IPv6, trailing dots) still applies to what comes out. A
191+
* value that is already a bare host passes through untouched, which is what keeps the egress path and
192+
* the built-in default rule behaving exactly as before.
193+
*/
194+
function hostFromValue(value) {
195+
const raw = String(value ?? '').trim();
196+
if (raw === '') return '';
197+
198+
// A scheme (`http://`, and deliberately any other) or a protocol-relative URL. Parsing rather than
199+
// string-slicing is what makes the userinfo evasion (`http://trusted@169.254.169.254/`) resolve to the
200+
// host actually contacted, and keeps `http://evil.com#@127.0.0.1` resolving to evil.com.
201+
if (/^[a-z][a-z0-9+.-]*:\/\//i.test(raw) || raw.startsWith('//')) {
202+
try {
203+
return new URL(raw.startsWith('//') ? `http:${raw}` : raw).hostname;
204+
} catch {
205+
// Unparseable: hand the raw value on, where the host check rejects it rather than guessing.
206+
return raw;
207+
}
208+
}
209+
210+
// `[::1]:8080` — bracketed IPv6 with or without a port.
211+
if (raw.startsWith('[')) {
212+
const end = raw.indexOf(']');
213+
if (end > 0) return raw.slice(1, end);
214+
}
215+
216+
// `169.254.169.254:80`. Only a single colon followed by digits: a bare IPv6 address has several, and
217+
// must not have its last group mistaken for a port.
218+
const colon = raw.indexOf(':');
219+
if (colon > 0 && raw.indexOf(':', colon + 1) === -1 && /^\d+$/.test(raw.slice(colon + 1))) {
220+
return raw.slice(0, colon);
221+
}
222+
223+
return raw;
224+
}
225+
179226
function isInternalHost(hostname) {
180227
if (!hostname) return false;
181228
let host = String(hostname).toLowerCase().replace(/^\[|\]$/g, '');
@@ -282,10 +329,28 @@ function expandIPv6(host) {
282329
return out;
283330
}
284331

332+
// Report a `when` block that names nothing this engine understands. Fail-open is correct for a scope that
333+
// cannot be EVALUATED, but a scope that cannot be UNDERSTOOD is an authoring mistake with the opposite
334+
// consequence: the rule silently applies to every request instead of one route, which for a blocking rule
335+
// is a false-positive surface across the whole app. Warned once so it is discoverable in a log.
336+
const warnedScopes = new Set();
337+
function warnUnrecognisedScope(when) {
338+
const key = Object.keys(when).sort().join(',');
339+
if (warnedScopes.has(key)) return;
340+
warnedScopes.add(key);
341+
console.warn(
342+
`[patchstack] Rule scope \`when: { ${key} }\` names no supported key — the engine understands ` +
343+
`\`method\` and \`path\`. The scope is IGNORED and the rule applies to every request.`
344+
);
345+
}
346+
285347
// Route/method scope for a rule's optional `when: { method, path }`. Fail-open: if the scope can't
286348
// be evaluated, the rule still applies (never silently suppress a rule).
287349
function ruleAppliesTo(when, resolver) {
288350
try {
351+
if (when.method === undefined && when.path === undefined && Object.keys(when).length > 0) {
352+
warnUnrecognisedScope(when);
353+
}
289354
if (when.method) {
290355
const methods = (Array.isArray(when.method) ? when.method : [when.method]).map((m) => String(m).toUpperCase());
291356
const actual = String(resolver.resolve('server.REQUEST_METHOD')[0] ?? 'GET').toUpperCase();
@@ -502,8 +567,9 @@ export function matchValue(type, value, matchVal, matchObj) {
502567
}
503568

504569
case 'internal_host':
505-
// SSRF egress: private / loopback / link-local / cloud-metadata destinations.
506-
return isInternalHost(strValue);
570+
// SSRF: private / loopback / link-local / cloud-metadata destinations. The value may be a bare
571+
// host (egress) or a URL / host:port in an application parameter (request) — see `hostFromValue`.
572+
return isInternalHost(hostFromValue(strValue));
507573

508574
case 'quotes':
509575
// engine-php exposes `inline_js_xss` as an alias of `quotes`.

‎tests/protect/engine.test.ts‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,52 @@ describe('RuleEngine', () => {
123123
}
124124
});
125125

126+
it('should match internal_host when the value is a URL, not a bare host', () => {
127+
// The gap this closes: `internal_host` was written for the egress phase, where the value IS the
128+
// destination host. On the request phase the same question arrives as an application parameter and
129+
// the value is a full URL — so a served, correctly-pinned SSRF rule matched nothing at all.
130+
for (const value of [
131+
'http://169.254.169.254/latest/meta-data/',
132+
'http://localhost:3000/admin',
133+
'https://127.0.0.1/x',
134+
'http://[::1]/x',
135+
'http://metadata.google.internal/computeMetadata/v1/',
136+
'//10.0.0.5/x',
137+
'169.254.169.254:80',
138+
'[::1]:8080',
139+
]) {
140+
assert.strictEqual(matchValue('internal_host', value, null), true, `${value} should be internal`);
141+
}
142+
143+
for (const value of [
144+
'https://api.stripe.example/v1/charges',
145+
'http://8.8.8.8/resolve',
146+
'how to use localhost in docker',
147+
'https://example.com/?next=/admin',
148+
]) {
149+
assert.strictEqual(matchValue('internal_host', value, null), false, `${value} should be external`);
150+
}
151+
});
152+
153+
it('should read the host a URL actually contacts, not the one it advertises', () => {
154+
// Why the host is parsed rather than sliced out of the string. Userinfo puts a trusted-looking name
155+
// before the real host, and a fragment puts one after it; a substring check reads the wrong one in
156+
// both directions, which is a bypass in the first case and a false positive in the second.
157+
assert.strictEqual(matchValue('internal_host', 'http://api.stripe.example@169.254.169.254/', null), true);
158+
assert.strictEqual(matchValue('internal_host', 'http://evil.example/#@127.0.0.1', null), false);
159+
assert.strictEqual(matchValue('internal_host', 'http://evil.example/?next=http://127.0.0.1/', null), false);
160+
});
161+
162+
it('should leave a bare host classified exactly as before', () => {
163+
// The egress path and the built-in default rule pass `egress.host`, which is already a hostname.
164+
// Extraction must be a no-op for those, or this change would alter what a live guard blocks today.
165+
assert.strictEqual(matchValue('internal_host', '169.254.169.254', null), true);
166+
assert.strictEqual(matchValue('internal_host', '::1', null), true);
167+
assert.strictEqual(matchValue('internal_host', '2130706433', null), true); // decimal 127.0.0.1
168+
assert.strictEqual(matchValue('internal_host', 'example.com', null), false);
169+
assert.strictEqual(matchValue('internal_host', '', null), false);
170+
});
171+
126172
it('should match quotes (and the inline_js_xss alias)', () => {
127173
assert.strictEqual(matchValue('quotes', `x' OR 1=1`, null), true);
128174
assert.strictEqual(matchValue('quotes', 'no quotes here', null), false);
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
{
2+
"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.",
3+
"shape": {
4+
"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.",
5+
"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.",
6+
"boundPlaceholders": "`<param>` and `<route>` are substituted before serving. A rule that reached an app with them intact would load and never match — `<param>` is not a parameter source — which is indistinguishable from protection until someone fires an exploit at it."
7+
},
8+
"served": {
9+
"id": "pulse-1",
10+
"title": "Block internal-host URLs reaching the vulnerable HTTP client",
11+
"rule_v2": [
12+
{
13+
"match": { "type": "contains", "value": "/api/preview" },
14+
"inclusive": true,
15+
"mutations": ["urldecode"],
16+
"parameter": "server.REQUEST_URI"
17+
},
18+
{
19+
"match": { "type": "internal_host" },
20+
"inclusive": true,
21+
"mutations": ["urldecode"],
22+
"parameter": "get.url"
23+
}
24+
],
25+
"enforcement": "dry-run"
26+
},
27+
"template": {
28+
"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.",
29+
"rule_v2": [
30+
{
31+
"match": { "type": "contains", "value": "<route>" },
32+
"inclusive": true,
33+
"mutations": ["urldecode"],
34+
"parameter": "server.REQUEST_URI"
35+
},
36+
{
37+
"match": { "type": "internal_host" },
38+
"inclusive": true,
39+
"mutations": ["urldecode"],
40+
"parameter": "<param>"
41+
}
42+
]
43+
}
44+
}

0 commit comments

Comments
 (0)