Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sign cache values #1

Open
Varbin opened this issue Feb 8, 2019 · 4 comments
Open

sign cache values #1

Varbin opened this issue Feb 8, 2019 · 4 comments
Labels
enhancement New feature or request

Comments

@Varbin
Copy link

Varbin commented Feb 8, 2019

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.

@northernSage northernSage added the enhancement New feature or request label May 24, 2021
@davidism davidism changed the title Suggestions: Sign cache values sign cache values Jun 25, 2021
@northernSage
Copy link
Member

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

@Varbin
Copy link
Author

Varbin commented Aug 8, 2021

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.

@ThiefMaster
Copy link

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)

@northernSage
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

3 participants