actix / actix-web

Actix Web is a powerful, pragmatic, and extremely fast web framework for Rust.
https://actix.rs
Apache License 2.0
21.6k stars 1.67k forks source link

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

Closed ghost closed 5 years ago

ghost commented 5 years ago

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 https://github.com/actix/actix-web/blob/86a21c956c247ab098a7631bc18ae60032ef5eac/src/middleware/identity.rs#L352 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`]
fafhrd91 commented 5 years ago

this is good point. would you provide PR?

ghost commented 5 years ago

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

ghost commented 5 years ago

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

fafhrd91 commented 5 years ago

sure