security: js injection

[mifi]

See discussion in https://github.com/transloadit/uppy/pull/2967/files/f70ac754ba2074af8c5d3cad14568148d0fb1122#r659685817

uppy/packages/@uppy/companion/src/server/controllers/send-token.js

Line 55 in f4e99fa

window.opener.postMessage(JSON.stringify({token: "${token}"}), "${sanitizeHtml(origin)}")

[Murderlon]

@goto-bus-stop @mifi @aduh95 is this still relevant?

[aduh95]

For reference, we're now passing sanitizeHtml which should protect us from that.

uppy/packages/@uppy/companion/src/server/controllers/send-token.js

Line 18 in 8f7e63b

window.opener.postMessage(${sanitizeHtml(JSON.stringify({ token }))}, ${sanitizeHtml(JSON.stringify(origin))})

However, the patch has not been released to a stable release yet:
@latest (still vulnerable): https://unpkg.com/@uppy/companion@2.12.1/lib/server/controllers/send-token.js
@next (patched): https://unpkg.com/@uppy/companion@2.13.0-alpha.0/lib/server/controllers/send-token.js

[Murderlon]

But do we still want to implement one of @mifi's suggestions, or will this suffice? AFAIK we're good currently.

I think the only easy safe things to do is to either

1. escape the token/origin as html and put it inside a hidden DIV as JSON.stringified, then use javascript in the document to parse the div content before using it.

2. if the token/origin only consists of a certain type of characters (e.g. alphanumeric), then do a token.replace(/[^0-9a-z]/ig, '') before inserting it to the document. then it's guaranteed to not contain any unsafe characters.

[mifi]

Yes I believe those are the only safe options. A third safe option is to use a library like serialize-javascript to safely json stringify and escape html and js line terminators. See https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html#rule-3-javascript-encode-before-inserting-untrusted-data-into-javascript-data-values

To clarify: just doing normal escapehtml + normal json.stringify is not safe inside a <script> tag.

[Murderlon]

Alright! At first, option 2 seems the most straightforward to me, if the token is indeed alphanumeric.

[aduh95]

To clarify: just doing normal escapehtml + normal json.stringify is not safe inside a <script> tag.

I'm unconvinced that's true, JSON.stringify returns "a String in UTF-16 encoded JSON format representing an ECMAScript value, or undefined", so it's guaranteed by spec to be safe in a JS environment. As the article points out, a closing </script> tag would allow to escape to HTML and create an XSS exploit, but escapehtml would prevent that. The other character would be a risk only only if we were inside a HTML attribute according to that article, but that's not the case here. Or am I missing something?
That being said, if the token is indeed alphanumeric, sure doing token.replace(/\W/g, '') would be fine and address all the concerns.

[goto-bus-stop]

https://github.com/zertosh/htmlescape does json.stringify+escaping </script>s

[mifi]

Normal JSON.stringify does not escape all JS line terminators. This is important because inside a script tag, line terminators can be used to end a js statement and then start a new statement (e.g inject JS code, see yahoo/serialize-javascript#19). From the owasp article I linked:

A safe JSON serializer will allow developers to serialize JSON as a string of literal JavaScript which can be embedded in an HTML in the contents of the <script> tag. HTML characters and JavaScript line terminators need be encoded. Consider the Yahoo JavaScript Serializer for this task.

Browser security is hard so imo it's best to just follow advice from experts. It's easy to make a mistake and leave a hole open if we try to make our own solution. Seems like https://github.com/zertosh/htmlescape handles the line terminators, but it does not seem to escape /. I'm not sure which potential exploits that might open up. See:

• https://github.com/yahoo/serialize-javascript/blob/7ad4d72396ec72836bae610d95958b3655ec276d/index.js#L25
• https://github.com/zertosh/htmlescape/blob/02dbcc367dd3069b73253ac08d87a40d37984239/htmlescape.js#L9

[aduh95]

For the record, those line terminators should no longer be an issue in modern JS engines: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Globalaa_Objects/JSON/stringify#issue_with_plain_json.stringify_for_use_as_javascript

Historically, JSON was not a completely strict subset of JavaScript. The literal code points U+2028 LINE SEPARATOR and U+2029 PARAGRAPH SEPARATOR could appear literally in string literals and property names in JSON text. But they could not appear literally in similar context in JavaScript text, only using Unicode escapes as \u2028 and \u2029. This recently changed: now both code points may appear literally in strings in JSON and JavaScript both.

This change was apparently made in ES2019 (I thought it was older than that), so it's indeed probably fair to assume not everyone has updated their parser to support these.

Browser security is hard so imo it's best to just follow advice from experts.

+1

[Murderlon]

Does this mean we still want to go with option 2? IMO it's a simple change if it takes away any doubt.

[mifi]

I also thnik 2 is the simplest. The only disadvantages are:

• if we are not 100% sure that the tokens are guaranteed to be alphanumeric and the providers change this in the future, it would suddently start to crash in the future
• if a developer in the future sees the string.replace code they may be tempted to swap it out with a encode HTML and reintroducing a vulnerability. This can be worked around with a comment above the line.

[mifi]

I created a PR with the serialize-javascript solution, as it seems to be the most safe and convenient solution