cachelib icon indicating copy to clipboard operation
cachelib copied to clipboard

sign cache values

Open Varbin opened this issue 6 years ago • 4 comments

I have a suggestion: It should be possible to sign (/ apply HMAC) to cache values in the same way werkzeug.contrib.securecookie does already.

pickle is used as serializer to serialize the content. While this is absolutely fine as long nobody can access the underlying cache back end (Redis, FS, Memcached), it may allow privilege escalation once an attacker gains access to it, as pickle allows to store arbitrary code.

Proposal:

  1. Add a warning to the documentation.
  2. Add the option pass a signing key to sign the results and raise a warning if no signing key is passed at initialization.
  3. Deprecate not using a signing key and ultimately enforce using one.

Practically pallets' ItsDangerous could be used here. If wanted, I can create a pull request implementing my proposal.

Varbin avatar Feb 08 '19 19:02 Varbin

Hey @Varbin, I would be happy to review a PR for this if you are still interested!

northernSage avatar Aug 07 '21 22:08 northernSage

Sure! I think #11 should be solved first, to not have accidentally conflicting code.

Anyway, my proposed change will be like:

  • Add itsdangerous~=2.0.0 as installation requirement.
  • Add key kwarg to the class constructors
  • Add _load and _dumps methods to the BaseCache class, which will replace calls to pickle.loads / pickle.dumps - if a key is set this will call the itsdangerous instance, otherwise plain pickle.
  • Add a deprecation warning to calls without a key.

The only class that will not support the key argument will be memcached, as the memcache client libraries already do serialization.

Varbin avatar Aug 08 '21 12:08 Varbin

Add a deprecation warning to calls without a key.

I'm not sure on that part. IMHO it should be up to the user. Not signing cache data is only an issue when you have an attacker with arbitrary write access to your cache backend - which means you are either already compromised (in that case the secret might no longer be secret anyway) or severely misconfigured stuff (redis bound to a public IP and not blocked by a local firewall and directly exposed to a malicious network/client)

ThiefMaster avatar Aug 08 '21 20:08 ThiefMaster

Sure! I think #11 should be solved first, to not have accidentally conflicting code.

Don't let this hold you back @Varbin. If you feel like getting started, please go ahead. I'll just solve the conflicts when the time comes.

  • Add a deprecation warning to calls without

I agree with @ThiefMaster on that one. I would rather add a very conspicuous note to our (soon to be written) documentation stating that using a key is strongly recommended.

northernSage avatar Aug 09 '21 20:08 northernSage