Security issue in this repo - Deserialization of Untrusted Data

Hi guys, we are using Synk which found that this repo has a high severity security issue.
https://app.snyk.io/vuln/SNYK-PYTHON-JSONPICKLE-1052447

Can someone take a look at it?

Sorry, this is documented and in common with the stdlib pickle module.

https://jsonpickle.readthedocs.io/en/latest/#jsonpickle-usage

Duplicate of #332 so there's nothing to do here. Don't pass untrusted input to jsonpickle. It's perfectly safe if you are in control of your inputs.

@davvid

This library is used when parsing input json, so yeah, it might be untrusted, that's the whole point, to validate.. @davvid

Sorry, you're asking for something that is equivalent to solving the halting problem, which is impossible.

https://en.wikipedia.org/wiki/Halting_problem

@davvid

Bluntly: if you are taking in untrusted data and passing it into jsonpickle, you almost certainly have a security issue and you should consider switching to another tool. Even if you write both the client and server code if there is an untrusted entity running the client. You could attempt to sanitize the input server-side yourself, but personally, I suspect this task is daunting to do correctly and would be better spent using the stdlib's json module to specifically and manually reanimate your data structures.

When I first wrote jsonpickle in 2008 it was merely my exploration of the Python object model -- attempting to persist arbitrary graphs of Python objects and restore them via the JSON format, which only allows strings, numbers, booleans, lists, dicts, & null. At that time, very few tools provided native JSON-serialization (whereas now we have a plethora of native methods like Keras' model.to_json()). @davvid and numerous other authors have made the system incredibly more advanced, and I doubt that any of my original code exists at this point. What follows is my opinion as an "alumni" of the project that others may disagree with.

I have occasionally thought about a "Jurassic Park" line when I look back to 2008:

Your scientists were so preoccupied with whether or not they could, they didn’t stop to think if they should.

In isolation, jsonpickle is a reasonable tool, but it is likely a security issue for some dependent projects. I am happy to see tools and researchers help developers see an extra warning flag outside of the documentation. As @j0lt-github recently did, providing a proof of concept of an RCE.

I am glad dependency scanning tools like snyk exist and it is the first tool I'm aware of to identify jsonpickle as a risk. However, it would be more ideal if static code analysis tools were aware of jsonpickle and could help a package author reason if jsonpickle is getting untrusted input or not.

Similar to Python's own pickle, which snyk also notes (9. Deserialize selectively), jsonpickle allows for the arbitrary content that is then reanimated into Python objects in memory. Given the dynamic nature of Python and the natural challenges of sandboxing it, I can not envision a path forward that would allow jsonpickle to work while simultaneously sanitizing an arbitrary object graph. jsonpickle is more similar to the browser's eval() than JSON.parse().

In my opinion, the jsonpickle project could consider several improvements:

In the documentation, provide further guidance of when it may be appropriate and inappropriate to use jsonpickle. My personal fear is a novice developer making a website who googles "pickle JSON" and does not understand what "untrusted data" means.
Re-consider the inclusion of jsonpickleJS as part of this repository (or organization). The jsonpickle maintainers may disagree, but personally, this feels like an endorsement of a possibly dangerous architectural pattern.
Work with security researchers & products to attempt to add static code analysis of calls to jsonpickle, the same way they alert of calls to pickle

There are interesting applications of AST trees in python which I have seen used to make safe versions of eval() -- like ast.literal_eval, simpleeval, and more.

Do you (davvid, johnpaulett) think jsonpickle could be made more safe by using AST to parse, or is this a misunderstanding of how jsonpickle functions?

Edit: Add link to ast.literal_eval

@johnpaulett

I like the ideas that @johnpaulett outlined above. Regarding jsonpickleJS, moving it into a contrib/ subdirectory with a new README in that directory that clearly outlines the dangers and generally discourages its use for most applications might be a nice middle-ground. I dunno, maybe removing it and linking back to the original project, alongside adequate warning and guidance, is better because then we don't actually carry the JS code around with us and it makes it clear that there are many security pitfalls when accepting arbitrary input.

One approach to making jsonpickle more safe that I was going to look into pursuing in the future (unless someone beats me to it or has a better idea) was going to be the addition of a new allow_imports={None/set/list/tuple} argument for both encode() and decode().

The decode() implementation would be the most beneficial security-wise of the two, but we'd want the feature for both. When allow_imports is non-empty then we would only import modules and types that fnmatch the patterns specified by the user. Any requests to import something that was not allowed would throw a ValueError exception with a message specifying which name could not be imported.

That would allow users to do something like allow_imports=['my_package.*'] and then jsonpickle will only allow importing stuff from my_package. For now, throwing an exception seems best since all-or-nothing is a sensible approach for this feature.

For encode(), the same mechanism can prevent json from being emitted altogether. It can also throw a ValueError exception when it gets asked to encode an object which would result in importing objects that are outside the allow_imports set/tuple/list.

Regarding eval and literal_eval -- we do use literal_eval in a few places, but loadrepr() seems to be the only place where we call eval() as a fallback code path and rely on being able to eval the repr() representation of an object. That functionality could also be made to respect allow_imports by enforcing that both the module name and the repr string itself (note that the encoded representation has it as <module>/<repr>) have to be specified in allow_imports.

Regarding security researchers -- I'd be happy to work with security researchers to have jsonpickle calls get flagged as being as dangerous as pickle but I don't have any contacts. If there's anyone we can reach out to that'd be a good thing to do; please let me know if you know of any projects we should approach. If we had something like allow_imports then one thing they could flag in the future would be any code that doesn't supply the argument.

What do y'all think?

I'd love to be wrong, but think sandboxing python is a "Hard" problem to do correctly. For instance, consider a module demo.py and if I just wanted to allow imports from demo.*

import sys

class MyClass:
  ...

Unfortunately, my code's imports are re-exported and I have access to a whole range of things I might not intend

$ python3
>>> import demo
>>> demo.sys
<module 'sys' (built-in)>
>>> demo.__builtins__
...

Stringing this into an attack vector would take some work, but I suspect it could be done.

I don't want to stop productive work but suspect that trying to sandbox via an allow-list or AST transforms needs to be perfect all the time.

That's too true, it's a very hard problem. I wonder whether my immediate suggestion to go with fnmatch is a bad idea since it'd be better to force users to be explicit and specify demo.MyClass so that nothing else can leak through, especially for a sensitive feature that is intended to be an additional tool to harden things. The classic security vs. convenience trade-off comes to mind in that a user might be annoyed that they have to enumerate everything they allow, but it's still better than the alternative where the feature becomes a footgun.

loadrepr() seems like it'd be pretty darn hard to use with allow-lists without wildcards, so having a way to disable the eval() code path completely might be a better idea than trying to make it usable, it seems.

Yeah, it's true that sandboxing python is an extremely hard problem so I'm not sure whether or not allow-lists would be a useful feature in general. Giving users a false sense of security is not good and we definitely don't want such a feature to be interpreted as a silver bullet or remedy for dangerous architectures. But maybe it'd be helpful for some subset. I'm definitely open to sugs here ;-)

docs: better document the security implications

I've since made the Security warnings much more prominent in the documentation. We've adopted the same exact guidance and warnings from the stdlib pickle module.

I'm not familiar at all with security researchers and products and how we might interact with them to have jsonpickle treated the same as pickle. I opened issues with snyk's python plugin and python-security's pyt tool to see if they might be able to provide some guidance on how to proceed.

Can this be closed since https://security.snyk.io/vuln/SNYK-PYTHON-JSONPICKLE-1052447 has been revoked?

@tysonclugg

Sure. Thanks for letting us know @tysonclugg! And for anyone popping in on this conversation in the future, please know that you can pass safe=True to jsonpickle.decode to avoid jsonpickle using eval, but only if you don't need compatability with jsonpickle<0.7.

Navigation Menu

Repository navigation

Security issue in this repo - Deserialization of Untrusted Data #335

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Security issue in this repo - Deserialization of Untrusted Data #335

Description

Activity

davvid commented on Dec 24, 2020

ran-isenberg commented on Dec 24, 2020

davvid commented on Dec 24, 2020

johnpaulett commented on Dec 24, 2020

Theelx commented on Jan 20, 2021

davvid commented on Jan 20, 2021

johnpaulett commented on Jan 20, 2021

davvid commented on Jan 21, 2021

davvid commented on Jul 3, 2021

tysonclugg commented on Aug 8, 2022

Theelx commented on Aug 8, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions