Skip to content
This repository was archived by the owner on Dec 13, 2018. It is now read-only.

Auth cookies should not be compressed by default #79

Closed
GrabYourPitchforks opened this issue Oct 17, 2014 · 13 comments
Closed

Auth cookies should not be compressed by default #79

GrabYourPitchforks opened this issue Oct 17, 2014 · 13 comments
Assignees
Milestone

Comments

@GrabYourPitchforks
Copy link
Contributor

Compression allows for chosen plaintext attacks against the cryptosystem. See http://www.iacr.org/cryptodb/archive/2002/FSE/3091/3091.pdf for an example.

@Tratcher
Copy link
Member

Sad. Then we should make it easier to set up session-mode cookies. This should be much easier now with IDistributedCache & IMemoryCache.

@kevinchalet
Copy link
Contributor

Note that cookies are not the only target of this kind of attack, and that the same considerations can be applied to the different tokens produced or received by the OAuth middleware, given that both the authorization server and the bearer middleware use TicketDataFormat and TicketSerializer (which uses GZip internally) to compress/encrypt and decrypt/decompress access tokens by default. Unlike TicketSerializer, PropertiesSerializer (which is heavily used by the OAuth2 clients to store some state stuff during an authorization flow) doesn't use compression and shouldn't be directly impacted. That said, I imagine that the BinaryWriter (used in both classes) could also leak some statistical information that could be used in such an attack.

IMHO, removing compression is not an option, given that it will make non-reference cookies totally unusable in most cases, the 4 KB limit being quickly reached without any kind of compression.
Same remark for the session/reference-mode, that will make the cookies middleware far harder to use if you need to set up an external store like Redis (an in-memory store is definitely not something you want in production due to its volatile nature).

Now, I imagine that compressing the data after they have been encrypted might reduce the theoretical impact of this attack. Out of curiosity, do you know practical cases where this - rather old - attack has been used?

@GrabYourPitchforks
Copy link
Contributor Author

This attack is only really interesting when a single compressed payload contains both attacker-controlled data and data that needs to be kept confidential from the attacker. If a payload doesn't contain both of these, then no worries. You could also compress each component individually and concat them together into a final payload if you're truly concerned about size.

A similar attack was used back in 2012 to break the confidentiality of SSL sessions - see CRIME. Finally, compressing data after encryption is useless, as any good encryption routine should be indistinguishable from a PRF.

If there's concern about cookie size, then what that reality means is that the architecture of how the security middleware generates cookies should be revisited. Compression is just papering over an architectural defect at that point.

@kevinchalet
Copy link
Contributor

If by "attacker-controlled data" you mean data sent by an attacker to an app persisting these values as-is in the authentication ticket (and thus in the cookie), then this is probably a non-issue given that the different security middleware I know don't store as-is data: the cookie middleware itself doesn't store external values and the existing OAuth2 clients only store data retrieved from the authorization server.

Do you have an example of some attacker-controlled data in mind?

@GrabYourPitchforks
Copy link
Contributor Author

Username and email address are the big ones that are commonly round-tripped as-is. If you're thinking claims, you could also imagine full name and similar. I think ReturnURL (which is often attacker-controlled) is also round-tripped during the first half of the flow, but I'd have to examine the code again.

@kevinchalet
Copy link
Contributor

I think ReturnURL (which is often attacker-controlled) is also round-tripped during the first half of the flow, but I'd have to examine the code again.

In the built-in middleware, the RedirectUri is not stored by TicketDataFormat/TicketSerializer but by PropertiesDataFormat/PropertiesSerializer, which doesn't use any compression:

https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Security.OAuth/OAuthAuthenticationHandler.cs#L229
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Security.OAuth/OAuthAuthenticationMiddleware.cs#L71
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Security/DataHandler/Serializer/PropertiesSerializer.cs

Username and email address are the big ones that are commonly round-tripped as-is. If you're thinking claims, you could also imagine full name and similar.

Hum, right, that's annoying.

I gonna try a custom TicketSerializer disabling compression to see what impact it could have on a real world application.

@kevinchalet
Copy link
Contributor

As expected, the results are dramatic:

image

image

And of course, the 4 KB per-domain cookies limit being reached, Safari-based browsers stop working:

image

@brentschmaltz
Copy link
Contributor

@PinpointTownes Compressing after encrypting would offer very little.
The cookie that Katana generates when building the 'app session' for WS-Fed and OIDC is built from the SAML or id_token, which are both passed through the user-agent in the clear.

I didn't read the whole paper, is this a data leakage attack OR is the key vulnerable?

@kevinchalet
Copy link
Contributor

@brentschmaltz indeed, the gain is totally marginal.

@Tratcher Tratcher self-assigned this Dec 5, 2014
@Tratcher
Copy link
Member

Tratcher commented Dec 5, 2014

Using the social auth sample, here are my before and after sizes:
Before - After:
Facebook: 469 - 618
Google: 511 - 874
Google Access Token: 426 - 511
Twitter: 426 - 554
Github: 447 - 554
Github Access Token: 383 - 426

Certainly lager, but not as bad as I was expecting. Of course WsFed & ODIC are still going to be in trouble.

@Tratcher Tratcher added this to the 1.0.0-beta2 milestone Dec 5, 2014
@lodejard
Copy link

lodejard commented Dec 7, 2014

This reminds me of a related question that came up - the claim names we're using are based on the uri values. Several folks were suggesting for the built-in types to use more terse names.

@brockallen
Copy link

@Tratcher
Copy link
Member

Tratcher commented Dec 8, 2014

Filed #105 to track the name discussion.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants