add 'state' parameter to OAuth authorize request (prevent CSRF)

[ghost]

   state
         RECOMMENDED.  An opaque value used by the client to maintain
         state between the request and callback.  The authorization
         server includes this value when redirecting the user-agent back
         to the client.  The parameter SHOULD be used for preventing
         cross-site request forgery as described in Section 10.12.

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

Actually, the specification is TOO WEAK here, it is actually a MUST to prevent CSRF:

http://homakov.blogspot.nl/2012/08/saferweb-oauth2a-or-lets-just-fix-it.html

[untitaker]

We've discussed this in IRC:

• In the case of native apps, CSRF is prevented through much more effective means (you can't csrf a webview)
• In the case of remotestorage.js, there's no easy way to exploit this. The article presumes an XSS attack, but if you have XSS in an entirely client-side app, you can do whatever you want already.
• remotestorage.js has a non-security bug: You can troll people by sending them to https://app.com/#access_token=lolol to log them out.

[ghost]

In the case of native apps, CSRF is prevented through much more effective means (you can't csrf a webview)

A webview cannot always be used, and is also a bad idea for security. One does not know where the password is being typed. I would never do that for example. Using a webview to ask for a password is a bad idea. Don't do that.

In the case of remotestorage.js, there's no easy way to exploit this. The article presumes an XSS attack, but if you have XSS in an entirely client-side app, you can do whatever you want already.

This is not true, ANY website on the Internet could trigger this, not necessarily your app. Even an advertisement network ;-)

remotestorage.js has a non-security bug: You can troll people by sending them to https://app.com/#access_token=lolol to log them out.

Well, I think in the particular case of RS we are lucky as some additional parameters need to align for it to succeed. There are situations where this can be a bigger problem: target user used the same storage server, and all users share the same storage root. If a service does not provide public files there is no need to have the userid in the URL.

Too sum up: making the RS library sent a state parameter with a nonce is a really good idea from a security perspective (linking request to response). That with luck it cannot currently be exploited (as far as I know) does not mean we should not implement it.

[raucao]

A webview cannot always be used, and is also a bad idea for security. One does not know where the password is being typed. I would never do that for example. Using a webview to ask for a password is a bad idea. Don't do that.

We do exactly this in rs.js on Cordova. If your argument was "you cannot see where your password is typed", then the answer is that we actually show the address bar for the in-app-browser.

This is not true, ANY website on the Internet could trigger this, not necessarily your app. Even an advertisement network ;-)

How? If it's such a big problem, it should be easy to write down a specific attack example that applies to this library and servers conforming to the spec.

If a service does not provide public files there is no need to have the userid in the URL.

Then that service doesn't comply with the remoteStorage spec.

Too sum up: making the RS library sent a state parameter with a nonce is a really good idea from a security perspective (linking request to response).

A good idea in your opinion. I personally am always glad to be convinced of new ideas, but so far it looks like a rather superficial addition to me.

[untitaker]

@fkooman: A webview cannot always be used, and is also a bad idea for security. One does not know where the password is being typed. I would never do that for example. Using a webview to ask for a password is a bad idea. Don't do that.

Well, under Android all apps are doing it. IMO it solves more security problems than it creates.

@skddc: we actually show the address bar for the in-app-browser

I think @fkooman is talking about malicious apps that try to snitch your password (which would give them the possibility to e.g. access more than the scope they requested)

---

@fkooman: This is not true, ANY website on the Internet could trigger this, not necessarily your app. Even an advertisement network ;-)

That depends on what exactly you're trying to do. Steal or inject tokens?

• Steal tokens: XSS is absolutely necessary. Clients are identified by redirect URI (on sensible servers), so you need to hack the redirect URI to get the token. For servers that don't identify by redirect URI, you can execute at least as bad attacks, at that point requiring a state or not becomes irrelevant.
• Inject tokens: You would have to inject the storage root URL as well, at this point the attack becomes client-specific.

[ghost]

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

The client MUST implement CSRF protection for its redirection URI.

I guess having a particular storage server configured is sufficient for CSRF protection...

However:

The binding value used for CSRF
   protection MUST contain a non-guessable value (as described in
   Section 10.10),

That would rule out the storage URL. Luckily it is not just my opinion.

[untitaker]

The only thing you can inject via the redirect URI is the access token. And remoteStorage.js should absolutely implement CSRF protection for that, it currently doesn't. But the text you're quoting assumes that protection happens by passing around an additional value.

remoteStorage.js might as well set a simple flag in localstorage that indicates whether it currently does OAuth or not. It can then use that flag to determine whether to use the fragment's token or to discard it.

This is almost equally secure. I'm not sure it is completely equal to sending a state param. For example: During a normal OAuth flow, an attacker could make the user visit the redirect URI. But I am not sure how such an attack would be possible.

[untitaker]

But in any case, whether remoteStorage.js uses a simple flag in localstorage or the state param, none of this is security relevant.

[ghost]

Let's just implement the state parameter in remotestorage.js. Something that is basically a MUST, or even if it was just a SHOULD, can't be ignored without a very good reason. Hand waving and statements like "This is almost equally secure." are not really giving me any confidence :)

Also, it is super easy to generate a random (hex) number in the browser, bind it to a SPA routing URL and store that in localStorage or IndexDB.

Also note, that not all values are allowed in a state parameter. I ran into some issues with taskrs where an invalid state parameter was sent. The syntax for state is:

     state      = 1*VSCHAR
     VSCHAR     = %x20-7E

So an empty state is not allowed, nor are characters outside the range shown above.

[raucao]

You can hardly convince other people by repeating "let's just do it" often enough. You still haven't put forward a clear attack case that is realistic with rs.js and servers that conform to the spec.

We need to prevent injection anyway (if it's actually possible right now), and I agree with @untitaker that that would have the side effect of basically replacing your proposal in the same step.

[untitaker]

After sleeping over this, I'm on neither side anymore. I don't see how a state parameter would be vastly superior (security-wise) to a simple flag, on the other side I don't see much difference in complexity in either implementation of CSRF-protection.

[ghost]

Actually, it is the other way around. Not implementing it requires justification. Following the specification, best practices and security recommendations does not. I am actually surprised there is so much resistance to it.

In my server I can't do anything else than warn the users the no state parameter is used by the RS client and tell them that they are probably secure in the case of remotestorage.js, but I don't know enough about remotestorage.js to feel confident...

[untitaker]

https://unterwaditzer.net/taskrs/#remotestorage=untitaker@5apps.com&token=HAHAHA

I think I agree with @fkooman now.

[raucao]

Why? Same situation as before.

Actually, it is the other way around. Not implementing it requires justification. Following the specification, best practices and security recommendations does not. I am actually surprised there is so much resistance to it.

It's not a MUST, so we are following the specification. No? Also, there's no "resistance" per se, but you keep asking for it without an argument other than your feelings.

[untitaker]

Argh. Yeah. I forgot that remoteStorage.js actually clears its localstorage between logins.

[ghost]

My feelings? I suggest following security recommendations. Well, I guess you can classify that that is me feeling like it is a good idea to follow those ;)

Based on @untitaker's last comment I guess we have a working CSRF attack now?

[untitaker]

Based on @untitaker's last comment I guess we have a working CSRF attack now?

It's not that easy. Yes, you can point people's apps to arbitrary storages, but in the case of 5apps, this "attack" is already used to launch apps from their dashboard, and automatically log them into their storage. It's not a bug, it's a feature.

Since remoteStorage.js clears its localstorage, there's no data theft possible. The spec actually mentions this attack:

A malicious party could link to an application, but specifying a
remoteStorage account address that it controls, thus tricking the
user into using a trusted application to send sensitive data to the
wrong remoteStorage server. To mitigate this, applications SHOULD
clearly display to which remoteStorage server they are sending the
user's data.

[raucao]

Yup, exactly.

My feelings? I suggest following security recommendations. Well, I guess you can classify that that is me feeling like it is a good idea to follow those ;)

Yes, you repeatedly put forward your "feeling more confident" as the only argument in this thread. I mean no offense; these are your own words, and there are no other clear arguments so far that I personally can parse out.

[raucao]

So, in regards to the storage-first auth, I think the new widget should definitely follow the spec in that regard and clearly show that it just connected to sth, and to what as who!

[untitaker]

@skddc François does have a point. The fact that remoteStorage.js allows sending people to arbitrary storages is both a usability feature and a potential security issue, as mentioned in the spec. I think this disagreement (over whether this feature is worth it) is what this issue boils down to.

[untitaker]

It could even prompt whether the user is fine with that. It probably should try a sync with the old credentials and data to avoid data loss.

[raucao]

Ironically, this feature was added to the spec for something that @fkooman was working on at the time, and I personally was opposed to it. History is funny like that. :D

[raucao]

It could even prompt whether the user is fine with that. It probably should try a sync with the old credentials and data to avoid data loss.

Prompt is a great idea!

Edit: in fact, that's a fantastic idea. Let's do that from the start in the new widget.

[untitaker]

I wonder whether the token and remotestorage "fragment-query args" should be mutually exclusive.

• If token is there, it's an OAuth flow
• If remotestorage is there, it's storage-first auth (and the app could init a normal OAuth flow)

[ghost]

Ah yeah. Storage first. I was experimenting with that some years ago. The "flow" I envisioned was not that a storage server provides applications with a token! The application would be provided with a storage root and authorization endpoint. That way this particular application could obtain its own token using the normal OAuth flow. My "storage first" server would pre-approve the application so the user did not need to confirm. I guess it just got implemented in a misunderstood way (read: I was probably not clear enough in the specification).

The important part of "storage first" was to make RS work without "WebFinger", not to short-circuit the OAuth flow :)

[untitaker]

I'm not sure if there's actually any value to that though. Malicious servers may still just auto-approve instead of showing an OAuth dialog, and you'd have the same user experience with more roundtrips.

[ghost]

Not if you use the state parameter...

[untitaker]

@fkooman Sure they can. You send the user to the OAuth server with the state parameter, and the malicious OAuth server sends the client to the redirect URI, with the correct state param attached.

[raucao]

It looks like we didn't come to consensus here. From what I understand adding the parameter doesn't add a meaningful benefit for our case, right?

[untitaker]

Storage-first auth flow is in the spec, so just implementing a state param is not sufficient.

It seems the correct approach is:

• If token param is given:
  • Require state param to be set, otherwise ignore the entire fragment. Oh yeah, and also validate it (somehow), of course.
  • Require remotestorage param not to be set, or at least ignore it.
• If remotestorage param is given:
  • Require token not to be set, or at least ignore it.
  • If logged in with different account, log out.
  • Initiate OAuth flow with account from the param.

[ghost]

To improve the situation:

1. make very clear to the user at all times to which storage server they are connected, possibly in the RS widget top right corner;
2. do not "just" redirect the browser to any storage server to obtain an access token, but first inform the user if the storage URI was provided as a query parameter (storage first)

I would suggest to ALWAYS use state. It is a SHOULD (or if you read between the lines actually a MUST) in the specification/security documentation to prevent CSRF). So while omitting it in RS may actually be okay if 1 above is implemented, I would not risk it. It is not that expensive to implement.

[untitaker]

I would suggest to ALWAYS use state.

I'm not sure if you really mean always -- in the case of storage-first auth, it's not useful to send the user to https://example.com#remotestorage=...&state=....

[raucao]

Ok, sounds good. I guess that means we need to change the spec in regard to storage-first auth, right? Because right now we do send a token and don't do an additional OAuth roundtrip.

[untitaker]

Yes, probably:

When the user gestures they want to use a certain application whose
manifest is present on the dashboard, the dashboard SHOULD redirect
to the application or open it in a new window. To mimic coming back
from the OAuth dialog, it MAY add 'access_token' and 'scope'
fields to the URL fragment.

[raucao]

Upon reading that excerpt, I just realized how that is also an issue with old tokens, when the app changed scopes in the meantime.

[untitaker]

I will open a spec issue for summarization.

[untitaker]

https://github.com/remotestorage/spec/issues/144