search

TremoloSecurity / OpenUnison

Unified Identity Management
Apache License 2.0
76 stars 9 forks source link

Omitting state parameter causes NullPointerException #900

Closed tmarback closed 3 months ago

tmarback commented 4 months ago

If a client application attempts to authenticate with Unison but omits the state parameter, the call results in a server error due to a NullPointerException:

[2024-07-02 03:46:34,145][XNIO-1 task-8] ERROR ConfigSys - Could not process request
jakarta.servlet.ServletException: java.lang.NullPointerException
    at com.tremolosecurity.idp.providers.OpenIDConnectIdP$10.nextSys(OpenIDConnectIdP.java:1992) ~[unison-idp-openidconnect-1.0.40.jar:?]
    at com.tremolosecurity.proxy.auth.AzSys.doAz(AzSys.java:139) ~[unison-sdk-1.0.40.jar:?]
    at com.tremolosecurity.idp.providers.OpenIDConnectIdP.completeFederation(OpenIDConnectIdP.java:2003) ~[unison-idp-openidconnect-1.0.40.jar:?]
    at com.tremolosecurity.idp.providers.OpenIDConnectIdP.doGet(OpenIDConnectIdP.java:445) ~[unison-idp-openidconnect-1.0.40.jar:?]
    at com.tremolosecurity.idp.server.IDP.doGet(IDP.java:80) ~[unison-sdk-1.0.40.jar:?]
    at jakarta.servlet.http.HttpServlet.service(HttpServlet.java:527) ~[jakarta.servlet-api-6.0.0.jar:6.0.0]
    at jakarta.servlet.http.HttpServlet.service(HttpServlet.java:614) ~[jakarta.servlet-api-6.0.0.jar:6.0.0]
    at io.undertow.servlet.handlers.ServletHandler.handleRequest(ServletHandler.java:74) ~[undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.FilterHandler$FilterChainImpl.doFilter(FilterHandler.java:129) ~[undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at com.tremolosecurity.embedd.NextEmbSys.nextSys(NextEmbSys.java:135) ~[unison-server-core-1.0.40.jar:?]
    at com.tremolosecurity.proxy.auth.AuthMgrSys.doAuthMgr(AuthMgrSys.java:138) ~[unison-server-core-1.0.40.jar:?]
    at com.tremolosecurity.embedd.NextEmbSys.nextSys(NextEmbSys.java:126) ~[unison-server-core-1.0.40.jar:?]
    at com.tremolosecurity.proxy.auth.AzSys.doAz(AzSys.java:89) ~[unison-sdk-1.0.40.jar:?]
    at com.tremolosecurity.embedd.NextEmbSys.nextSys(NextEmbSys.java:111) ~[unison-server-core-1.0.40.jar:?]
    at com.tremolosecurity.proxy.auth.AuthSys.doAuth(AuthSys.java:88) ~[unison-server-core-1.0.40.jar:?]
    at com.tremolosecurity.embedd.NextEmbSys.nextSys(NextEmbSys.java:105) ~[unison-server-core-1.0.40.jar:?]
    at com.tremolosecurity.proxy.ConfigSys.doConfig(ConfigSys.java:296) [unison-server-core-1.0.40.jar:?]
    at com.tremolosecurity.embedd.NextEmbSys.nextSys(NextEmbSys.java:93) [unison-server-core-1.0.40.jar:?]
    at com.tremolosecurity.filter.UnisonServletFilter.doFilter(UnisonServletFilter.java:299) [unison-server-core-1.0.40.jar:?]
    at io.undertow.servlet.core.ManagedFilter.doFilter(ManagedFilter.java:67) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.FilterHandler$FilterChainImpl.doFilter(FilterHandler.java:131) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.FilterHandler.handleRequest(FilterHandler.java:84) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.security.ServletSecurityRoleHandler.handleRequest(ServletSecurityRoleHandler.java:62) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.ServletChain$1.handleRequest(ServletChain.java:68) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.ServletDispatchingHandler.handleRequest(ServletDispatchingHandler.java:36) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.RedirectDirHandler.handleRequest(RedirectDirHandler.java:68) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.security.SSLInformationAssociationHandler.handleRequest(SSLInformationAssociationHandler.java:117) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.security.ServletAuthenticationCallHandler.handleRequest(ServletAuthenticationCallHandler.java:57) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43) [undertow-core-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.security.handlers.AbstractConfidentialityHandler.handleRequest(AbstractConfidentialityHandler.java:46) [undertow-core-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.security.ServletConfidentialityConstraintHandler.handleRequest(ServletConfidentialityConstraintHandler.java:64) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.security.handlers.AuthenticationMechanismsHandler.handleRequest(AuthenticationMechanismsHandler.java:60) [undertow-core-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.security.CachedAuthenticatedSessionHandler.handleRequest(CachedAuthenticatedSessionHandler.java:77) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.security.handlers.AbstractSecurityContextAssociationHandler.handleRequest(AbstractSecurityContextAssociationHandler.java:43) [undertow-core-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43) [undertow-core-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.SendErrorPageHandler.handleRequest(SendErrorPageHandler.java:52) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43) [undertow-core-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.ServletInitialHandler.handleFirstRequest(ServletInitialHandler.java:276) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.ServletInitialHandler$2.call(ServletInitialHandler.java:135) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.ServletInitialHandler$2.call(ServletInitialHandler.java:132) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.core.ServletRequestContextThreadSetupAction$1.call(ServletRequestContextThreadSetupAction.java:48) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.core.ContextClassLoaderSetupAction$1.call(ContextClassLoaderSetupAction.java:43) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.ServletInitialHandler.dispatchRequest(ServletInitialHandler.java:256) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.servlet.handlers.ServletInitialHandler$1.handleRequest(ServletInitialHandler.java:101) [undertow-servlet-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.server.Connectors.executeRootHandler(Connectors.java:393) [undertow-core-2.3.12.Final.jar:2.3.12.Final]
    at io.undertow.server.HttpServerExchange$1.run(HttpServerExchange.java:859) [undertow-core-2.3.12.Final.jar:2.3.12.Final]
    at org.jboss.threads.ContextClassLoaderSavingRunnable.run(ContextClassLoaderSavingRunnable.java:35) [jboss-threads-2.3.6.Final.jar:2.3.6.Final]
    at org.jboss.threads.EnhancedQueueExecutor.safeRun(EnhancedQueueExecutor.java:1982) [jboss-threads-2.3.6.Final.jar:2.3.6.Final]
    at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.doRunTask(EnhancedQueueExecutor.java:1486) [jboss-threads-2.3.6.Final.jar:2.3.6.Final]
    at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1377) [jboss-threads-2.3.6.Final.jar:2.3.6.Final]
    at org.xnio.XnioWorker$WorkerThreadFactory$1$1.run(XnioWorker.java:1282) [xnio-api-3.8.13.Final.jar:3.8.13.Final]
    at java.base/java.lang.Thread.run(Thread.java:829) [?:?]
Caused by: java.lang.NullPointerException
    at java.base/java.net.URLEncoder.encode(URLEncoder.java:224) ~[?:?]
    at java.base/java.net.URLEncoder.encode(URLEncoder.java:196) ~[?:?]
    at com.tremolosecurity.idp.providers.OpenIDConnectIdP.postResponse(OpenIDConnectIdP.java:2055) ~[unison-idp-openidconnect-1.0.40.jar:?]
    at com.tremolosecurity.idp.providers.OpenIDConnectIdP$10$1.postProcess(OpenIDConnectIdP.java:1972) ~[unison-idp-openidconnect-1.0.40.jar:?]
    at com.tremolosecurity.proxy.filter.HttpFilterChainImpl.nextFilter(HttpFilterChainImpl.java:91) ~[unison-sdk-1.0.40.jar:?]
    at com.tremolosecurity.proxy.filters.SetupGroupMetadataWatch.doFilter(SetupGroupMetadataWatch.java:54) ~[unison-applications-k8s-1.0.40.jar:?]
    at com.tremolosecurity.proxy.filter.HttpFilterChainImpl.nextFilter(HttpFilterChainImpl.java:85) ~[unison-sdk-1.0.40.jar:?]
    at com.tremolosecurity.idp.providers.OpenIDConnectIdP$10.nextSys(OpenIDConnectIdP.java:1989) ~[unison-idp-openidconnect-1.0.40.jar:?]
    ... 51 more

With the problem being (as far as I can tell) that the redirect generation code unconditionally attempts to process the state without checking if it exists.

The state parameter is optional in the OIDC spec so in theory this should be accepted, although an argument can be made that it shouldn't be omitted so requiring it isn't entirely unreasonable (though still it would be nice to be able to opt to disable the requirement, since sometimes there just isn't much that can be done about it).

But either way, this should at least give a proper error identifying the problem; preferably at the start of handling to save on calls to the upstream provider.