Security vulnerability for non-HTTP URLs

[redonkulus]

@rrdelaney @okuryu

We got a report internally that the new URL option can introduce a security vulnerability in the code.

When URLs are serialized, unsafe HTML characters can still be introduced through non-http URLs. When reflected into a <script> element, the serialized code can escape out of the script tag by introducing </script> in the URL.

Steps to reproduce

const serialize = require('serialize-javascript');

let x = serialize({
    x: new URL("x:</script>")
});

console.log(x)

Expected Output: The <, / and > characters should be escaped (through escapeUnsafeChars).

Actual Output:

{"x":new URL("x:</script>")}

Impact

When reflected into a <script> element, XSS can occur. This requires the attacker to control the URL being serialized.

Potential Fix

The string in the new URL constructor could be sanitized by passing it through the serialize function.

if (type === 'L') {
    return "new URL(\"" + serialize(urls[valueIndex].toString()) + "\")"; 
}

Output change:

AssertionError: expected 'new URL(""x:\u003C\u002Fscript\u003E"…' to equal 'new URL("x:</script>")'
+ expected - actual

-new URL(""x:\u003C\u002Fscript\u003E"")
+new URL("x:</script>")

I've tried out this fix, but it will also escape the characters in the URL as well. I am not sure if this will introduce any breaking changes.

Opening this up for thoughts and ideas on a fix.

[rrdelaney]

@redonkulus Good find! Adding serialize there should work, I think the tests would just need to be updated. I don't think it would be a breaking change for the deserialize or serialize API's, but if someone is depending on the intermediate format it might.

[redonkulus]

@rrdelaney do you care to open a PR with the fix?

[rrdelaney]

@rrdelaney do you care to open a PR with the fix?

Sure!