Possibly unsafe use of pickle()

[rcarmo]

I've recently come across http://vudang.com/2013/01/python-web-framework-from-lfr-to-rce/, which details possible vulnerabilities related to unpickling cookies.

I've noticed that pickle is used in https://github.com/rcarmo/beaker/blob/master/beaker/session.py#L288, and was wondering if you had any plans to change that.

[bbangert]

Alternatively, perhaps just switching to this:
http://home.gna.org/oomadness/en/cerealizer/index.html

And forcing people to register any class they want to serialize?

[amol-]

For most use cases the json module is able to serialize and deserialize most data people store inside their sessions.
Would it be a feasible solution to use json by default and provide a configuration option to change serialization backend until it supports the loads/dumps functions?

[rcarmo]

I'd go with JSON.

On Apr 18, 2013, at 21:29 , Alessandro Molina [email protected] wrote:

For most use cases the json module is able to serialize and deserialize most data people store inside their sessions.
Would it be a feasible solution to use json by default and provide a configuration option to change serialization backend until it supports the loads/dumps functions?

—
Reply to this email directly or view it on GitHub.

[robvdl]

Any reason why the above patch #56 is not applied yet? We just had a security review of a Pyramid app and the cookie pickle article was mentioned in the review. I kind of was hoping Beaker had resolved this issue by now.

[bbangert]

If you're already using pyramid, I'd highly suggest using its cookie session facility. I haven't really had time that I've wanted to spend going through Beaker issues lately which is the main reason it hasn't been applied yet. The session is always signed which reduces the odd's that someone is going to fabricate a payload that will be unpickled (they'd have to know your secret key to sign it properly). If however they were able to do that, it would definitely be bad that remote code could be run as a result.

I will see if I am able to spend some time on Beaker this upcoming week.

[robvdl]

Just as a quick update, I ended up manually applying this patch #56 and used json as session serialisation method, in hope it would have been ok, but after a second security review of our code they were still not happy because in the deserialize() method of that patch it tries to use pickle first and then falls back to json if the depickle fails, therefore not fixing the security issue.

I really don't want to have to keep a fork beaker for a little thing like this, and to be honest I don't think this is even necessary, and as big of a deal as they make it out to be, that beaker is using pickle for session serialisation.

I am going to try to argue that this whole thing is somewhat pointless because it requires the secret key to be compromised and if that happens, you got much bigger problems, but also I am really hoping that beaker can get support for json as a serializer in the future.

[bbangert]

@robvdl I sort of agree, however, there has been a decent amount of arbitrary directory traversal bugs in various pieces of software. So it doesn't seem too far-fetched using one of those, to pick out a secret key, and then using this bug, you can do remote code execution.

I don't know how likely such a thing is, but it definitely means that a directory traversal bug could quickly become a remote code execution issue.

[robvdl]

I have been looking at patch #56 and in the deserialize() method it already gets a "method" argument, which can be checked, instead of trying pickle first and THEN if that fails using json

[robvdl]

I've created an updated pull request for this, #65, which is based on #56 but does not try to use pickle first in the deserialize function and fall back to json if it fails.

Instead it makes use of the 'method' argument of the deserialize function.

[robvdl]

Looks like my pull request #65 got merged, that is nice, and means this ticked can be closed.

[amol-]

Yep, merged it and tweaked it to cope with the recent changes to support python3.
Sorry if I messed up during the merge conflicts resolution and it ended up being a plain commit instead of a merge :)