Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security vulnerability for non-HTTP URLs #172

Closed
redonkulus opened this issue Jan 9, 2024 · 3 comments · Fixed by #173
Closed

Security vulnerability for non-HTTP URLs #172

redonkulus opened this issue Jan 9, 2024 · 3 comments · Fixed by #173

Comments

@redonkulus
Copy link
Collaborator

@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
Copy link
Contributor

@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
Copy link
Collaborator Author

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

@rrdelaney
Copy link
Contributor

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

Sure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants