OIDC post_logout_redirect_uri

[Thomas Broyer]

Hi,

I'm using jakartaee-pac4j with OIDC; I've configured the defaultUrl of my LogoutFilter (https://www.pac4j.org/docs/logout-endpoint.html) to "/" to always redirect back to the root of my application, but in the DefaultLogoutLogic it only sends a post_logout_redirect_uri to the IdP logout_endpoint if the redirectUrl starts with "http" or "https".

My application could be possibly reached from different URLs so I couldn't really (and don't want to) configure a full URL as the defaultUrl. As a workaround, I could make my own LogoutFilter that computes the URL from the HttpRequest before calling the LogoutLogic.

I believe it would however just be a bad workaround and there's a deeper bug (not just limitation) in pac4j: DefaultLogoutLogic also checks for a url query parameter and matches it against the logoutUrlPattern, that defaults to "only relative URLs are allowed"; and using that, the computed redirectUrl would still be relative and be ignored and never sent as post_logout_redirect_uri to the IdP. This leads me to think that the DefaultLogoutLogic was meant to resolve the relative URLs rather than ignore them. I would add that the check for whether the URLs are absolute is kinda broken: it should check whether it begins with "http://" or "https://", not just "http" or "https".

I can work on a fix, but not quite sure what behavior is actually expected, given that there's a test that seems to contradict the default value for logoutUrlPattern: https://github.com/pac4j/pac4j/blob/046bce785e0f89f2bfdf5e1e625f1196d3d67015/pac4j-core/src/test/java/org/pac4j/core/engine/DefaultLogoutLogicTests.java#L166

Anyone managed to have a post_logout_redirect_uri without configuring an absolute URL as defaultUrl? (or using the url query parameter as a relative URL, i.e. without reconfiguring the logoutUrlPattern)

[Jérôme LELEU]

Hi,

What's the host of your application and the host of your OIDC provider?

Thanks.

Best regards,

Jérôme

--
You received this message because you are subscribed to the Google Groups "Pac4j users mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to pac4j-users...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/pac4j-users/2a8a60f9-00c3-4543-a2ec-c42498294444n%40googlegroups.com.

[Thomas Broyer]

Le jeu. 29 sept. 2022 à 14:59, Jérôme LELEU <lel...@gmail.com> a écrit :

Hi,

What's the host of your application and the host of your OIDC provider?

Not sure why it matters but here you go, assuming you're asking about URLs and not deployment infrastructure.

The OIDC provider is a Keycloak instance; the application uses an embedded Jetty server with ServletContext (so no war or web.xml). pac4j is wired up entirely programmatically (no pac4j-config).

On the developer machine, Keycloak is run as a local Docker container, so the application will be either http://localhost:8081 or http://localhost:9200 (depending on whether I go through the JS dev server or directly connect to the Java server) and the OIDC provider will be http://localhost:8080.

In test, I'll have something like https://myapp.mycompany.dev or http://myapp.dev.priv.mycompany.com for the application, and https://myapp-auth.mycompany.dev for the OIDC provider.

In preprod, I'll have something like https://myapp-preprod.priv.example.com and https://myapp-auth-preprod.priv.example.com, and in prod https://myapp.example.com (or https://myapp.priv.example.com from the internal network) and https://myapp-auth.example.com (with TLS terminated by a reverse proxy) ; same binary deployed on different machines.

That's why currently I just created my LogoutFilter with a hardcoded defaultUrl of "/", assuming the actual post_logout_redirect_uri would be resolved from the request.

Full code for creating the filter (the 'config' only contains a KeycloakOidcClient and a matcher to exclude static resources):

    var logoutFilter = new LogoutFilter(config, "/");

    logoutFilter.setDestroySession(true);
    logoutFilter.setCentralLogout(true);

    context.addFilter(new FilterHolder(logoutFilter), "/logout", null);

In dev currently, because I didn't configure a baseUrl in the Keycloak client for my application (as it could equally be localhost:8081 or localhost:9200), Keycloak displays a "you are logged out" page with no "back to application" link. I was expecting that pac4j would redirect to Keycloak with a post_logout_redirect_uri so Keycloak would just redirect back to my application without even showing the "you are logged out" page. But currently pac4j "expects" me (the code in its current form at least, not necessarily the expected behavior) to configure the LogoutFilter with a full "http://localhost:8081/" or "http://localhost:9200/".

Fwiw, the problem is here in the code (form my debugging sessions and my understanding of pac4j internals):

* https://github.com/pac4j/pac4j/blob/b4810eb2168d9525f37e7a0dee9b5534f06e6acc/pac4j-core/src/main/java/org/pac4j/core/engine/DefaultLogoutLogic.java#L74-L80 (where logoutUrlPattern defaults to "/.*" to only match relative URLs)

* https://github.com/pac4j/pac4j/blob/b4810eb2168d9525f37e7a0dee9b5534f06e6acc/pac4j-core/src/main/java/org/pac4j/core/engine/DefaultLogoutLogic.java#L112-L120 (the redirectUrl –either defaultUrl or a url query parameter that matched logoutUrlPattern– has never been resolved to a full URL, so is a relative URL and won't start with either "http" or "https")

--

Cordialement,

-- 

Thomas Broyer

Atol Conseils et Développements

[Jérôme LELEU]

Hi,

You can pass a url parameter to control the post logout URL and this URL is checked against the logoutUrlPattern parameter which is by default /.*

This is done this way for security reasons. For local logout, we only allow relative URLs (by default) to avoid phishing.

When it comes to central logout and the post logout URL, this post logout URL will be used to redirect from the identity provider to the web app after the SLO process.

As the application and the identity server are not on the same host, an absolute URL and the http/https check are required in that case. Hence my question.

That's why you have both checks. The solution is to configure the logoutUrlPattern parameter to the absolute URL you want.

Thanks.

Best regards,

Jérôme

[Thomas Broyer]

Le jeu. 29 sept. 2022 à 17:37, Jérôme LELEU <lel...@gmail.com> a écrit :

Hi,

You can pass a url parameter to control the post logout URL and this URL is checked against the logoutUrlPattern parameter which is by default /.*

This is done this way for security reasons. For local logout, we only allow relative URLs (by default) to avoid phishing.

When it comes to central logout and the post logout URL, this post logout URL will be used to redirect from the identity provider to the web app after the SLO process.

As the application and the identity server are not on the same host, an absolute URL and the http/https check are required in that case. Hence my question.

That's why you have both checks. The solution is to configure the logoutUrlPattern parameter to the absolute URL you want.

Thanks for the explanation, and confirming that this is actually the expected behavior.

That however means that I'm forced to use the url query parameter (which means compute it in my app when generating links to /logout); when pac4j would actually be able to resolve the relative URLs to an absolute one (rather than reject them) by itself, using the current scheme, host and port.

Let's repurpose my problem as a request for enhancement then 😉

Is there a reason that this would be a bad idea? (parse the redirectUrl as a java.net.URI, get/compute the base URI –scheme://host:port– for the current request, and use URI#resolve: if redirectUrl is an absolute URI it's kept as is, otherwise it's resolved against the scheme://host:port to an absolute URI)

One potential problem would be with "context paths" in servlet deployments, where redirects have special resolution rules (https://javadoc.io/doc/jakarta.platform/jakarta.jakartaee-api/latest/jakarta/servlet/http/HttpServletResponse.html#sendRedirect(java.lang.String))

Fwiw, given the low complexity of LogoutFilter, I think I'll just make one in the project that computes the 'defaultUrl' it passes to the LogoutLogic, rather than using the url query parameter.

How about maybe adding a "hook" in LogoutFilter so we could dynamically compute a getDefaultUrl() at request time before the filter calls the LogoutLogic? …or extract the redirectUri computation in DefaultLogoutLogic to a pluggable callback?

WDYT?

[Jérôme LELEU]

Hi,

You can dynamically use the url parameter or statically set the defaultUrl to an absolute URL (and change the logoutUrlPattern accordingly).

Dynamically computing the defaultUrl is not possible yet and this is the first time the question has been raised.

That said, I like the idea of an overridable method for this kind of rare use cases. This is what I just committed: https://github.com/pac4j/pac4j/commit/5139681207a2052ef1d87b2628d06dc78a3aee2f (version 5.6.1-SNAPSHOT).

You could then create your own CustomLogoutLogic inheriting from DefaultLogoutLogic and override the method computeDefaultUrl with the appropriate UrlResolver.

Thanks.

Best regards,

Jérôme

[Thomas Broyer]

Le jeu. 29 sept. 2022 à 19:10, Jérôme LELEU <lel...@gmail.com> a écrit :

That said, I like the idea of an overridable method for this kind of rare use cases. This is what I just committed: https://github.com/pac4j/pac4j/commit/5139681207a2052ef1d87b2628d06dc78a3aee2f (version 5.6.1-SNAPSHOT).

You could then create your own CustomLogoutLogic inheriting from DefaultLogoutLogic and override the method computeDefaultUrl with the appropriate UrlResolver.

Thanks.

Fwiw, I'd find it even more useful if it processed the redirectUri, after the url query parameter has been validated against the logoutUrlPattern. That would allow using relative URLs in the url parameter, which looks much better and is easier to validate in the logoutUrlPattern: https://myapp.example.com/logout?url=/foobar would be able to compute a redirectUri of https://myapp.example.com/foobar that could then be used as the post_logout_redirect_uri. The exact same app accessed as http://myapp.priv.example.com:8080/logout?url=/foobar would correctly compute http://myapp.priv.example.com:8080/foobar too.

Also (only looking at the code on GitHub so I might be missing something), I don't see how I would get the current request's scheme/host/port in the overridden computeDefaultUrl; should't the WebContext be passed as an argument to the method?

What would such a CustomLogoutLogic look like? (given my use case of: redirect to the "root of the app" –defaultUrl="/"–, whose absolute URL I don't have as a configuration option but need/want to compute from the current request)

public class CustomLogoutLogic extends DefaultLogoutLogic {

   @Override

   protected String computeDefaultUrl(final String defaultUrl) {

      // ???

   }

}

[Jérôme LELEU]

Hi,

You're right, the web context (at least) was missing. See: https://github.com/pac4j/pac4j/commit/8d925f4ac77baeb587fc8870491b3ed9ae7349c5

And your feedback makes me think that the "redirection URL enhancement" should only happen if we want to perform a central logout so I renamed and moved the method: https://github.com/pac4j/pac4j/commit/15310ee6b79003737bd80071dfb0961f57018e88

Are we good now?

Thanks.

Best regards,

Jérôme

[Thomas Broyer]

Le ven. 30 sept. 2022 à 11:44, Jérôme LELEU <lel...@gmail.com> a écrit :

Hi,

You're right, the web context (at least) was missing. See: https://github.com/pac4j/pac4j/commit/8d925f4ac77baeb587fc8870491b3ed9ae7349c5

And your feedback makes me think that the "redirection URL enhancement" should only happen if we want to perform a central logout so I renamed and moved the method: https://github.com/pac4j/pac4j/commit/15310ee6b79003737bd80071dfb0961f57018e88

Are we good now?

Looks good, yes, thanks a lot!

Now that it's been moved, should the Client be passed to the method as well? (thinking about how to get a UrlResolver: I could get one from the Config, or from the IndirectClient – or create a DefaultUrlResolver)

[Jérôme LELEU]

Good point: https://github.com/pac4j/pac4j/commit/a79e69967d50598d0435f7dbb05c757d957c4f74

[Misagh]

Have you checked to see if this leads to an open-redirect issue? A
quick read of the latest commits suggests that it might.

> --
> You received this message because you are subscribed to the Google Groups "Pac4j users mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to pac4j-users...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/pac4j-users/CAP279Ly8Pp2DPjbMt4o0Q1W7vkqcB5aQoLYZKvoeoQSwiYgGUA%40mail.gmail.com.

[Jérôme LELEU]

Hi,

Any enhancement would be controlled by the code of the application, not by something provided externally which could be modified by an attacker.

In that case, it's about adding a predefined host to a relative URL, so even if you change the relative URL, the host is still fixed by the code.

Do you have something in mind?

Thanks.

Best regards,

Jérôme

[Misagh]

> Any enhancement would be controlled by the code of the application, not by something provided externally which could be modified by an attacker.
>
> In that case, it's about adding a predefined host to a relative URL, so even if you change the relative URL, the host is still fixed by the code.

I don't think that is the case; I can bypass the host. Please see:
https://gist.github.com/mmoayyed/f677660b5415e372c45f125e69786363

This lets me redirect to: file:///evil.example.org/download.exe

We need to tighten the logout pattern. This is too loose by default.

[Misagh]

A more secure approach would be that if the logout pattern is
undefined (and it should be undefined by default), never match
anything and do not use the default pattern. Let the app "explicitly"
define what is allowed.

We can never come up with a pattern that would be both safe and
convenient. Maybe, but it's always a source of worry! So it's best to
block all by default, unless someone forcefully decides to take that
option. This is the sort of advanced option that most folks would not
pay attention to, and by blocking this by default, you'd force them to
pay attention.

of course, issue WARN statements to make the behavior known.

[Jérôme LELEU]

Hi,

I just tested your use case on a demo (https://github.com/pac4j/spring-webmvc-pac4j-boot-demo).

If I use /evil.example.org/download.exe for the url parameter, I'm redirected to: http://localhost:8080/evil.example.org/download.exe, the existing host remains and I get a 404, no security vulnerability.

Thanks.

Best regards,

Jérôme

--
You received this message because you are subscribed to the Google Groups "Pac4j users mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to pac4j-users...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/pac4j-users/CAGSBKkdohox%2BKVKcZfZUPqJZG9mZ384K7UsB4JJ6-yfPb3MKXg%40mail.gmail.com.

[Jérôme LELEU]

Hi,

This is what I think about the relative URL pattern: /.* It's safe and convenient, isn't it?

Thanks.

Best regards,

Jérôme

--
You received this message because you are subscribed to the Google Groups "Pac4j users mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to pac4j-users...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/pac4j-users/CAGSBKkf9MWPjp%3DWKLGoJvK97mOC%3DznzE%2BNU%2BfnEDzx-VJ_6rEg%40mail.gmail.com.

[Misagh]

Try this: http://localhost:8080/logout?url=//evil.example.org/download.exe

[Jérôme LELEU]

You're right, it redirects to http://evil.example.org/download.exe!

Should we use /[^/].* instead of /.* or you still think it's too risky?

Thanks.

Best regards,

Jérôme

[Misagh]

The safest course would be to not match by default and let the user
define the pattern; if that is too much, I suppose your pattern could
be a good replacement. To be exact, this should do: ^\/[^\/].*$

Even though the code is using Pattern.matches(), I think it's best to
make the pattern be explicit with ^ and $ surrounding it.

[Thomas Broyer]

Fwiw, I must say I don't see how the recent changes to DefaultLogoutLogic create any open-redirect issue (but maybe you were not looking at the very latest changes)

https://github.com/pac4j/pac4j/compare/b4810eb2168d9525f37e7a0dee9b5534f06e6acc...a79e69967d50598d0435f7dbb05c757d957c4f74

They only touch the code path for indirect clients / central logout and just pass the value through a new overridable method which returns it as-is (by default): the check for an absolute URL is still there for instance, unchanged.

If there's an open-redirect vulnerability without central logout, it already existed before those changes were made.

And even the code path that reassigns redirectUrl to the result of the enhanceRedirectUrl method call is benign because the variable is not reused afterwards (I would however assign targetUrl instead –then reassign it to null if the absolute URL check fails–, just in case the "fallback" code when the client returns an empty logoutAction is later changed to use redirectUrl and assumes it hasn't changed since it was computed earlier in the method).

Le ven. 30 sept. 2022 à 14:01, Misagh <misagh....@gmail.com> a écrit :

--

[Misagh]

It's possible this was there all along. The conversation here just help me/us catch it. We have already confirmed/reproduced this. 

-- Misagh

[Jérôme LELEU]

Hi,

I'm fully in line with you. I will do some more tests and cut the security releases (and make the announcement).

Thank you for spotting the issue.

Best regards,

Jérôme

To view this discussion on the web visit https://groups.google.com/d/msgid/pac4j-users/CAGSBKkdQm7Ti3UZ8-ehyuN3qVuNaxuG1x5MkMVFpTMU277%3D%3DPw%40mail.gmail.com.

[Jérôme LELEU]

Yes, the problem has always been there :-(

To view this discussion on the web visit https://groups.google.com/d/msgid/pac4j-users/CAGSBKkdidUNt%2BbxcoSVYPFf2RbeoR9jWkOmA6mykA8X-%3DfEXiw%40mail.gmail.com.

[Thomas Broyer]

Just a heads-up to let you know that this works like a charm!

Updated my dependencies to 5.6.1 (and jakartaee-pac4j 7.1.0) and added the following to my initialization code:

    config.setLogoutLogic(
        new DefaultLogoutLogic() {
          @Override
          protected String enhanceRedirectUrl(Config config, Client client, WebContext context, SessionStore sessionStore, String redirectUrl) {
            return ((KeycloakOidcClient) client).getUrlResolver().compute(redirectUrl, context);
          }
        });

--

[Jérôme LELEU]

Thanks for the follow up