Skip to content

Commit 6f3e97a

Browse files
dxbjavidreta
dxbjavid
authored and
reta
committed
validate redirect origin in oidc rp sign-in completion (#3241)
* validate redirect origin in oidc rp sign-in completion Signed-off-by: dxbjavid <dxbjavid@gmail.com> * harden redirect state in oidc rp filter rather than completion service * derive same-origin base from request uri info instead of message context * add filter test for state with scheme but no authority Signed-off-by: Javid Khan <dxbjavid@gmail.com> --------- Signed-off-by: dxbjavid <dxbjavid@gmail.com> Signed-off-by: Javid Khan <dxbjavid@gmail.com> (cherry picked from commit d36cff4) # Conflicts: # rt/rs/security/sso/oidc/pom.xml
1 parent a31fbe4 commit 6f3e97a

3 files changed

Lines changed: 138 additions & 1 deletion

File tree

‎rt/rs/security/sso/oidc/pom.xml‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838
</cxf.osgi.import>
3939
<!-- this configures the surefire plugin to run your tests with the javaagent enabled -->
4040
<!-- (openJPA loadtime weaving) -->
41-
<cxf.surefire.fork.vmargs.extra>-javaagent:${org.apache.openjpa:openjpa:jar:jakarta}</cxf.surefire.fork.vmargs.extra>
41+
<cxf.surefire.fork.vmargs.extra>-javaagent:${org.apache.openjpa:openjpa:jar:jakarta} -javaagent:${org.mockito:mockito-core:jar}</cxf.surefire.fork.vmargs.extra>
4242
</properties>
4343
<dependencies>
4444
<dependency>
@@ -64,6 +64,12 @@
6464
<artifactId>junit</artifactId>
6565
<scope>test</scope>
6666
</dependency>
67+
<dependency>
68+
<groupId>org.mockito</groupId>
69+
<artifactId>mockito-core</artifactId>
70+
<version>${cxf.mockito.version}</version>
71+
<scope>test</scope>
72+
</dependency>
6773
<dependency>
6874
<groupId>org.hsqldb</groupId>
6975
<artifactId>hsqldb</artifactId>

‎rt/rs/security/sso/oidc/src/main/java/org/apache/cxf/rs/security/oidc/rp/OidcRpAuthenticationFilter.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,33 @@ private MultivaluedMap<String, String> toRequestState(ContainerRequestContext rc
115115
rc.setEntityStream(new ByteArrayInputStream(StringUtils.toBytesUTF8(body)));
116116

117117
}
118+
// The "state" carried here is read back by the sign-in completion service and returned
119+
// as a redirect Location, so a caller-supplied value collides with the redirect query
120+
// the filter itself writes. Anything that is not within this application's own origin is
121+
// dropped, otherwise completion would become an open redirect.
122+
String location = requestState.getFirst("state");
123+
if (location != null && !isSameOrigin(rc, location)) {
124+
requestState.remove("state");
125+
}
118126
return requestState;
119127
}
128+
private boolean isSameOrigin(ContainerRequestContext rc, String location) {
129+
final URI uri;
130+
try {
131+
uri = URI.create(location);
132+
} catch (IllegalArgumentException ex) {
133+
return false;
134+
}
135+
if (uri.getScheme() == null && uri.getAuthority() == null) {
136+
// a path-only reference is resolved by the browser against the current request
137+
return true;
138+
}
139+
URI base = rc.getUriInfo().getAbsolutePath();
140+
return uri.getScheme() != null
141+
&& uri.getScheme().equalsIgnoreCase(base.getScheme())
142+
&& uri.getAuthority() != null
143+
&& uri.getAuthority().equalsIgnoreCase(base.getAuthority());
144+
}
120145
public void setRedirectUri(String redirectUri) {
121146
this.redirectUri = redirectUri;
122147
}

‎rt/rs/security/sso/oidc/src/test/java/org/apache/cxf/rs/security/oidc/rp/OidcRpAuthenticationFilterTest.java‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
/**
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.apache.cxf.rs.security.oidc.rp;
20+
21+
import java.lang.reflect.Method;
22+
import java.net.URI;
23+
24+
import jakarta.ws.rs.container.ContainerRequestContext;
25+
import jakarta.ws.rs.core.MultivaluedHashMap;
26+
import jakarta.ws.rs.core.MultivaluedMap;
27+
import jakarta.ws.rs.core.UriInfo;
28+
29+
import org.junit.Test;
30+
31+
import static org.junit.Assert.assertEquals;
32+
import static org.junit.Assert.assertFalse;
33+
import static org.junit.Assert.assertTrue;
34+
import static org.mockito.Mockito.mock;
35+
import static org.mockito.Mockito.when;
36+
37+
public class OidcRpAuthenticationFilterTest {
38+
39+
private static final URI ABSOLUTE_PATH = URI.create("https://app.example.com:8080/services/rp/complete");
40+
41+
@Test
42+
public void testDropsCrossOriginState() {
43+
MultivaluedMap<String, String> state = requestState("https://evil.example.com/phish");
44+
assertFalse(state.containsKey("state"));
45+
}
46+
47+
@Test
48+
public void testDropsProtocolRelativeState() {
49+
MultivaluedMap<String, String> state = requestState("//evil.example.com/phish");
50+
assertFalse(state.containsKey("state"));
51+
}
52+
53+
@Test
54+
public void testDropsUserinfoHostConfusionState() {
55+
MultivaluedMap<String, String> state = requestState("https://app.example.com:8080@evil.example.com/phish");
56+
assertFalse(state.containsKey("state"));
57+
}
58+
59+
@Test
60+
public void testDropsNoAuthority() {
61+
MultivaluedMap<String, String> state = requestState("http:/");
62+
assertFalse(state.containsKey("state"));
63+
}
64+
65+
@Test
66+
public void testKeepsSameOriginState() {
67+
MultivaluedMap<String, String> state = requestState("https://app.example.com:8080/services/protected");
68+
assertTrue(state.containsKey("state"));
69+
assertEquals("https://app.example.com:8080/services/protected", state.getFirst("state"));
70+
}
71+
72+
@Test
73+
public void testKeepsRelativeState() {
74+
MultivaluedMap<String, String> state = requestState("/services/protected");
75+
assertTrue(state.containsKey("state"));
76+
assertEquals("/services/protected", state.getFirst("state"));
77+
}
78+
79+
private MultivaluedMap<String, String> requestState(String stateLocation) {
80+
MultivaluedMap<String, String> query = new MultivaluedHashMap<>();
81+
query.putSingle("state", stateLocation);
82+
83+
UriInfo uriInfo = mock(UriInfo.class);
84+
when(uriInfo.getQueryParameters(true)).thenReturn(query);
85+
when(uriInfo.getAbsolutePath()).thenReturn(ABSOLUTE_PATH);
86+
87+
ContainerRequestContext rc = mock(ContainerRequestContext.class);
88+
when(rc.getUriInfo()).thenReturn(uriInfo);
89+
when(rc.getMediaType()).thenReturn(null);
90+
91+
return invokeToRequestState(new OidcRpAuthenticationFilter(), rc);
92+
}
93+
94+
@SuppressWarnings("unchecked")
95+
private static MultivaluedMap<String, String> invokeToRequestState(OidcRpAuthenticationFilter filter,
96+
ContainerRequestContext rc) {
97+
try {
98+
Method method = OidcRpAuthenticationFilter.class.getDeclaredMethod("toRequestState",
99+
ContainerRequestContext.class);
100+
method.setAccessible(true);
101+
return (MultivaluedMap<String, String>)method.invoke(filter, rc);
102+
} catch (ReflectiveOperationException ex) {
103+
throw new IllegalStateException(ex);
104+
}
105+
}
106+
}

0 commit comments

Comments
 (0)