A handful of small security-related tweaks. #5
Conversation
Previously, this was susceptible to replay attacks where the cookie is still trusted even if the user has been de-authorized on the Google side. This means that the only time we'd actually re-auth with Google would be if underpants were restarted. This adds a timestamp to the authenticated cookie payload and ensures that the timestamp is not too old, which forces re-auth through Google once the cookie has matured.
The `map` lookup is potentially susceptible to timing attacks which could allow discovery of a valid value for the auth cookie. Since validating+decoding the cookie is quite fast, it's probably safest just to remove the cache.
This fixes an issue where some sizes of cookie payloads were being truncated.
| now := time.Now() | ||
| age := now.Sub(u.LastAuthenticated) | ||
| if age.Seconds() >= authMaxAge { | ||
| return nil, errors.New(fmt.Sprintf("Cookie too old for: %s", u.Email)) |
This adds a new configurable in the JSON config file. When enabled, a
few headers which can improve application security are added to all
proxied and self-generated HTTP responses. The headers are:
X-Frame-Options: SAMEORIGIN
Cache-Control: private, no-cache
Pragma: no-cache
Strict-Transport-Security: max-age=16070400; includeSubDomains
The latter is only added if TLS certs are configured.
If not configured in the JSON, the old behaviour (not adding headers)
will be maintained.
|
|
||
| // An LRU cache that maps raw cookie strings to full user object. This avoids having | ||
| // to authenticate and decode user cookies on each request. | ||
| cache *lilcache.Cache |
There was a problem hiding this comment.
I'm fine taking this out because I think you are right that it is no longer needed. (Thanks for including the benchmark, also). Given lock contention in that cache, I was curious it was really even giving us benefit at this point. That said, I'm a little skeptical of the timing attack vector, would love to hear more. My understanding of golang maps is that the write is dominated by the lnv hash, which should not early-out. It also seems really hard to exert enough control to get side channel information from timing on bucket collisions .. and certainly collisions would tell you very little unless you know an awful lot about all the data in the map.
There was a problem hiding this comment.
TBH, I haven't attempted a timing attack on it, but @JordanMilne pointed me to https://golang.org/src/runtime/hashmap_fast.go#L208 which does look somewhat suspicious.
There was a problem hiding this comment.
Honestly, I don't think it would be a viable remote attack vector (although there have been some experiments with promising results when attacking from the same colo) and I didn't spend any time verifying that one existed.
From my reading of mapaccess1_faststr() it will take longer to return if there is a string with the same prefix in the bucket. I don't know enough about golang internals to know how many buckets a map with as few elements as this one would have, but it just seemed like another good reason to take the cache out if it wasn't giving any speed benefit.
|
Ready to merge? |
|
Ready! |
A handful of small security-related tweaks.
Hi there, this is a few little security-related things. Explanation for each should be in the commit messages. Thanks for underpants, very clean and enjoyable code!