Seems to be vulnerable to the pickle security issue (remote code execution)

[robvdl]

I am using Pyramid and Beaker at the moment for sessions, but Beaker is currently also vulnerable to the pickle issue found in so many Python frameworks:

http://www.balda.ch/posts/2013/Jun/23/python-web-frameworks-pickle/

Django has fixed it now, but Beaker and Pyramid's built-in session haven't

I was hoping that pyramid_redis_sessions would not be vulnerable to this security vulverability, but a quick grep of the code shows, that it too is using pickle instead of json for serialisation.

If this is an issue, can we please get this fixed by using JSON for serialisation, not pickle

[robvdl]

Recently we had this issue come up in a security review of one of our applications, and I had to manually patch Beaker, but the patch is yet to be merged.

We also recently had this come up during a talk at Kiwi Pycon 2014:

http://www.youtube.com/watch?v=1x0XS7jkwzs&list=PLBGl1tVyiWQSVwxne3yOH79uaSqgbnCqL#t=1125

[ericrasmussen]

I haven't had time to look in-depth but would like to get a fix out. Can you send me a link showing how you patched it in beaker? Thanks!

[robvdl]

Hi, I am happy to help out get this resolved.

The basic issue is that depickling pickled data has the potential to execute code, though you still need to know the secret key in order to insert things into the session, when combined with another attack, it could potentially be used to execute arbitrary code on the server, inserted by the attacker into the session.

json is usually an ideal format for serializing/deserializing session data because generally you will only be storing strings, ints, floats, lists, tuples, booleans, etc.. and that can easily be converted to json using the built-in json library in python.

There are two versions of this fix and I have a tag on each of them in my fork of beaker on github.

the "pickle fix 1" tag properly patches writing session data, by only using json, but it only "half" patches reading session data by using json first and then falling back to pickle if that didn't work.

After another security review of our app, the security reviewers were still not happy with this patch. and I can understand exactly why, if there is another way (directory traversal bug in an application for example) malicious pickled data can be inserted into the session, it is still open to this issue when reading session data.

so then the "pickle fix 2" tag is a proper full fix, it basically ONLY ever uses json and never falls back to pickle when loading session data.

[robvdl]

Sorry it took me a while to get back, been very busy approaching the end of the year.

Anyway, I checked the docs and it clearly says you can define your own serializers like so:

redis.sessions.serialize = cPickle.dumps
redis.sessions.deserialize = cPickle.loads

That is good, that means you should be able to set them to json.loads and json.dumps already.

Maybe all that needs doing is an update to the docs, show examples using json instead of Pickle and put a quick statement why you shouldn't use Pickle for serializing session data because it can be a security risk.

Also, if it's using Pickle by default if you don't define serializers, it shouldn't really be the default, defaulting to json would be best.

[joulez]

pickle via PickleSerializer is the default in pyramid as well. Because it's possible that third party apps/libs could possibly break changing the hardwired default (there is limits to what json can serialize). The better option would be to at least add a json serializer and perhaps issue deprecation warnings in relation to changes to default in the future. Please see #1525

[mmerickel]

This comes up every once in a while and the answer is always the same. Yes, pickle is insecure. That's why we sign the content. If your secret is compromised then you're screwed, obviously. If your secret is not compromised then I have yet to see an attack against this technique. This is what Pyramid does by default, and beaker for years before it and I've never heard of an exploit.

[robvdl]

I agree, but I think it only becomes a problem when combined with another attack (such as a directory traversal bug in a different library), as a security review company have told me in the past. It's highly unlikely I agree, but then it's not entirely impossible either they tell me. I am not sure how convinced I am about this myself, but I see their point.

Personally I don't see much use in storing anything other than dicts, lists, int, bool, and string types in the session and not complex objects, so for most cases a JSON serializer is going to be just fine, but I understand that other people may already be storing objects in their session.

[redspider]

Not expressing an opinion on solutions, however it seemed important to note the following:

1. The pickling/unpickle of storage within redis itself is probably not an issue, the ability for an attacker to modify the blob inside redis is predicated on their ability to access redis as well as know the secret key. At this point I think it's reasonable to say the application is well and truly compromised, typically applications are not built to defend themselves from a compromise of their backend data storage.
2. However, the key for the particular redis blob that is to be decoded is stored in the public facing cookie. This is done using pickle as well, via pyramid.session/signed_*serialize which makes use of a sha1 hmac for signing.
3. There is no way for an application to modify the behaviour of signed_*serialize other than monkeypatching, nor is it possible to instruct redis_sessions to use a different serialization option, so typical app developers cannot mitigate this by switching serialization methods.

A proposal to replace the hardcoded serialization exists here: #38 - webob.SignedCookieProfile uses sha512 and json serialization by default.

[jgelens]

@ericrasmussen

I guess using json will work fine.
I've tried:
redis.sessions.serialize = json.dumps
redis.sessions.deserialize = json.loads

But the code expects json to be imported. How should this be used?

[ericrasmussen]

@jgelens pyramid_redis_sessions uses pyramid.config's maybe_dotted function (http://docs.pylonsproject.org/docs/pyramid/en/latest/api/config.html) to resolve the import. I tested those settings as you have them and they worked fine. If they didn't work for you, can you open a separate issue with a reproducible example? Thanks!

[ericrasmussen]

FYI, I'm closing this issue for now because pickle support is required by the pyramid ISession interface (http://docs.pylonsproject.org/projects/pyramid/en/latest/api/interfaces.html#pyramid.interfaces.ISession), and because users can choose other serializers if needed.

Thanks everyone for the discussion and feedback -- I agree with the concerns/issues and I'm all for getting people to use a safer form of data serialization, but I also can't break pyramid support (the whole reason this library exists!) for the default.

[jvanasco]

This issue has been addressed in the 1.5.1 version of my fork, https://github.com/jvanasco/pyramid_session_redis

The problematic call to pickle has nothing to do with ISession. Serializing the data with pickle is absolutely safe in this context of server-side sessions. Changing the serializer to JSON as suggested above will not address this issue.

The potential vulnerability in this library is from signing/reading the cookie value via pyramid.sessions deprecated signed_serialize and signed_deserialize functions. Those functions call pickle.loads on the user-submitted cookie content (if the hmac secret is broken). The value of this payload is just the backend session_id.

The potential fixes are:

• don't sign the cookie payload, and remove the calls to these functions
• rework the code to do what Pyramid does for signed cookies, and invoke webob's signing functions. this is what I did, with a small difference. Pyramid uses JSON serialization on the cookie value, but since the session_id is an alphanumeric string, I just wrote a null-serializer that mimics the interface and passes the data through.

pyramid_redis_sessions/pyramid_redis_sessions/__init__.py

Line 284 in 6888d9d

cookieval = signed_serialize(session.session_id, secret)

pyramid_redis_sessions/pyramid_redis_sessions/__init__.py

Line 260 in 6888d9d

session_id = signed_deserialize(cookieval, secret)