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

CookieIdentityPolicy should raise a warning when a private cookie has been tampered with #734

Closed
ghost opened this issue Mar 26, 2019 · 4 comments
Labels
C-improvement Category: an improvement to existing functionality

Comments

@ghost
Copy link

ghost commented Mar 26, 2019

Hello
I have a question about the identity middleware and CookieIdentityPolicy.

let's say I manage my session cookies with

CookieIdentityPolicy::new(private_key).name("my_cookie_name")

The way it's managed, session cookies are signed and encrypted with private_key, so that attackers and clients can't see/modify them without the server knowing.

CookieIdentityPolicy retrieves the cookies from the request with

if let Ok(cookies) = req.cookies() {

but only returns None (i.e. user is logged out) if req.cookies() returns an error.

Such errors are returned if:

  1. the cookie isn't valid utf-8 https://github.com/actix/actix-http/blob/c5c7b244be264154ad9b60335a88722b0139206b/src/httpmessage.rs#L119
  2. the cookie doesn't have a name/= https://github.com/alexcrichton/cookie-rs/blob/9edf0ee7037440bf8244722c50804a278524811c/src/parse.rs#L108
  3. the urldecoded value isn't valid utf-8 https://github.com/alexcrichton/cookie-rs/blob/9edf0ee7037440bf8244722c50804a278524811c/src/parse.rs#L82

The last two errors are from Cookie::parse_encoded.

The actual question:
If private_key is leaked, cookies can be altered in such a way that it raises error 1 or 3, and these errors are ignored by CookieIdentityPolicy (which just returns as if the user is logged out).

TLDR: Shouldn't there be a warning if the session cookie is at least correctly signed/encrypted but isn't valid (urlencoded) utf-8? for example

Cookie: my_cookie_name=[random bytes signed and encrypted with `private_key`]
@ghost ghost changed the title CookieIdentityPolicy should generate a warning when a private cookie has been tampered with CookieIdentityPolicy should raise a warning when a private cookie has been tampered with Mar 26, 2019
@fafhrd91
Copy link
Member

this is good point. would you provide PR?

@ghost
Copy link
Author

ghost commented Mar 26, 2019

Sure, though it doesn't seem that cookie-rs has support for cookies that are not utf-8

@DoumanAsh DoumanAsh added the C-improvement Category: an improvement to existing functionality label Mar 27, 2019
@ghost
Copy link
Author

ghost commented Mar 30, 2019

You forked cookie-rs, I can patch the fork and solve the breaking changes in actix-web?

@fafhrd91
Copy link
Member

sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-improvement Category: an improvement to existing functionality
Projects
None yet
Development

No branches or pull requests

2 participants