negroni-oauth2 is unsafe and vulnerable to csrf
jtolio opened this issue · 8 comments
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
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
check out https://github.com/jtolds/go-oauth2http
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.
Awesome, thanks, I will
Any progress on fixing this?
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
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.