search

GoIncremental / negroni-oauth2

Apache License 2.0
71 stars 25 forks source link

negroni-oauth2 is unsafe and vulnerable to csrf #2

Closed jtolio closed 9 years ago

jtolio commented 10 years ago

the oauth2 "state" field, the first argument of AuthCodeURL, is supposed to be a CSRF token - a completely unguessable random string of bytes. further, on the callback, the oauth2 service will return the provided state and negroni-oauth2 should be checking it for equality

it's certainly clever that the next url is being passed in as the state field, but it's insecure. both the expected state and the next url should be kept in the session

jtolio commented 10 years ago

http://tools.ietf.org/html/rfc6749#section-10.12

jtolio commented 10 years ago

it may be safe to include additional information inside the state field besides a csrf token (e.g. the next url field), but any benefit from that is possibly negated by having to store the expected csrf token somewhere

jtolio commented 10 years ago

check out https://github.com/jtolds/go-oauth2http

Bochenski commented 10 years ago

Thanks for letting me know, most of this was derived from martini-contrib/oauth2 so might be worth letting them know also if it's a security risk.

jtolio commented 10 years ago

Awesome, thanks, I will

flexd commented 9 years ago

Any progress on fixing this?

jtolio commented 9 years ago

i'm not sure this is the right place to point this out, but the main thing negroni offers is a different http.Handler interface for chaining handlers together, which you really don't need. you can wrap handlers together and compose them much more simply without negroni at all.

the oauth2 library i linked to above has this security issue fixed, and doesn't require negroni

Bochenski commented 9 years ago

So I've updated the code to pass 16 random bytes, hex encoded instead of the next url, and stored / retrieved the next url inside the session instead. Thanks again for the heads up on this.