diff --git a/gateway-service/src/main/java/org/zowe/apiml/gateway/config/WebSecurity.java b/gateway-service/src/main/java/org/zowe/apiml/gateway/config/WebSecurity.java index c54727953e..50fdf62139 100644 --- a/gateway-service/src/main/java/org/zowe/apiml/gateway/config/WebSecurity.java +++ b/gateway-service/src/main/java/org/zowe/apiml/gateway/config/WebSecurity.java @@ -83,6 +83,8 @@ import reactor.core.publisher.Mono; import java.io.IOException; +import java.net.URI; +import java.net.URISyntaxException; import java.nio.charset.StandardCharsets; import java.security.KeyStoreException; import java.security.MessageDigest; @@ -131,6 +133,9 @@ public class WebSecurity { @Value("${apiml.security.enableStrictUrlValidation:true}") private boolean isStrictUrlValidationEnabled; + @Value("${apiml.service.externalUrl:}") + private String externalUrl; + private final ClientConfiguration clientConfiguration; private final TokenProvider tokenProvider; @@ -213,11 +218,154 @@ SecurityWebFilterChain oauth2WebFilterChain( .build(); } + /** + * Validates a relative return URI. + * Only root-relative paths are accepted. Protocol-relative URLs, + * paths with an authority component, and other ambiguous forms are rejected. + * + * @param uri the relative URI to validate + * @return a safe relative location or {@link #CONTEXT_PATH} if invalid + */ + private String sanitizeRelativeUri(URI uri) { + if (uri.getRawAuthority() != null) { + return CONTEXT_PATH; + } + + String path = uri.getRawPath(); + + if (StringUtils.isEmpty(path) + || !path.startsWith("/") + || path.startsWith("//")) { + return CONTEXT_PATH; + } + + return toRelativeLocation(uri); + } + + /** + * Checks whether the supplied absolute URI matches the configured Gateway origin. + * + * @param candidate the URI provided by the client + * @param configuredExternalUri the configured Gateway external URL + * @return true if both URIs share the same origin, false otherwise + */ + private boolean isSameOrigin(URI candidate, URI configuredExternalUri) { + if (candidate.getScheme() == null + || candidate.getHost() == null + || configuredExternalUri.getScheme() == null + || configuredExternalUri.getHost() == null + || candidate.getRawUserInfo() != null) { + return false; + } + + return candidate.getScheme().equalsIgnoreCase(configuredExternalUri.getScheme()) + && candidate.getHost().equalsIgnoreCase(configuredExternalUri.getHost()) + && effectivePort(candidate) == effectivePort(configuredExternalUri); + } + + /** + * Returns the effective port of the URI. + * If the URI does not explicitly specify a port, the default port for + * the URI scheme is returned (80 for HTTP, 443 for HTTPS). + * + * @param uri the URI + * @return the effective port + */ + private int effectivePort(URI uri) { + if (uri.getPort() >= 0) { + return uri.getPort(); + } + + if ("https".equalsIgnoreCase(uri.getScheme())) { + return 443; + } + + if ("http".equalsIgnoreCase(uri.getScheme())) { + return 80; + } + + return -1; + } + + /** + * Converts an absolute or relative URI into a relative redirect location. + * The returned value contains only the path, query, and fragment. + * + * @param uri the URI to normalize + * @return the relative redirect location + */ + private String toRelativeLocation(URI uri) { + String path = StringUtils.defaultIfEmpty(uri.getRawPath(), "/"); + + StringBuilder location = new StringBuilder(path); + + if (uri.getRawQuery() != null) { + location.append('?').append(uri.getRawQuery()); + } + + if (uri.getRawFragment() != null) { + location.append('#').append(uri.getRawFragment()); + } + + return location.toString(); + } + + /** + * Validates and normalizes a return URL. + * Only root-relative paths or absolute URLs matching the configured + * Gateway origin are accepted. Any invalid or untrusted value falls back + * to {@link #CONTEXT_PATH} to prevent open redirect attacks. + * + * @param candidate the return URL supplied by the client + * @return a safe redirect target + */ + private String sanitizeReturnUrl(String candidate) { + if (StringUtils.isBlank(candidate)) { + return CONTEXT_PATH; + } + + String lowercaseCandidate = candidate.toLowerCase(Locale.ROOT); + + // Backslashes can be interpreted as slashes by some clients. + // Also reject encoded backslashes and CR/LF characters, to avoid CRLF Injection. + if (candidate.contains("\\") + || lowercaseCandidate.contains("%5c") + || lowercaseCandidate.contains("%0d") + || lowercaseCandidate.contains("%0a")) { + return CONTEXT_PATH; + } + + try { + URI candidateUri = new URI(candidate); + + if (!candidateUri.isAbsolute()) { + return sanitizeRelativeUri(candidateUri); + } + + if (StringUtils.isBlank(externalUrl)) { + return CONTEXT_PATH; + } + URI configuredExternalUri = new URI(externalUrl); + + if (!isSameOrigin(candidateUri, configuredExternalUri)) { + return CONTEXT_PATH; + } + + return toRelativeLocation(candidateUri); + } catch (URISyntaxException | IllegalArgumentException e) { + return CONTEXT_PATH; + } + } + public Mono updateCookies(WebFilterExchange webFilterExchange, OAuth2AuthorizedClient oAuth2AuthorizedClient) { - webFilterExchange.getExchange().getResponse().addCookie(defaultCookieAttr(ResponseCookie.from(COOKIE_AUTH_NAME, oAuth2AuthorizedClient.getAccessToken().getTokenValue())).build()); - var location = webFilterExchange.getExchange().getRequest().getCookies().getFirst(COOKIE_RETURN_URL); + ServerWebExchange exchange = webFilterExchange.getExchange(); + + exchange.getResponse().addCookie(defaultCookieAttr(ResponseCookie.from(COOKIE_AUTH_NAME, oAuth2AuthorizedClient.getAccessToken().getTokenValue())).build()); + + var location = exchange.getRequest().getCookies().getFirst(COOKIE_RETURN_URL); + if (!HAS_NO_VALUE.test(location)) { - redirect(webFilterExchange.getExchange().getResponse(), location.getValue()); + redirect(exchange.getResponse(), sanitizeReturnUrl(location.getValue())); } clearCookies(webFilterExchange); return Mono.empty(); @@ -468,7 +616,7 @@ public Mono saveAuthorizationRequest(OAuth2AuthorizationRequest authorizat exchange.getResponse().addCookie( createCookie(COOKIE_NONCE, String.valueOf(authorizationRequest.getAttributes().get(OidcParameterNames.NONCE))) ); - exchange.getResponse().addCookie(createCookie(COOKIE_RETURN_URL, getReturnUrl(exchange))); + exchange.getResponse().addCookie(createCookie(COOKIE_RETURN_URL, getSafeReturnUrl(exchange))); exchange.getResponse().addCookie(createCookie(COOKIE_STATE, authorizationRequest.getState())); return Mono.empty(); } @@ -478,6 +626,15 @@ String getReturnUrl(ServerWebExchange exchange) { .orElse(exchange.getRequest().getHeaders().getFirst(HttpHeaders.ORIGIN)); } + /** + * Sanitize the return URL + * @param exchange the exchange + * @return the sanitized return URL + */ + private String getSafeReturnUrl(ServerWebExchange exchange) { + return sanitizeReturnUrl(getReturnUrl(exchange)); + } + @Override public Mono removeAuthorizationRequest(ServerWebExchange exchange) { Mono requestMono = loadAuthorizationRequest(exchange); diff --git a/gateway-service/src/test/java/org/zowe/apiml/gateway/config/WebSecurityTest.java b/gateway-service/src/test/java/org/zowe/apiml/gateway/config/WebSecurityTest.java index b578ff2e4f..8eca59c306 100644 --- a/gateway-service/src/test/java/org/zowe/apiml/gateway/config/WebSecurityTest.java +++ b/gateway-service/src/test/java/org/zowe/apiml/gateway/config/WebSecurityTest.java @@ -15,7 +15,9 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.MethodSource; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.beans.factory.annotation.Autowired; @@ -62,8 +64,10 @@ import java.net.InetSocketAddress; import java.util.HashMap; +import java.util.Objects; import java.util.Optional; import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -111,7 +115,7 @@ void shouldAddRegistryAuthorityToAllowedUser() { @Test void whenAccessTokenIsAvailable_thenAddItAsCookie() { var token = "thisIsToken"; - var location = "localhost:10010"; + var location = "/gateway/some/path"; var webFilterExchange = mock(WebFilterExchange.class); var exchange = mock(ServerWebExchange.class); when(webFilterExchange.getExchange()).thenReturn(exchange); @@ -130,9 +134,9 @@ void whenAccessTokenIsAvailable_thenAddItAsCookie() { when(accessToken.getTokenValue()).thenReturn(token); webSecurity.updateCookies(webFilterExchange, oAuth2AuthorizedClient); - assertEquals(token, serverHttpResponse.getCookies().getFirst("apimlAuthenticationToken").getValue()); - assertEquals("", serverHttpResponse.getCookies().getFirst(WebSecurity.COOKIE_NONCE).getValue()); - assertEquals("", serverHttpResponse.getCookies().getFirst(WebSecurity.COOKIE_STATE).getValue()); + assertEquals(token, Objects.requireNonNull(serverHttpResponse.getCookies().getFirst("apimlAuthenticationToken")).getValue()); + assertEquals("", Objects.requireNonNull(serverHttpResponse.getCookies().getFirst(WebSecurity.COOKIE_NONCE)).getValue()); + assertEquals("", Objects.requireNonNull(serverHttpResponse.getCookies().getFirst(WebSecurity.COOKIE_STATE)).getValue()); assertEquals(location, serverHttpResponse.getHeaders().getFirst(HttpHeaders.LOCATION)); } @@ -243,8 +247,6 @@ void oauth2WebFilterChain_whenClientConfigurationConfigured_shouldConfigureSecur ServerHttpSecurity.AuthorizeExchangeSpec authorizeExchangeSpec = mock(ServerHttpSecurity.AuthorizeExchangeSpec.class); ServerHttpSecurity.AuthorizeExchangeSpec.Access access = mock(ServerHttpSecurity.AuthorizeExchangeSpec.Access.class); - ServerHttpSecurity.OAuth2LoginSpec oauth2LoginSpec = mock(ServerHttpSecurity.OAuth2LoginSpec.class); - ServerHttpSecurity.HeaderSpec headerSpec = mock(ServerHttpSecurity.HeaderSpec.class); when(http.headers(any())).thenReturn(http); when(http.securityContextRepository(any())).thenReturn(http); @@ -486,8 +488,108 @@ void saveAuthorizationRequest_whenCalled_shouldSaveCookies () { requestRepository.saveAuthorizationRequest(oauth2AuthReq, exchange).block(); - assertThat(cookies.getFirst(WebSecurity.COOKIE_NONCE).getValue()).isEqualTo("test-nonce"); - assertThat(cookies.getFirst(WebSecurity.COOKIE_STATE).getValue()).isEqualTo("test-state"); + assertThat(Objects.requireNonNull(cookies.getFirst(WebSecurity.COOKIE_NONCE)).getValue()).isEqualTo("test-nonce"); + assertThat(Objects.requireNonNull(cookies.getFirst(WebSecurity.COOKIE_STATE)).getValue()).isEqualTo("test-state"); + + } + + @Nested + class SanitizeReturnUrl { + + private static final String EXTERNAL_URL = "https://gateway.zowe.example:10010"; + + WebSecurity.ApimlServerAuthorizationRequestRepository requestRepository; + + @BeforeEach + void setUp() { + var webSecurity = new WebSecurity(null, tokenProvider, basicAuthProvider, applicationContext); + ReflectionTestUtils.setField(webSecurity, "externalUrl", EXTERNAL_URL); + var resolver = mock(ServerOAuth2AuthorizationRequestResolver.class); + requestRepository = webSecurity.new ApimlServerAuthorizationRequestRepository(resolver); + } + + private String sanitize(String returnUrlQueryParam, String originHeader) { + var request = mock(ServerHttpRequest.class); + var headers = new HttpHeaders(); + if (originHeader != null) { + headers.set(HttpHeaders.ORIGIN, originHeader); + } + when(request.getHeaders()).thenReturn(headers); + + var queryParams = new LinkedMultiValueMap(); + if (returnUrlQueryParam != null) { + queryParams.add("returnUrl", returnUrlQueryParam); + } + when(request.getQueryParams()).thenReturn(queryParams); + + var exchange = mock(ServerWebExchange.class); + when(exchange.getRequest()).thenReturn(request); + var response = mock(ServerHttpResponse.class); + when(exchange.getResponse()).thenReturn(response); + + var cookies = new LinkedMultiValueMap(); + doAnswer(invocation -> { + var cookie = invocation.getArgument(0, ResponseCookie.class); + cookies.add(cookie.getName(), cookie); + return null; + }).when(response).addCookie(any(ResponseCookie.class)); + + var oauth2AuthReq = OAuth2AuthorizationRequest.authorizationCode() + .clientId("test-client") + .state("test-state") + .authorizationUri("https://test.auth.server/oauth/authorize") + .attributes(attrs -> attrs.put(OidcParameterNames.NONCE, "test-nonce")) + .build(); + + requestRepository.saveAuthorizationRequest(oauth2AuthReq, exchange).block(); + + return Objects.requireNonNull(cookies.getFirst(WebSecurity.COOKIE_RETURN_URL)).getValue(); + } + + static Stream safeAndUnsafeReturnUrls() { + return Stream.of( + // no returnUrl and no Origin header at all + Arguments.of(null, WebSecurity.CONTEXT_PATH), + Arguments.of("", WebSecurity.CONTEXT_PATH), + // plain relative paths are passed through untouched + Arguments.of("/gateway/foo", "/gateway/foo"), + Arguments.of("/gateway/foo?bar=1#frag", "/gateway/foo?bar=1#frag"), + // protocol-relative and backslash tricks that resolve to a different host + Arguments.of("//attacker.example/x", WebSecurity.CONTEXT_PATH), + Arguments.of("/\\attacker.example", WebSecurity.CONTEXT_PATH), + Arguments.of("/foo%5Cattacker.example", WebSecurity.CONTEXT_PATH), + Arguments.of("/foo%0D%0ASet-Cookie:evil=1", WebSecurity.CONTEXT_PATH), + // absolute URLs matching the configured externalUrl are accepted, authority stripped + Arguments.of(EXTERNAL_URL + "/gateway/foo?x=1#frag", "/gateway/foo?x=1#frag"), + Arguments.of(EXTERNAL_URL, "/"), + Arguments.of("HTTPS://GATEWAY.ZOWE.EXAMPLE:10010/gateway/foo", "/gateway/foo"), + // absolute URLs that differ from externalUrl in host, port, scheme or userinfo + Arguments.of("https://attacker.example/x", WebSecurity.CONTEXT_PATH), + Arguments.of("https://gateway.zowe.example:9999/x", WebSecurity.CONTEXT_PATH), + Arguments.of("http://gateway.zowe.example:10010/x", WebSecurity.CONTEXT_PATH), + Arguments.of("https://user@gateway.zowe.example:10010/x", WebSecurity.CONTEXT_PATH), + // non-http(s) schemes and malformed URIs + Arguments.of("javascript:alert(1)", WebSecurity.CONTEXT_PATH), + Arguments.of("mailto:foo@bar.com", WebSecurity.CONTEXT_PATH), + Arguments.of("http://exa mple.com", WebSecurity.CONTEXT_PATH) + ); + } + + @ParameterizedTest + @MethodSource("safeAndUnsafeReturnUrls") + void sanitizesReturnUrlQueryParam(String returnUrl, String expected) { + assertThat(sanitize(returnUrl, null)).isEqualTo(expected); + } + + @Test + void whenNoReturnUrlParam_thenFallsBackToOriginHeader_sameOrigin() { + assertThat(sanitize(null, EXTERNAL_URL)).isEqualTo("/"); + } + + @Test + void whenNoReturnUrlParam_thenFallsBackToOriginHeader_crossOrigin() { + assertThat(sanitize(null, "https://attacker.example")).isEqualTo(WebSecurity.CONTEXT_PATH); + } } @@ -500,7 +602,7 @@ class ApimlStrictServerWebExchangeFirewall { @Mock private StrictServerWebExchangeFirewall nonRoutingFirewall; private WebSecurity.ApimlStrictServerWebExchangeFirewall apimlStrictServerWebExchangeFirewall; - private ApplicationInfo applicationInfo = ApplicationInfo.builder().build(); + private final ApplicationInfo applicationInfo = ApplicationInfo.builder().build(); @BeforeEach void setUp() {