Review JSON Security #248

Closed
mitsuhiko opened this Issue Jun 20, 2011 · 26 comments

Projects

None yet

8 participants

@mitsuhiko
Member

ECMAscript 5 changed behavior of Array literals. That part of the documentation is outdated by now. We should check what browsers are affected by this and update the docs accordingly.

@untitaker untitaker added this to the 1.0 milestone Oct 19, 2014
@duaneking

Can you please explain what you mean by 'security issues'?

mitsuhiko/flask-sqlalchemy#229

@mitsuhiko
Member

The linked issue is unrelated to this one.

@duaneking

The linked issue was closed due to a statement by you that reflected the idea that it was a security issue; Wouldnt this being about that same security issue mean they are related?

Also, with respect, you didn't answer my question as to what the security issue was.

@davidism
Member

The security issue mentioned on that issue has nothing to do with JSON. The issue he talks about there is exposing sensitive data unless attributes are whitelisted. He makes the point that if attributes are whitelisted we might as well manually serialize the objects. The conclusion is that an automatic solution is not appropriate for the extension, but should be implemented in the author's application.

@duaneking

Respectfully, The problem here is that @mitsuhiko has claimed on multiple topics that he is closing them due to security concerns, but has not in any of these cases clarified what that exact security concern is in that context. The result is the current misunderstanding.

@DasIch
Contributor
DasIch commented Oct 20, 2014

@duaneking The jsonify() feature you proposed, mirrors well known similar features in Ruby on Rails and Django in regard to automatic form generation and parsing of query arguments that caused severe security issues in a huge number of web applications, using these frameworks. Both projects have either deprecated or already removed such features. In general automatically generating anything users can interact with using a non-whitelist method is likely to cause information leakage, unauthorized writes and may provide a vector for DoS attacks.

If that isn't obvious to you (as you might have gathered it is, to several other people), you should educate yourself on basic security measures when writing software in general and web applications in particular. Reading about security issues (Django has excellent documentation on those) can also be very helpful.

@duaneking

Respectfully, Please stop making assumptions about what I do or do not know. Its rude, and quit frankly as developers we should treat each other better than you just treated me.

I'm fully aware of these issues in other frameworks; I'm also fully aware that other people making a mistake is on them, not on me in any way. I'm not going to ever accept the false idea that what other people choose to do somehow can be blamed on me. They have freedom of choice; Its up to them to be responsible for their own actions.

The jsonify issue as you pointed out, was separate from this; Why troll me?

@untitaker
Member

This bug has nothing to do with database models. In case you decide not to derail this issue for once, please consider we already agreed that the current array-restriction in Flask's jsonify is unnecessary, which is exactly what you're advocating. Unless you have something new to say on that topic, spare the overly aggressive drive-by remarks.

@nickretallack

How about we make it easy to opt out of this feature, and check for it specifically? My patch should do that. Please review.

@untitaker
Member

The problem is that the reason this was prohibited doesn't really matter nowadays, as discussed in the linked issues.

While that may be possible, if you have to defend the API with "it can be worked around" you have unquestionably failed at its design.

So if we provide such an option where the default is already to disable the protection, we might as well remove that mechanism in its entirety, as it is a trivial one and isn't useful to a majority of Flask's userbase.

@nickretallack

The default in my patch is to enable the protection.

The exploit affects IE 6, 7, and 8 because they use ECMAScript 3.

I wish people didn't use IE8, but some people still do.

@ThiefMaster
Member

There haven't been updates for these browsers fixing the hole?!

@DasIch
Contributor
DasIch commented Oct 20, 2014

There haven't been updates for these browsers fixing the hole?!
"Fixing" this means breaking backwards compatibility.

I still have to target IE 7 running on the remaining Windows XP machines at work, I very much doubt that I'm the only one. I'm -1 on changing the default behavior, given that IE 7 is apparently affected. =

@duaneking

I agree with @DasIch , I do not feel that supporting an insecure, old, and not updated browser improves security in any way as it only allows such browsers to propagate and harm others.

@nickretallack Would a back-end that uses this browser dependent code also need to mirror the same browser dependent interface on the client side, and use a custom client specific to that old version of IE?

@nickretallack

@duaneking what? I think you are confused.

There is nothing browser-dependent here. This "security feature" is only designed to warn web app developers that, if your users are running old IE, attack sites can read JSON responses that they shouldn't be able to read. If you don't care about old IE users leaking data, possibly because your site explicitly disallows IE8 entirely, you can disable it.

The purpose of my patch is to make this protection more explicit and understandable, and not prevent other useful things.

@DasIch
Contributor
DasIch commented Oct 21, 2014

I agree with @DasIch , I do not feel that supporting an insecure, old, and not updated browser improves security in any way as it only allows such browsers to propagate and harm others.

This is not at all what I said. I'm of the opinion that we should not change the default behavior of Flask from what it is right now. So we should continue to raise an exception, if a list is passed to jsonify().

The fact that IE7 is outdated alone, may be sufficient reason for a developer to stop ensuring that a website looks as intended or is even usable in IE7. It's not a sufficient reason to not ensure that these users remain safe. Flask users can always override this decision but we should go with the conservative option for the framework itself.

@danielchatfield
danielchatfield commented Oct 21, 2014 edited

Just to reiterate what I said before (I cited a github dev's (formerly Microsoft) article on it), every browser affected by this (IE 5 and below, FF2 etc.) also has unpatched RCE bugs.

IE 7 is not affected by this.

@untitaker
Member

@danielchatfield That is directly contradictory with what @nickretallack said.

@untitaker
Member

In this new situation, where it is at least not clear whether this is a security issue, i suggest we merge #1209 (with the exact behavior as it stands) for 1.0.

@danielchatfield

@untitaker Those browsers have been patched though, and if a user is running a pre-patch version (disabled windows update) then they also won't have several RCE fixes.

@untitaker
Member

Ah i see.

@nickretallack

Yeahs, sorry for sounding so definitive. I am not really sure if the exploit is possible in IE 6,7,8. If it isn't, feel free to ditch this feature.

@untitaker
Member

So if i understood that correctly, IE versions 6, 7, 8 are vulnerable unless they're patched. @danielchatfield argues there are far worse security issues in the unpatched versions, so this one shouldn't matter.

I am still not sure about this. I think we should use the linked PR as-is, and update the security docs with the current status on vulnerable browser versions, so the user can decide if they want to take that risk.

@nickretallack

Now that I think about it, this is really another case where CSRF protection could be used. The only time you'd want to load JSON data in a browser is in an XHR, and XHRs can easily set the X-CSRFToken header. Also, the only time you'd leak private information is if you're using cookies. So one way to implement the same security measure would be:

If all of these apply:

  • The request contains a session cookie
  • The request method is GET (or any other method, really)
  • The response type is application/json

Then:

  • Ensure the X-CSRFToken header is set correctly

Anyway, I'm curious to see if people can reproduce the exploits in any browsers than anyone still uses. Anyone have old IE?

I guess the point of the existing implementation was to ensure the output is always an object because anything beginning with { fails to parse as javascript, since it is interpreted as a block instead of an object. In light of this, perhaps I should change the implementation to ensure the response starts with { instead of preventing it from starting with [. But that may not be necessary if there's no way to hijack a string or integer.

@untitaker
Member

Sorry for responding so late:

But that may not be necessary if there's no way to hijack a string or integer.

There shouldn't be... I think the issue is specific to arrays.

@davidism davidism closed this Jan 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment