Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow passing additional state #182

Merged
merged 6 commits into from Feb 27, 2016
Merged

Allow passing additional state #182

merged 6 commits into from Feb 27, 2016

Conversation

@klaronix
Copy link
Contributor

klaronix commented Feb 6, 2016

I'd like to add additional data to the OAuth state parameter. So far bell creates its state parameter by using an arbitrary nonce and there is no way for intercepting or extending the state.

This is useful due to restrictions on OAuth redirect_uri. Each provider has its own rules for the callback uri. E.g. OneDrive only allows urls from within a master domain. This makes development and testing complicated: I need to change allowed redirect uris for this. Also if you like to host multiple instances of an application on different domains, things get complicated - at least, or cannot be solved (OneDrive example). By passing additional data a central redirect location can decide where to forward a successful authorization request. Use the bell 'location' setting for this and provide your data by a state query parameter. Also you need to switch on this extension via 'allowRuntimeState'.

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Feb 7, 2016

Thanks for trying to solve this:

  • The current implementation impacts the non allowRuntimeState path
  • I don't like the increased cost and complexity involved in validating state. This is something the spec warns directly about. We're increasing the surface area for denial of service, input validation.

If you really want something like this I think you have 2 solutions:

  1. Fork your own version of Bell
  2. Provide a solution that allows to pass a function for changing location.
@klaronix

This comment has been minimized.

Copy link
Contributor Author

klaronix commented Feb 7, 2016

Thank you for looking into this. But as I tried to explain in my comment to #184 my problem is not about dynamic lookup of location setting, but rather about forwarding state to another supporting redirect server.

But I understand your remarks about the impact on validation and the non allowRuntimeState path. Before trying to improve the solution, please give me a short notice, whether my scenario is something which should be supported this way or needs to be solved differently.

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Feb 7, 2016

With this explanation I guess I understand a bit more. I don't know what you are going to do on server B, but you are opening yourself up to a lot of attack vectors.

How is your solution safe from these attacks?

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

This will fail at the last step.. but server B may do stuff in between

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

You are not verifying the state query with the cookie value so how do you know it was not tampered in flight. The cookie is only valid for a.com domain.

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

You just set yourself up, for an attacker to redirect wherever

Because I think so much depends on how server B.com is implemented and how A and B communicate, I don't think I would want to accept such a PR. I would advise you find a different overall solution here.

Now, I've been known to be completely wrong on many occasions so feel free to demonstrate.

@klaronix

This comment has been minimized.

Copy link
Contributor Author

klaronix commented Feb 8, 2016

I thought a lot about this the last days and I'm confident, that it won't open additional attack vectors, if server at "b.com" uses the same precautions than any OAuth service provider: only forward to registered redirect locations. Then why I'm arguing about this here (since redirect uris already need to be registered): my problem is mentioned in

https://tools.ietf.org/html/rfc6819#section-5.2.3.5 (last note)

Pre-registering clients might not scale in some deployments (manual process) or
require dynamic client registration (not specified yet).

My server at "b.com" could implement a - yet to be developed - secure automatic registration/deregistration of a cluster of backend servers, which will handle the client authorization requests. The only thing I need for this is some piggy-backed information on the state token, since this is exchanged through the whole authorization (code) flow.

To your questions:

  • CSRF attacks for this scenario is not any more problematic than without the additional redirect. The already existing Bell state parameter (nonce) and the authorization cookie won't be altered. Server at "b.com" not even has access to the cookie (cross-domain) and doesn't need to. Validation happens after the redirect from "b.com" back to "a.com": Bell code at "a.com" checks original cookie and state the same way as before and prevents further processing on mismatch.
  • 10.14/10.15: "b.com" must validate redirect uri against its registered redirect locations or only allow a registered set of keys to "a.com" server instances.

As you said: it depends on what "b.com" will do with this information. If it doesn't check validity of the received piggy-backed state against its table of registered redirect locations, then you'll get an open redirector. But I think this is not a problem of Bell: an insecure OAuth service provider (without pre-registered redirect locations) would open the same attack vectors. I could imagine two problems:

  • The additional state information is altered before it reaches "b.com". Since we don't allow arbitrary redirects from "b.com", an attacker could only select one within the range of registered redirects. At least in my scenario this won't be an issue (since all of the registered "a.com" servers are equal and under my control) and it could be detected, since the modified "a2.com" server knows that it has not started the authorization and might fail.
  • Requesting access token will fail if the OAuth provider implements https://tools.ietf.org/html/rfc6819#section-5.2.4.5 (bind authorization code to redirect_uri). This would mean that "a.com" also needs to go through "b.com" for getting an access token later.

Hopefully I'm not missing some important point...

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Feb 14, 2016

Ok sounds good,

  1. Make sure it doesn't impact the most common case
  2. Is it possible to make it happen such that you don't pass the dynamic state inside the query parameter but as a function that you need to implement and must return a base64 encoded string that just gets appended to the state?
  3. Then, we see the call to Cryptiles.randomString(22); make this 22 be an internals variable and when parsing state, just do substring because that's all we care about.

I may have missed something since it's been a week but I think that covers it. I'll accept this.
Thank you!

@klaronix

This comment has been minimized.

Copy link
Contributor Author

klaronix commented Feb 15, 2016

Thank you for checking! I'm going to prepare a new PR, but it'll take some days before I'm able to get back on this topic.

@klaronix

This comment has been minimized.

Copy link
Contributor Author

klaronix commented Feb 17, 2016

@ldesplat: Finished now with refactoring. I hope this matches your expectations.

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Feb 25, 2016

This looks a lot better. I may have 1 comment, just give me a bit more time to figure it out but 👍

Thank you :)

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Feb 27, 2016

Thanks again :)

ldesplat added a commit that referenced this pull request Feb 27, 2016
Allow passing additional state
@ldesplat ldesplat merged commit f694386 into hapijs:master Feb 27, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ldesplat ldesplat added this to the 7.0.0 milestone Feb 27, 2016
@ldesplat ldesplat mentioned this pull request Feb 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.