fix(security): strip control characters before scanning CSS for url()
Review follow-up on the sanitizer dedup. sanitizeCSS already carried the rule — "any pass that can join tokens has to happen before validation, not after" — written above the URL scan to explain why it runs after the HTML-comment strip. The control-character strip is exactly such a pass and sat eleven lines below it. So `u<CTRL>rl(https://tracker.example/p.gif)` was scanned as clean, and the strip below then joined it into a live remote request with no warning. Newlines are control characters here too, so `u\nrl(...)` did it without an exotic byte. Verified against the real function before and after: all five variants returned a live remote url() and now return `none` plus the blocked-URL warning. This PR is what exposed it. Dropping newsletterService's second stripRemoteCssUrls pass was right — the duplicate hid a defect in the shared sanitizer rather than fixing it — but it removed the belt that was catching this for the newsletter path. Fixing the ordering fixes it for every caller instead of restoring the second pass. Refs #1264
This commit is contained in:
@@ -221,6 +221,24 @@ describe('sanitizeCSS', () => {
|
|||||||
expect(asParsed(sanitized)).not.toContain('evil.example');
|
expect(asParsed(sanitized)).not.toContain('evil.example');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it.each([
|
||||||
|
['a C0 control character', '\u0001'],
|
||||||
|
['a NUL byte', '\u0000'],
|
||||||
|
['a DEL byte', '\u007F'],
|
||||||
|
// Newlines are control characters for this strip, so the bypass did not
|
||||||
|
// need an exotic byte — ordinary-looking wrapped CSS was enough.
|
||||||
|
['a newline', '\n'],
|
||||||
|
])('blocks a url() that only becomes one after %s is removed', (_label, ch) => {
|
||||||
|
// Same token-joining hazard as the HTML-comment case above: the control
|
||||||
|
// strip used to run AFTER the URL scan, so `u\u0001rl(...)` was scanned
|
||||||
|
// as clean and then joined into a live remote request, with no warning.
|
||||||
|
const { sanitized, warnings } = sanitizeCSS(
|
||||||
|
`.a{background:u${ch}rl(https://evil.example/p.gif)}`
|
||||||
|
);
|
||||||
|
expect(asParsed(sanitized)).not.toContain('evil.example');
|
||||||
|
expect(warnings.join(' ')).toContain('external URL');
|
||||||
|
});
|
||||||
|
|
||||||
it('still blocks the other forbidden patterns', () => {
|
it('still blocks the other forbidden patterns', () => {
|
||||||
const { sanitized } = sanitizeCSS(
|
const { sanitized } = sanitizeCSS(
|
||||||
'@import url("https://x/e.css"); .a{width:expression(alert(1));behavior:url(e.htc)}'
|
'@import url("https://x/e.css"); .a{width:expression(alert(1));behavior:url(e.htc)}'
|
||||||
|
|||||||
@@ -291,12 +291,22 @@ function sanitizeCSS(cssContent) {
|
|||||||
// Remove HTML comments that might be used for injection
|
// Remove HTML comments that might be used for injection
|
||||||
sanitized = sanitized.replace(/<!--[\s\S]*?-->/g, '');
|
sanitized = sanitized.replace(/<!--[\s\S]*?-->/g, '');
|
||||||
|
|
||||||
// URLs are scanned AFTER the comment strip, not before. Removing
|
// Remove control characters BEFORE the URL scan. This is the same
|
||||||
// `<!--x-->` from `u<!--x-->rl(https://evil.example/p.gif)` JOINS the
|
// token-joining hazard as the HTML-comment strip above: dropping the
|
||||||
// remaining characters into a live `url(...)` — so a scan that ran first
|
// \u0001 from `u\u0001rl(https://evil.example/p.gif)` joins the remainder
|
||||||
// saw no token, reported the input clean, and the transformation below it
|
// into a live `url(...)`, so a scan that ran first saw no token and
|
||||||
// then produced exactly the request the scan was there to prevent. Any
|
// reported the input clean. Newlines are control characters too, which
|
||||||
// pass that can join tokens has to happen before validation, not after.
|
// made `u\nrl(...)` the same bypass in ordinary-looking CSS.
|
||||||
|
// eslint-disable-next-line no-control-regex -- intentional: strips control chars from untrusted CSS
|
||||||
|
sanitized = sanitized.replace(/[\u0000-\u001F\u007F]/g, '');
|
||||||
|
|
||||||
|
// URLs are scanned AFTER the comment and control-character strips, not
|
||||||
|
// before. Removing `<!--x-->` from `u<!--x-->rl(https://evil.example/p.gif)`
|
||||||
|
// JOINS the remaining characters into a live `url(...)` — so a scan that
|
||||||
|
// ran first saw no token, reported the input clean, and the transformation
|
||||||
|
// below it then produced exactly the request the scan was there to
|
||||||
|
// prevent. Any pass that can join tokens has to happen before validation,
|
||||||
|
// not after.
|
||||||
const urlPass = stripDisallowedUrls(sanitized);
|
const urlPass = stripDisallowedUrls(sanitized);
|
||||||
if (urlPass.blocked > 0) {
|
if (urlPass.blocked > 0) {
|
||||||
warnings.push(
|
warnings.push(
|
||||||
@@ -306,10 +316,6 @@ function sanitizeCSS(cssContent) {
|
|||||||
sanitized = urlPass.sanitized;
|
sanitized = urlPass.sanitized;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Remove control characters
|
|
||||||
// eslint-disable-next-line no-control-regex -- intentional: strips control chars from untrusted CSS
|
|
||||||
sanitized = sanitized.replace(/[\u0000-\u001F\u007F]/g, '');
|
|
||||||
|
|
||||||
// Remove any remaining script-like content
|
// Remove any remaining script-like content
|
||||||
sanitized = sanitized.replace(/<[^>]*>/g, '/* BLOCKED TAG */');
|
sanitized = sanitized.replace(/<[^>]*>/g, '/* BLOCKED TAG */');
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user