dangerous code in json.erl #50

Closed
lamvak opened this Issue Feb 9, 2011 · 7 comments

Projects

None yet

3 participants

@lamvak
lamvak commented Feb 9, 2011

This is a real issue.
We know that atoms are not garbage collected. Yet someone designed the jaws library the way, that it always create atoms when user decodes json objects. This is dangerous. We should rather change the design so that either we'll use list_to_atom( X, [safe]), or even better - we shan't use list_to_atom at all.
It's important, because json decoding is intended to be heavily used in servers - in deployment environment, not just e.g. while the compilation. This makes yaws (and in fact any application using this version of library) vulnerable to attack on server's memory.

@klacke
Owner
klacke commented Feb 10, 2011

Hmmm, you are right, it's not an easy thing to change this though. I didn't write the json lib, and frankly I wasn't aware of this. We had a similar issue a couple of years ago with parsing of the query part of an URL, and we did change. I'm not sure what is the proper way to handle this. Just changing, will break a lot of code. Boring.

@vinoski
Collaborator
vinoski commented Feb 17, 2011

Since we can't break existing code, I would think the only way to really fix this would be to leave the original module in place but copy it to a new module with a different name where the fix can be made. We could then put a big warning into the original module explaining the problem and advising users to switch to the new one.

@vinoski
Collaborator
vinoski commented Apr 10, 2011

Anyone interested in json might want to try this code:

https://github.com/davisp/jiffy

@lamvak
lamvak commented Apr 14, 2011

cool.
thanks for the link. it's nice. i'm doing some alternative prototyping on the subject, i'm going to put in my tuppence soon.
anyways, i wonder what would be the best course of action with this issue? is there any code in yaws itself using this library?
if you don't know just already, i can analyze the newest revisions
in case there's no use of this library in yaws itself i would vote just to add some strong depreciation in documentation (in repository and on the web site) and to close this issue

@vinoski
Collaborator
vinoski commented Apr 14, 2011

There are modules in yaws that use the json module, but I'm not sure how heavily those modules are used or whether the details of the json representation leak out of those modules. Seems like one good plan of action might be:

  • either create a new json module from the existing one, fixing the problems with it, or grab another json module from somewhere else, like from the jiffy repo
  • change existing modules in yaws that rely on the json module to use the new one
  • put large warnings and deprecation notices in the existing json module
@vinoski vinoski was assigned Apr 18, 2011
@vinoski
Collaborator
vinoski commented Apr 18, 2011

What makes this extra fun is that the json module doesn't even currently pass its own internal tests.

@vinoski
Collaborator
vinoski commented Apr 24, 2011

OK, this is now fixed. I pushed a new module named json2 that avoids list_to_atom and also passes all its tests.

@vinoski vinoski closed this Apr 24, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment