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

Allow top level JSON arrays again #1009

Closed
ntrrgc opened this Issue Mar 8, 2014 · 8 comments

Comments

Projects
None yet
6 participants
@ntrrgc

ntrrgc commented Mar 8, 2014

At the moment Tornado forbids writing JSON lists due to a potential cross-site vulnerability explained in an article from 2008.

Note, however, that vulnerability is now 8 years old (first records I find on it are from 2006), and in the 2008 article it only affected Firefox <= 2.0 (not even IE6!), which ended security support the same year.

Does it make sense for us to maintain this kind of limitation because of an ancient vulnerability in browsers that has been fixed 6 years ago?

Related: #109

@bdarnell

This comment has been minimized.

Member

bdarnell commented Mar 11, 2014

There is a second array-related vulnerability: http://haacked.com/archive/2009/06/25/json-hijacking.aspx/ This one uses Object.prototype.__defineSetter__. This has been an issue more recently

So even though the Array-redefinition trick has long been fixed, it's unclear how long ago the defineSetter bug was fixed. Even if both bugs have been fixed, the history of top-level JSON arrays suggests that it is still prudent to accept the minor inconvenience of wrapping everything in a top-level object (Flask does this too: http://flask.pocoo.org/docs/security/). The comments should probably be updated to point to one of the more up-to-date json-hijacking articles.

@ntrrgc

This comment has been minimized.

ntrrgc commented Mar 11, 2014

Oh, I was not aware of the new vulnerability. Thanks for the tip.

It seems prudent to keep the padding for the time being, although the docs should be updated on the matter.

@jameswald

This comment has been minimized.

jameswald commented Mar 31, 2015

According to http://kangax.github.io/compat-table/es5/ most browsers are ES5 compliant, though it isn't entirely clear if any are still vulnerable to JSON hijacking. It's hard to find any information on this topic after 2013 other than this GitHub issue. Perhaps there are certain situations that may warrant additional security measures, but it may be about time to drop this requirement from Tornado.

@bdarnell

This comment has been minimized.

Member

bdarnell commented Mar 31, 2015

Can you explain to a javascript-illiterate like me how that chart relates to the vulnerabilities discussed here?

In any case, given the history of multiple vulnerabilities in this area it seems unwise to me to ever relax this restriction, since the benefit is extremely small. If you want to send JSON arrays without a dict wrapper (and you've satisfied yourself with the security of this decision), you can: it's a two-line change in your application to call json.dumps yourself and set the content-type accordingly.

@jameswald

This comment has been minimized.

jameswald commented Mar 31, 2015

Though that chart might suggest that most browsers conform to ES5+, it doesn't directly test for conformance with the note in 11.1.4 Array Initialiser:

NOTE [[DefineOwnProperty]] is used to ensure that own properties are defined for the array even if the standard built-in Array prototype object has been modified in a manner that would preclude the creation of new own properties using [[Put]].

I'm not a JavaScript expert myself, however, I think this requirement prevents the array initializer content from being hijacked by __defineSetter__ because the setter shouldn't be used by the initializer. Considering that the state of this issue is still unclear and that the workaround is simple enough, it does seem wise to stick with the existing policy.

@jakub-g

This comment has been minimized.

jakub-g commented Sep 25, 2015

FYI, I've been searching around for info whether JSON hijacking is still an issue and found this.

According to this stackoverflow thread, it's no longer an issue with modern browsers. It's been fixed many years ago both in Firefox (3.5 which has javascript 1.8.1) and Chrome. IE9 is also safe, and Object.defineProperty was not available in IE8-. I suppose it's been long fixed in Safari too.

So it seems the only vulnerable browsers are very ancient Firefoxes and webkits.

@kived

This comment has been minimized.

kived commented Aug 25, 2016

Instead of worrying about the "limitation" imposed, take advantage of the fact that you have to wrap your responses in an object by adding pagination!

GET /stuff/?page=2&count=10
{
    "results": [
        {"name": "stuff11", "links": {"self": "http://examplesite/stuff/stuff11/"},
        {"name": "stuff12", "links": {"self": "http://examplesite/stuff/stuff12/"},
        ...
        {"name": "stuff20", "links": {"self": "http://examplesite/stuff/stuff20/"}
    ],
    "links": {
        "next": "http://examplesite/stuff/?page=3&count=10",
        "previous": "http://examplesite/stuff/?page=1&count=10"
    }
}
@dyong0

This comment has been minimized.

dyong0 commented May 3, 2017

@kived +1 Even I am not the user of tornado. This idea is better than having an array structure at the top level in most cases.

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