alexedwards / scs

HTTP Session Management for Go
MIT License
2.13k stars 166 forks source link

Use securecookie #44

Closed dhui closed 6 years ago

dhui commented 6 years ago

Any objection to using securecookie to write and load the session token/key/id from the cookie?

I think it's a good idea to use signed cookies to prevent tampering. e.g. you can avoid unnecessary queries to the session store by throwing away invalid cookies

With you blessing, I'll open a PR to integrate securecookie into scs with the following features:

  1. Backwards compatible with existing valid scs cookies. We could accomplish this with one of the following approaches:
    1. Flag to enable securecookies
      • Won't "upgrade" cookies in-place
    2. Pack/encode the cookie value differently - Would have to use a non-base64 encoding character to signify differences
      • Will upgrade cookies in-place
      • Could encode the cookie value as a JSON string or just use a simple non-base64 character
    3. Migrate to another cookie name
      • Will upgrade cookies in-place
      • Secure cookie name would be configurable e.g. add secureName field to options struct
      • I prefer this approach since it's a cleaner design and we can use the configurable string enable/disable securecookies e.g. empty string disables secure cookies
  2. Unit-tested
alexedwards commented 6 years ago

Thanks for the thoughtful issue. I think there's two aspects to this: security and performance.

Security

The session token is currently made by base64-encoding 32 bytes generated by rand.Read(). Sitcking the token in securecookie would indeed make it more secure because the attacker would need to brute force a valid session token and then the correct signature for it. But mathematically we could achieve the same end result by simply increasing the length of the session token... and that wouldn't have any backward compatibility problems. As it stands the entropy for the session token well exceeds the minimum suggested by the OWASP guidelines, so I don't think this is necessary for the time being.

Performance

On top of the backward-compatiblity issues you mention, we would also need to change the API to allow the user to specify a 'secret' for signing the cookies and a mechanism for rotating their secrets.

All in all, it would add quite a bit of additional complexity to SCS. I don't mind doing that if there's a clear benefit, but at the moment I'm not seeing that!

dhui commented 6 years ago

Security

I'm not convinced that increasing the length of the session provides the same protection as a signature. The more active sessions your service has, the higher likelihood a random guess would hit a valid session. The same is not true for a signature. e.g. more signed sessions doesn't increase the likelihood of forged session

Perfomance

Latency

My understanding is that all sessions backed using securecookie will incur an increase in latency since the HMAC needs to be validated every time. But for encrypted cookies, I think if the signature is invalid, the payload is not decrypted, which will result in the variable latency you mentioned.

Bruteforcing

  1. Brute-forcing the signature key which would be done offline. The main protection we have against this is to use a large key with a secure HMAC. By default, securecookie uses SHA-256 which is considered safe for HMACs.
  2. Brute-forcing a signed cookie would also be done offline and if such a brute force attempt were successful, we're protected by short session durations and a safe HMAC.
  3. Brute-forcing a session token (e.g. id) - I think the best protection against these attacks is for the service to implement rate-limiting. Using explicit protection against brute-force attacks is better than implicit protection e.g. relying on performance of the choice of the backing session store. For example, if a service changes their backing session store, you wouldn't want quality of the brute-force protection to change.

Resource-use

Yep, this depends on the scenario and I don't have any data to make any claims. Since I/O is slower than compute, most web apps and services will protect the DB(s) with authentication and rate-limits. Another reason is that it's harder to scale a stateful service (e.g. a DB) than a stateless one (e.g. compute).

On top of the backward-compatiblity issues you mention, we would also need to change the API to allow the user to specify a 'secret' for signing the cookies and a mechanism for rotating their secrets.

Yep. I was planning on adding the following methods to Manager:

All in all, it would add quite a bit of additional complexity to SCS.

Agreed.

alexedwards commented 6 years ago

Thanks, it's good to think all this stuff through.

To avoid any confusion, SCS has two types of session cookies:

I'm not convinced that increasing the length of the session provides the same protection as a signature. The more active sessions your service has, the higher likelihood a random guess would hit a valid session. The same is not true for a signature. e.g. more signed sessions doesn't increase the likelihood of forged session

I think we might be talking at cross-purposes? I'm talking about increasing the length of the session ID for Type 2 cookies. A 512-bit random ID would definitely be more secure than the current 256-bit ID, by many orders of magnitude. I'm not 100% sure on the maths, but I don't think that a 512-bit random ID would be significantly easier to brute force than a 256-bit random ID + HMAC with a 256-bit secret key. Using a 512-bit random ID alone also has the added advantage that no part of it can be brute-forced offline.

The other thing to think about is that the whole point of signing a cookie. If your cookie contains data, like the price of a product or a flag to indicate that a user is logged-in, then it's vital to sign it so you can be confident that the data is valid and hasn't been altered. But for a randomly-generated session ID like we have, that's not needed. If the session ID is altered, it's OK. The probability of the attacker changing it a valid, active, session ID that we recognise is vanishingly small because of the random way that the ID's are generated.

If the session ID wasn't random, and was an auto-incrementing integer or something like that, then I would definitely agree that the cookie needs to be signed.

My understanding is that all sessions backed using securecookie will incur an increase in latency since the HMAC needs to be validated every time. But for encrypted cookies, I think if the signature is invalid, the payload is not decrypted, which will result in the variable latency you mentioned.

I was more talking about reduced latency because the database wouldn't be hit in the event of a tampered cookie. But you're right, rate-limiting is the best way to address this anyway.

dhui commented 6 years ago

Thanks, it's good to think all this stuff through.

Agreed. Thanks for the open discussion!

I think we might be talking at cross-purposes? I'm talking about increasing the length of the session ID for Type 2 cookies.

Yep, I was referring to the "type 2 cookie" as well. Thanks for clarifying!

I'm not 100% sure on the maths, but I don't think that a 512-bit random ID would be significantly easier to brute force than a 256-bit random ID + HMAC with a 256-bit secret key.

I'm not sure about the math either, but I agree that the odds of a randomly generated 256-bit session id/token being guessed are extremely small to the point that it's essentially zero.

At this point, I don't think tamper-resistant cookies for DB-backed cookies are necessary. They offer first-line protection against scanning for session ids in your DB, but rate-limiting would also provide similar protection.

I'll close this issue to keep your issue list clean since it doesn't seem like scs will use tamper-resistant cookies in the foreseeable future. Thanks again for discussing!

alexedwards commented 6 years ago

Thanks Dale :+1: