Make it secure secure by default

[eskimor]

The library should provide more functionality out of the box, trying hard to ensure that library users get a secure oauth handshake by default.

[3noch]

@eskimor Can you explain why the field here is not sufficient to make these flows secure?

https://github.com/obsidiansystems/obelisk-oauth/blob/65bfa447bd8beff8038325f7115d313029aa1a28/common/Obelisk/OAuth/Authorization.hs#L72-L73

[eskimor]

If used correctly, it might. You just need to generate an unguessable value and keep it to compare it for equality to what you get from the auth provider. This is achieved easiest at the frontend - on the backend you are going to need proper session handling to keep track of that value.

[3noch]

IIUC the idea here is to generate an unguessable nonce (UUID, say) and store that in a database table that links back to the session. When the auth provider forwards you back you can use the nonce to decide whose account should be connected to the auth token. To me this seems much easier to do on the backend than on the frontend.

[3noch]

All the frontend needs is the data that having the auth token provides. The frontend doesn't care about the token in and of itself.

[eskimor]

In this case I would include some hashed session data in that nonce, just for good measure. To make it really sure that the initial requester actually had access to that session. Especially the session state should match up - e.g. user has been authenticated?!

One important reason, I did not went with a table in a database was, that we did not use any database on that project - and I should not, that was a client requirement.

[3noch]

Ah so really this issue is about support for serverless/backendless use cases.

[3noch]

I don't think hashed session data adds any security to a random nonce in this case since either way it is opaque and unguessable and only I can verify it by looking at my database. If you were to somehow find my state parameter value, then you could complete the authorization whether or not it included hashed session information.

[eskimor]

I think the important part is "state", that's probably why this parameter is called that way. It should not be possible to inject an authentication call in a not yet authorized session, which is then later, due to the callback handled in an authorized context, for example.

The standard mentions something like this, I also found a document with attack scenarios, but I forgot the details.

[eskimor]

Ah so really this issue is about support for serverless/backendless use cases.

It has been a while, but that certainly was an important driver for the implementation.

[3noch]

Everything I've read so far on how to handle the state parameter simply says "Generate a random string..." You should, of course, only generate a random string and store it if the user was authorized to initiate the auth flow. So the nonce is basically a "one-time use password to authorize this specific action but expires in 1 hour."

However, I can imagine a client-only implementation being a bit trickier!

[eskimor]

I was referring to this:

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

In particular:

The client MUST implement CSRF protection for its redirection URI.
   This is typically accomplished by requiring any request sent to the
   redirection URI endpoint to include a value that binds the request to
   the user-agent's authenticated state (e.g., a hash of the session
   cookie used to authenticate the user-agent).  The client SHOULD
   utilize the "state" request parameter to deliver this value to the
   authorization server when making an authorization request.

it could easily be that this does not apply to typical rhyolite applications. On the other hand not all our client projects are typical rhyolite applications, so even if that's the case, we should make sure that the library can't easily be used in an insecure way.

[eskimor]

In general, with regard to CSRF, it feels one needs to be more careful with a backend based solution, than with a frontend based solution.

[3noch]

I think a hash of the session cookie is just an e.g. (example) of one solution. Everything I've read says generate random data.

It seems like beating a dead horse, but I think understanding each other well is important here for both of our understandings of the security model. I'm quite surprised that you think backend would be harder. With a backend-only solution an authenticated user says "I want to connect with X via OAuth", the backend then generates a random string, saves it, and remembers that it was associated with this particular user and request. The backend then forwards the user to the auth provider (with the associated state parameter). The auth provider then redirects back to a backend route (that has no JavaScript at all). That backend route can verify the state, and use it to know which user to associate with the token. What do you see as a danger in this flow?

[eskimor]

Yes, if only authenticated users can say "I want to connect with X via OAuth", then it should fine. Then it remains to ensure that the data is truly random and not too short, but that's a different story.

The reason, why a frontend based solution seems more secure in this regard, is the following:

If the frontend does the handshake - we only have one session, only one state parameter.

If the backend does the handshake - and we use the state to actually lookup the session: If that state gets intercepted (it is a query parameter after all), one could re-direct to another session and thus grant access to a different account.

Detailed attack scenario:

1. User gets forwarded to github, with state query parameter.
2. User gets forwarded back to page, by github - with state query parameter.
3. Malicious action: That forward gets intercepted and the state parameter replaced with that of a different handshake.
4. The backend looks up a different user session and completes the oauth handshake with that one, connecting the user's github account to a malicious user account.

This scenario would not be mitigated with some session token hash unfortunately either, it's just an example why using the state to actually look up the session feels less secure.

The same would not work in a frontend based handshake as there, the lookup of the wrong state would simply fail and the handshake would be rejected.

In particular, if we use the state parameter to lookup the session - it is less of an additional security layer, but just a necessity to complete the handshake at all.

[3noch]

So the attacker would need to be a different user on your system which has an in-flight (not yet completed) authentication flow (in order to get his own state value). And then he must be able to MITM another user in order to swap out the state parameters?

[eskimor]

Yeah, basically - not the easiest attack vector ;-) But I am also not great at exploiting vulnerabilities - it just "feels" a little less secure: Like we removed one layer.

Another scenario, that just came to mind:

1. User wants to authenticate to github
2. Attacker unplugs cable right before the data arrived there - or distorts the wifi
3. User hits monitor and gets a coffee
4. Attacker approaches computer and copies state value from the url bar.
5. Attacker has a "registered" state value and could use it to authorize that user account with his github for example.

[eskimor]

Because the url might even be in the history of the browser, the attacker could use that even if the user logged himself out in the meantime.

[3noch]

3. User hits monitor and gets a coffee
4. Attacker approaches computer and copies state value from the url bar.

:joy: I'm pretty sure this counts as "physical access" in which case all security guarantees are off.

This one-time-use nonce should expire after a pretty short time (30 mins or less I'd imagine) so this guy would also have to act very quickly.

[3noch]

even if the user logged himself out in the meantime

I suppose could see a case for refusing to use nonces for which there is no actively logged in user, or some similar mechanic.

[3noch]

Thank you for explaining. I do see now why a frontend only solution could possibly be more secure. However, I'm not convinced that less secure (for some unknown metric of "less") is the same as "not secure." I'm inclined to change the name of this ticket to: "Support frontend-only flows for additional security." What do you think?

[eskimor]

The issue was more about the handling of the "state" parameter should be handled by this library, otherwise we will have applications that just set it to "foobar", which is definitely problematic from a security point of view.

In addition, but that's a weaker requirement, I think we should not be using the state parameter to lookup a session/account - but I would need to think about that a bit harder: In particular, what to do instead? Put information in the path? Why would this be more secure? Alternatively/additionally: Dig up some trustworthy reference implementation and see how they are doing things.

But most importantly: All the security sensitive parts should be handled by this library and not by user code.

[3noch]

While I'm skeptical that there is something that is much better than using state to link to session data, I still think the title of this issue is misleading. This library is not insecure. It's just doesn't hide security issues from you automatically. Those are two very different things. One is a question of scope. One is a serious bug.

[3noch]

Notably:

Current master uses a deterministic guessable "state" parameter for the oauth handshake, which is bad for security

This isn't actually true. Current master uses a guessable state by default. It's up to you to actually use the state correctly.

[eskimor]

Notably:

Current master uses a deterministic guessable "state" parameter for the oauth handshake, which is bad for security

This isn't actually true. Current master uses a guessable state by default. It's up to you to actually use the state correctly.

That's actually true - the precise reason for this ticket was code that never made it in this library. When I started working on this, it was not yet factored out and I worked from that code - which included the insecure parts. (Or it got ripped out later, in any case - you are right, the code I was referring to does not exist in this library at this point) So let's change that. :-)

[eskimor]

Ok, now I am getting the threat model:

http://www.thread-safe.com/2014/05/the-correct-use-of-state-parameter-in.html https://tools.ietf.org/html/draft-bradley-oauth-jwt-encoded-state-00

So using it for looking up the session seems to be totally fine. Things are starting to make sense :-)

I think the explanation in the original rfc could be better.

[3noch]

When I started working on this, it was not yet factored out and I worked from that code - which included the insecure parts

Aha! That explains a lot! Thanks for clarifying. I was quite confused at first. :joy:

[eskimor]

Sorry about that.