Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Shouldn't CookieComponent::read expect encrypted data as default? #2392

Closed
hakito opened this Issue · 4 comments

2 participants

@hakito

I think the current implementation of read has a slight security flaw. Guess the following situation:

I set a encrypted cookie for a user to grant him backend access. The encrypted data is just a timestamp in the future for the time the access is granted. If the user would know what kind of data has to be inside the cookie he just has to set an unencrypted great integer and he can access the backend forever. If the user get's an appropriate error message indicating the content it shouldn't be too difficult to try some cookie values.

The simple solution for this problem would be to handle read the same way as write and use a parameter for encryption which is true per default.

@markstory
Owner

While I think this is a good idea, I don't think this is going to be possible with the current cookie component. Only accepting encrypted content will probably change the interface a bit, as you'd need to indicate whether you want to read encrypted/decrypted data in the call to read().

I think we'll need to address this in a future backwards incompatible release. In the mean time you might want to make your timestamp method less susceptible to these kinds of issues. Perhaps store the time out in the session content instead?

@hakito

I agree that changing this behavior would break existing cake instances. That's the reason why i just asked this issue as a question instead of opening a pull request.

One thing i could imagine as a non breaking change is to add a second parameter "encrypted" (true/false, default: null) to give at least the possibility to force decryption/plain variant. If set to null the method could trigger a deprecation warning or something but act as usual.

If you really target this change for 3.0 this is probably a good solution to help developers being ready for migration.

@markstory
Owner

One problem with changing it in 2.x is that the CookieComponent eagerly reads all cookies and decrypts everything it can on the first read/write/delete. This happens primarily so that updates that add keys to multi-dimensional cookie values work correctly.

For 3.0, I'm considering making cookie component able to manage more than one top level cookie and now also always/never encrypt based on configuration. Mixed modes are confusing to both people and computers.

@hakito

Ok, thanks!

@hakito hakito closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.