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

oxAuth Does Not Enforce Registered `post_logout_redirect_uri` #927

Closed
afroDC opened this Issue Oct 22, 2018 · 18 comments

Comments

Projects
None yet
5 participants
@afroDC
Contributor

afroDC commented Oct 22, 2018

A client with a post_logout_redirect_uri registered in oxTrust will not enforce this requirement, and will instead allow any post logout redirect uri to be appended to the end session request.

For example the below would redirect the user to evil.com:

https://good.gluu.org/oxauth/restv1/end_session?post_logout_redirect_uri=https://evil.com/

If this redirect uri is not registered, the user should not be redirected to that end point.

@afroDC afroDC added the bug label Oct 22, 2018

@afroDC afroDC added this to the 3.1.5 milestone Oct 22, 2018

@yuriyz yuriyz self-assigned this Oct 22, 2018

@yuriyz

This comment has been minimized.

Contributor

yuriyz commented Oct 22, 2018

@afroDC I've just tried it against ce-dev3 and it returns 400 (bad request) with error post_logout_uri_not_associated_with_client. Would you be so kind to try it against our ce-dev3 and record your http request/responses ?

@aliaksander-samuseu

This comment has been minimized.

Contributor

aliaksander-samuseu commented Oct 23, 2018

@yuriyz @afroDC

I think I have confirmation. The missing requirement to properly reproduce it is that no cookies related to an existing session at oxAuth must be passed in request. It may contain stale cookies (related to already ended session), or just no cookies at all.

When existing session is referenced, it won't send you to unregistered post-logout uri, as expected. But with no actual cookies and with https://evil.com/ not registered as valid post-logout uri, Chris's request results in next response from /end_session:

HTTP/1.1 307 Temporary Redirect
Date: Tue, 23 Oct 2018 00:12:00 GMT
Server: Jetty(9.4.12.v20180830)
X-Xss-Protection: 1; mode=block
X-Content-Type-Options: nosniff
Strict-Transport-Security: max-age=31536000; includeSubDomains
Expires: Thu, 01 Jan 1970 00:00:00 GMT
Content-Type: text/plain
Location: https://evil.com/
Content-Length: 149
Set-Cookie: session_id=;Path=/;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;HttpOnly
Connection: close

{"error":"invalid_grant_and_session","error_description":"The provided access token and session state are invalid or were issued to another client."}

As you see, though it reports an error, it still a redirect response code. So it indeed may be used as an open redirector.

Next request was used to reproduce it:

GET /oxauth/restv1/end_session?post_logout_redirect_uri=https%3A%2F%2Fevil.com%2F HTTP/1.1
Host: some-host.gluu.loc
Connection: close
Upgrade-Insecure-Requests: 1
User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.116 Safari/537.36
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8
Referer: https://evil.com/somepage
Accept-Encoding: gzip, deflate
Accept-Language: en,en-US;q=0.8,fr;q=0.6
Cookie: _ga=GA1.2.291166285.1534979713


@yuriyz

This comment has been minimized.

Contributor

yuriyz commented Oct 23, 2018

@afroDC @aliaksander-samuseu this behavior was requested by Yura and added by Eugeniu here #575 .

@yurem I think it makes sense to close this open redirect but then it means that we will return error as it was before (see #575). Do we have functionality that relies on this redirect?

@yurem

This comment has been minimized.

Contributor

yurem commented Nov 15, 2018

In issue #575 I requested "Don't show error page on logout when id_token is expired #575". This error page break many integration flows. But I agree to display it when data if wrong. We have to return back to previous issue discussion.

We need to store somehow information about expired tokens. I have one idea. What do you think about adding small AI to grant service cleaner job? I offer to not clean up all expired grants associated with specific user session. We need to keep last created grant (even expired) until new grant will be issued. This small data overhead not make DB very big but will resolve issue which we dicsussed in previous ticket.

@nynymike

This comment has been minimized.

Contributor

nynymike commented Nov 15, 2018

Sounds like a good idea, as long as the expired grant can't be used to get access to anything by mistake.

@yurem

This comment has been minimized.

Contributor

yurem commented Nov 15, 2018

yes, right. This require negative tests and will be useful for cases when cleaner interval is bigger than grant lifetime

@yuriyz

This comment has been minimized.

Contributor

yuriyz commented Nov 15, 2018

What should happen if on next grant creation we call logout with expired id_token ? Since expired grant will be cleaned up (because according to our AI new grant already created), we will forbid redirect. It will produce inconsistency in behavior and break integration flows as it was prior #575.

Should we add configuration options which allows or forbids such redirect with default value false? Then Admin can explicitly set it to true if needed.

@yurem

This comment has been minimized.

Contributor

yurem commented Nov 15, 2018

yes, after new grant creation we will remove expired grant. Why RP should call end_session with expired token after new grant creation? It's RP bug.

@yurem

This comment has been minimized.

Contributor

yurem commented Nov 15, 2018

This new option will just hide bug which Chris mentioned. Maybe this option will be good with global list of white-listed redirect_uri...?

@yuriyz

This comment has been minimized.

Contributor

yuriyz commented Nov 15, 2018

Right, it seems to be best option for now. If we didn't find out client (because of expired token, grant or client entity itself) then :

  • show error page if redirect_uri is not white listed
  • redirect if it is white listed.
@yurem

This comment has been minimized.

Contributor

yurem commented Nov 15, 2018

@afroDC What do you think? Should this help to resolve this issue?

@afroDC

This comment has been minimized.

Contributor

afroDC commented Nov 15, 2018

This looks like a valid solution. Have we mitigated any potential complications? @yurem @yuriyz

@yurem

This comment has been minimized.

Contributor

yurem commented Nov 20, 2018

I've one new idea. end_session endpoint uses id_token to validate client. But id_token can expire and this led to this issue. Why we are trying to keep expired tokens? There is another way to validate client authority. We can update end_session to accept either id_token or client_id/client_password (JWT/Basic/PostAuth).

@afroDC

This comment has been minimized.

Contributor

afroDC commented Nov 20, 2018

@yurem would this violate and deviate from the OpenID spec? https://openid.net/specs/openid-connect-session-1_0-00.html#anchor16

@yuriyz

This comment has been minimized.

Contributor

yuriyz commented Nov 21, 2018

id_token_hint parameter is optional. It can be omitted during request. Then oxauth will pick up session_id from cookie and validate against clients that are recorded in the session. However this particular case is about request when there is NO session (see request from Alex and his comments). So we don't have client(s) against which we can validate our post_logout_redirect_uri.

We stick to this: https://openid.net/specs/openid-connect-session-1_0.html#RPLogout

About id_token_hint vs session_id, it is explained here:
https://gluu.org/docs/ce/3.1.4/operation/logout/

Now back to question whether we need client_id/client_secret in request. That would mean we are introducing client authentication to /end_session endpoint which we need ONLY for case when session is not there. Doesn't it look as too much for negative use case ? White-listed post_logour_redirect_uri sounds as acceptable solution for current problem, doesn't it ?

@nynymike

This comment has been minimized.

Contributor

nynymike commented Nov 22, 2018

I think white listing post logout uri is ok, but isn't it potentially not backwards compatible? If so, we need to make the option configurable.

@yuriyz

This comment has been minimized.

Contributor

yuriyz commented Nov 22, 2018

Right, lets make it configurable (e.g. allow_post_logout_redirect_without_validation) with default value false. To summarize:

If we didn't find out client (because of expired token, grant or client entity itself) then :

  • if allow_post_logout_redirect_without_validation=true -> redirect as it is now;
  • if allow_post_logout_redirect_without_validation=false -> check white listed redirect_uris. If white listed then allow redirect, if not then show error page.

yuriyz added a commit that referenced this issue Nov 26, 2018

#927 : Allow post logout redirect without validation only if:
1. allowPostLogoutRedirectWithoutValidation = true
2. or if post_logout_redirect_uri is white listed
                                                   #927

yuriyz added a commit that referenced this issue Nov 26, 2018

#927 (4.0): Allow post logout redirect without validation only if:
1. allowPostLogoutRedirectWithoutValidation = true
2. or if post_logout_redirect_uri is white listed
                                                   #927

(cherry picked from commit d80099e)

yuriyz added a commit to GluuFederation/oxTrust that referenced this issue Nov 26, 2018

yuriyz added a commit to GluuFederation/oxTrust that referenced this issue Nov 26, 2018

yuriyz added a commit to GluuFederation/docs-ce-prod that referenced this issue Nov 26, 2018

@yuriyz

This comment has been minimized.

Contributor

yuriyz commented Nov 26, 2018

done in 3.1.5 and master

@yuriyz yuriyz closed this Nov 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment