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

Option to unicode escape <, >, and & in JSON output #3268

Closed
g-k opened this Issue Apr 5, 2017 · 17 comments

Comments

Projects
None yet
4 participants
@g-k
Copy link
Contributor

g-k commented Apr 5, 2017

An option to unicode escape <, >, & as \u003c, \u003e, \u0026 "to keep some browsers from misinterpreting JSON output as HTML."

https://golang.org/src/encoding/json/encode.go?s=6456:6499#L48

This can be implemented as a middleware layer monkey-patching res.send as below, but it might be nicer to have in core.

const escapeJson = function (json) {
    if (typeof json !== 'string') {
	return json;
    }
    return json.replace(/</g, '\\u003c')
	       .replace(/>/g, '\\u003e')
	       .replace(/&/g, '\\u0026');
};

module.exports = function(req, res, next) {
    var _send = res.send;

    res.send = function (body) {
	if (res.get('Content-Type') === 'application/json') {
	    body = escapeJson(body);
	}
	_send.call(res, body);
    };

    next();
};
@dougwilson

This comment has been minimized.

Copy link
Member

dougwilson commented Apr 12, 2017

Hi @g-k I can certainly see the use-case this would serve. Would like to hear what others think as well, as I'm generally neutral on this right now.

@thamaranth

This comment has been minimized.

Copy link

thamaranth commented Apr 26, 2017

I think it would be useful.

@wesleytodd

This comment has been minimized.

Copy link
Member

wesleytodd commented Apr 26, 2017

I do not think this is the responsibility of express. I think it is, and should remain, the developer who is inserting the json into the html's job to properly escape the content.

FWIW, I have two modules to for this, one for the server side escaping, and one for the client side unescaping, both using the underlying escape method from lodash:

https://github.com/wesleytodd/recursive-escape
https://github.com/wesleytodd/recursive-unescape

@g-k

This comment has been minimized.

Copy link
Contributor

g-k commented Apr 28, 2017

The issue is escaping JSON so it isn't misinterpreted as an HTML page when navigated to directly (for browsers that do mime type sniffing). For example, XSS from a JSON response like:

{"post_id":"81730c8682f1efa5","title":"<img src=x onerror=alert(1)>"}}

X-Content-Type-Options: nosniff should be set as a defense in depth measure too.

Using an escaping library and res.send with a string or buffer would work too, but I'd rather have a safe default that works with res.json and res.send called with an object.

I think the proposed change is a reasonable default or option for users to enable. Providing a hook for encoding (like node-restify formatters) is an option too.

@wesleytodd

This comment has been minimized.

Copy link
Member

wesleytodd commented Apr 28, 2017

Oh, that is a different context than I got from reading the comments in the go code, which just talked about embedding it in a script tag. I still think doing it by default is a bad idea, but I could get onboard with an option to turn this escaping on.

@g-k

This comment has been minimized.

Copy link
Contributor

g-k commented Apr 29, 2017

Oh yeah, the script context thing is for the line and paragraph separators which express already does for JSONP: http://timelessrepo.com/json-isnt-a-javascript-subset

This does incur a small perf cost and some apps won't care or can guarantee they aren't consumed by older brower clients (e.g. for internal or mobile app apis), so I'd be in favor for having an option to enable this with it off by default.

@wesleytodd

This comment has been minimized.

Copy link
Member

wesleytodd commented May 1, 2017

Just for clarification, this use case is ONLY for browsers directly requesting the json asset and displaying it to a user, not for any ajax or programmatic use?

If the answer to the above is true, I really do think that this should be left to a third party library, as it is too niche of a use case, IMO, to receive built in support. This could be as simple as:

var myJsonEscaper = require('my-json-escaper')

app.use(function (req, res) {
  res.json(myJsonEscaper({foo: '<bar>'}))
})
@g-k

This comment has been minimized.

Copy link
Contributor

g-k commented May 4, 2017

this use case is ONLY for browsers directly requesting the json asset and displaying it to a user, not for any ajax or programmatic use?

Yep, at least I can't think of any other use cases at the moment. If an attacker can get someone to visit their site they can programmatically redirect or window.open the JSON page for XSS. Or iframe it if the JSON endpoint is missing X-Frame-Options. There are a couple examples from the wild in: https://blog.qualys.com/securitylabs/2014/09/11/xss-vulnerability-shows-how-security-issues-can-creep-into-popular-software

In general, I'd rather have a safe default to enable than use middleware to monkey patch res.send or manually escape all JSON passed to res.json and res.send.

Escaping is a built-in option in rails and google considers it enough of a best practice to enable by default in the stdlib in go.

@wesleytodd

This comment has been minimized.

Copy link
Member

wesleytodd commented May 4, 2017

According to that post:

By referring to the methods provided in the article, I have confirmed that this vulnerability could be reproduced and exploited with IE8 (Exploited with IE8.0.6001.17184), IE7 and IE6. But I was unable to exploit the vulnerability with IE9 because the response page whose content type is application/json could not be rendered by IE 9.

As much as I totally agree we should have safe defaults, Microsoft dropped support for ie9 almost a year ago, so IMO setting a default behavior that would be a breaking change is not only a bad idea, but mostly pointless from a security perspective. The real issue is that by escaping the data, something on the other end has to unescape it, thus this being a breaking change.

@g-k

This comment has been minimized.

Copy link
Contributor

g-k commented May 4, 2017

@dougwilson dougwilson added this to the 4.16 milestone Jul 19, 2017

@dougwilson

This comment has been minimized.

Copy link
Member

dougwilson commented Jul 19, 2017

Unless there is a strong objection, I'm proposing that this lands in the 4.16.0 release.

@wesleytodd

This comment has been minimized.

Copy link
Member

wesleytodd commented Jul 20, 2017

My only objection is that this has weak reasons for inclusion. IE9 support is a non-reason for me, and I have never worked on, nor heard of an application, where non-developer users are encouraged to directly visit a url that serves a json body. Also considering how easy it is for a third party library to solve this issue for the people who have it, I believe this is best left to the user space.

@dougwilson

This comment has been minimized.

Copy link
Member

dougwilson commented Jul 20, 2017

It loos like the discusion above has some confusion going on. This is strictly a to prevent a form of XSS attack. For example, the extra encoded chars do not require any extra unescaping on the cliebt side to work; it is just standard JSON escaping. An attacking website can include an iframe (or even a top level redirect) to a JSON endpoint due to no faut of yours. If it is then one of the web browsers who then in turn see the large number of angle brackets and decide that it should be displayed as HTML instead of as text, then whatever HTML was in that response will execute.

An example attack is a comment board where all comments are plain text. User can submit anything and you duefully properly insert it on your page to display. You can load these comments via AJAX over a JSON endpoint, though. Now a user posts a comment with a script tag which doesn't cause XSS because you accounted for properly encoding everything that displays it, but it was unfortunate for a user who visited a sute where the ad network was malicious and included an iframe tag to your comments JSON where they planted that comment. Cookies are send to the attacker.

@dougwilson

This comment has been minimized.

Copy link
Member

dougwilson commented Jul 20, 2017

Effectively this is an issue for all users of Express.

@wesleytodd

This comment has been minimized.

Copy link
Member

wesleytodd commented Jul 20, 2017

Ok, that is much more clear to me. Thanks @dougwilson! This explanation changes my whole position from above. Do we plan on turning this on by default for 5.x?

@dougwilson

This comment has been minimized.

Copy link
Member

dougwilson commented Jul 20, 2017

Glad it helped :) ideally it could just be default in 4.x especially since it is transparent for all well behaved clients, but the size of the install base makes me iffy on that. We can always turn it on in a later 4.x if warranted or even yell (ugh) at users to make them either opt in or out, not sure. As for 5.x itself I didn't givr too much though besides to follow 4.x, but maybe it should default on in 5 even if it never does in 4.

@dougwilson dougwilson referenced this issue Sep 25, 2017

Merged

Release 4.16 #3423

20 of 20 tasks complete

@dougwilson dougwilson self-assigned this Sep 28, 2017

@dougwilson

This comment has been minimized.

Copy link
Member

dougwilson commented Sep 29, 2017

Delivered as part of the 4.16 release, currently off by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment