From e81ae5d200c7662160090e9c104873d543a374a6 Mon Sep 17 00:00:00 2001 From: Steve Marion Date: Fri, 7 Apr 2023 10:43:20 +0200 Subject: [PATCH] [SONAR-18809] replace hash by nonce for CSP headers --- .../src/main/resources/samlAuthResult.html | 2 +- .../saml/SamlAuthStatusPageGeneratorTest.java | 24 +- .../test/resources/samlAuthResultEmpty.html | 216 ------------------ .../SamlValidationCspHeaders.java | 29 +-- .../SamlValidationRedirectionFilter.java | 9 +- .../resources/validation-redirection.html | 4 +- .../SamlValidationCspHeadersTest.java | 67 +----- .../SamlValidationRedirectionFilterTest.java | 24 +- .../server/saml/ws/ValidationAction.java | 6 +- .../server/saml/ws/ValidationActionTest.java | 70 +----- 10 files changed, 53 insertions(+), 398 deletions(-) delete mode 100644 server/sonar-auth-saml/src/test/resources/samlAuthResultEmpty.html diff --git a/server/sonar-auth-saml/src/main/resources/samlAuthResult.html b/server/sonar-auth-saml/src/main/resources/samlAuthResult.html index a49503aa659..c7cf2554b3a 100644 --- a/server/sonar-auth-saml/src/main/resources/samlAuthResult.html +++ b/server/sonar-auth-saml/src/main/resources/samlAuthResult.html @@ -116,7 +116,7 @@
- - - diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/SamlValidationCspHeaders.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/SamlValidationCspHeaders.java index 2735f4b87ec..863449a733a 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/SamlValidationCspHeaders.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/SamlValidationCspHeaders.java @@ -19,47 +19,38 @@ */ package org.sonar.server.authentication; -import java.util.Base64; +import java.math.BigInteger; +import java.security.SecureRandom; import java.util.List; -import java.util.regex.Matcher; -import java.util.regex.Pattern; import javax.servlet.http.HttpServletResponse; -import org.apache.commons.codec.digest.DigestUtils; public class SamlValidationCspHeaders { - private static final Pattern SCRIPT_PATTERN = Pattern.compile("(?<=)"); - private SamlValidationCspHeaders() { throw new IllegalStateException("Utility class, cannot be instantiated"); } - public static void addCspHeadersToResponse(HttpServletResponse httpResponse, String hash) { + public static String addCspHeadersWithNonceToResponse(HttpServletResponse httpResponse) { + final String nonce = getNonce(); + List cspPolicies = List.of( "default-src 'self'", "base-uri 'none'", "connect-src 'self' http: https:", "img-src * data: blob:", "object-src 'none'", - "script-src 'self' '" + hash + "'", + "script-src 'nonce-" + nonce + "'", "style-src 'self' 'unsafe-inline'", "worker-src 'none'"); String policies = String.join("; ", cspPolicies).trim(); List cspHeaders = List.of("Content-Security-Policy", "X-Content-Security-Policy", "X-WebKit-CSP"); cspHeaders.forEach(header -> httpResponse.setHeader(header, policies)); + return nonce; } - public static String getHashForInlineScript(String html) { - Matcher matcher = SCRIPT_PATTERN.matcher(html); - if (matcher.find()) { - return getBase64Sha256(matcher.group(0)); - } - return ""; + private static String getNonce() { + // this code is the same as in org.sonar.server.authentication.JwtCsrfVerifier.generateState + return new BigInteger(130, new SecureRandom()).toString(32); } - - private static String getBase64Sha256(String string) { - return "sha256-" + Base64.getEncoder().encodeToString(DigestUtils.sha256(string)); - } - } diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/SamlValidationRedirectionFilter.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/SamlValidationRedirectionFilter.java index 7298e30e3df..a0d9e55b326 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/SamlValidationRedirectionFilter.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/SamlValidationRedirectionFilter.java @@ -37,8 +37,6 @@ import org.sonar.api.platform.Server; import org.sonar.api.web.ServletFilter; import static org.sonar.server.authentication.AuthenticationFilter.CALLBACK_PATH; -import static org.sonar.server.authentication.SamlValidationCspHeaders.addCspHeadersToResponse; -import static org.sonar.server.authentication.SamlValidationCspHeaders.getHashForInlineScript; public class SamlValidationRedirectionFilter extends ServletFilter { @@ -88,12 +86,13 @@ public class SamlValidationRedirectionFilter extends ServletFilter { String samlResponse = StringEscapeUtils.escapeHtml(request.getParameter(SAML_RESPONSE_PARAMETER)); String csrfToken = getCsrfTokenFromRelayState(relayState); + String nonce = SamlValidationCspHeaders.addCspHeadersWithNonceToResponse(httpResponse); + String template = StringUtils.replaceEachRepeatedly(redirectionPageTemplate, - new String[]{"%WEB_CONTEXT%", "%VALIDATION_URL%", "%SAML_RESPONSE%", "%CSRF_TOKEN%"}, - new String[]{server.getContextPath(), redirectionEndpointUrl.toString(), samlResponse, csrfToken}); + new String[]{"%NONCE%", "%WEB_CONTEXT%", "%VALIDATION_URL%", "%SAML_RESPONSE%", "%CSRF_TOKEN%"}, + new String[]{nonce, server.getContextPath(), redirectionEndpointUrl.toString(), samlResponse, csrfToken}); httpResponse.setContentType("text/html"); - addCspHeadersToResponse(httpResponse, getHashForInlineScript(template)); httpResponse.getWriter().print(template); return; } diff --git a/server/sonar-webserver-auth/src/main/resources/validation-redirection.html b/server/sonar-webserver-auth/src/main/resources/validation-redirection.html index f3060136404..518c6f16505 100644 --- a/server/sonar-webserver-auth/src/main/resources/validation-redirection.html +++ b/server/sonar-webserver-auth/src/main/resources/validation-redirection.html @@ -14,7 +14,7 @@ - diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/SamlValidationCspHeadersTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/SamlValidationCspHeadersTest.java index 9765357cba9..2ca3c6a6ab6 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/SamlValidationCspHeadersTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/SamlValidationCspHeadersTest.java @@ -22,73 +22,22 @@ package org.sonar.server.authentication; import javax.servlet.http.HttpServletResponse; import org.junit.Test; -import static org.junit.Assert.*; +import static org.mockito.ArgumentMatchers.contains; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; -import static org.sonar.server.authentication.SamlValidationCspHeaders.addCspHeadersToResponse; -import static org.sonar.server.authentication.SamlValidationCspHeaders.getHashForInlineScript; +import static org.sonar.server.authentication.SamlValidationCspHeaders.addCspHeadersWithNonceToResponse; public class SamlValidationCspHeadersTest { @Test - public void CspHeaders_are_correctly_added_to_response() { + public void addCspHeadersWithNonceToResponse_whenCalled_shouldAddNonceToCspHeaders() { HttpServletResponse httpServletResponse = mock(HttpServletResponse.class); - addCspHeadersToResponse(httpServletResponse, "hash"); - verify(httpServletResponse).setHeader("Content-Security-Policy", "default-src 'self'; base-uri 'none'; connect-src 'self' http: https:; img-src * data: blob:; object-src 'none'; script-src 'self' 'hash'; style-src 'self' 'unsafe-inline'; worker-src 'none'"); - verify(httpServletResponse).setHeader("X-Content-Security-Policy", "default-src 'self'; base-uri 'none'; connect-src 'self' http: https:; img-src * data: blob:; object-src 'none'; script-src 'self' 'hash'; style-src 'self' 'unsafe-inline'; worker-src 'none'"); - verify(httpServletResponse).setHeader("X-WebKit-CSP", "default-src 'self'; base-uri 'none'; connect-src 'self' http: https:; img-src * data: blob:; object-src 'none'; script-src 'self' 'hash'; style-src 'self' 'unsafe-inline'; worker-src 'none'"); - } - - @Test - public void hash_is_properly_calculated_for_an_inline_script() { - String hash = getHashForInlineScript(getBasicHtmlWithScript()); - assertEquals("sha256-jRoPhEx/vXxIUUkuTwJJ2ww4OPlo7B2ZK/wDVC4IXUs=", hash); - } - - @Test - public void hash_is_empty_when_no_inline_script_available() { - String hash = getHashForInlineScript(getBasicHtmlWithoutScript()); - assertEquals("", hash); - } - - private String getBasicHtmlWithScript() { - return """ - - - - SAML Authentication Test - - - - - - """; - } + String nonce = addCspHeadersWithNonceToResponse(httpServletResponse); - private String getBasicHtmlWithoutScript() { - return """ - - - - SAML Authentication Test - - -
-

SAML Authentication Test

-
-
-
-
-
- - - """; + verify(httpServletResponse).setHeader(eq("Content-Security-Policy"), contains("script-src 'nonce-" + nonce + "'")); + verify(httpServletResponse).setHeader(eq("X-Content-Security-Policy"), contains("script-src 'nonce-" + nonce + "'")); + verify(httpServletResponse).setHeader(eq("X-WebKit-CSP"), contains("script-src 'nonce-" + nonce + "'")); } } diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/SamlValidationRedirectionFilterTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/SamlValidationRedirectionFilterTest.java index 38e8ea8cefb..bfa90a614ee 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/SamlValidationRedirectionFilterTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/SamlValidationRedirectionFilterTest.java @@ -24,6 +24,7 @@ import com.tngtech.java.junit.dataprovider.DataProviderRunner; import com.tngtech.java.junit.dataprovider.UseDataProvider; import java.io.IOException; import java.io.PrintWriter; +import java.util.List; import javax.servlet.FilterChain; import javax.servlet.FilterConfig; import javax.servlet.ServletException; @@ -33,10 +34,13 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; +import org.mockito.internal.verification.VerificationModeFactory; import org.sonar.api.platform.Server; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertThrows; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.matches; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; @@ -47,6 +51,8 @@ import static org.mockito.Mockito.when; @RunWith(DataProviderRunner.class) public class SamlValidationRedirectionFilterTest { + public static final List CSP_HEADERS = List.of("Content-Security-Policy", "X-Content-Security-Policy", "X-WebKit-CSP"); + SamlValidationRedirectionFilter underTest; @Before @@ -57,6 +63,7 @@ public class SamlValidationRedirectionFilterTest { underTest.init(mock(FilterConfig.class)); } + @Test public void do_get_pattern() { assertThat(underTest.doGetPattern().matches("/oauth2/callback/saml")).isTrue(); @@ -82,7 +89,7 @@ public class SamlValidationRedirectionFilterTest { ArgumentCaptor htmlProduced = ArgumentCaptor.forClass(String.class); verify(pw).print(htmlProduced.capture()); - verifyResponseContentTypeAndCSPHeaders(servletResponse, "sha256-TClpsoWi64Z74Xuk4Fa3bdt7mY/7K+A2jHOgNpxDy2I="); + CSP_HEADERS.forEach(h -> verify(servletResponse).setHeader(eq(h), anyString())); assertThat(htmlProduced.getValue()).contains(validSample); assertThat(htmlProduced.getValue()).contains("action=\"contextPath/saml/validation\""); assertThat(htmlProduced.getValue()).contains("value=\"CSRF_TOKEN\""); @@ -107,7 +114,7 @@ public class SamlValidationRedirectionFilterTest { ArgumentCaptor htmlProduced = ArgumentCaptor.forClass(String.class); verify(pw).print(htmlProduced.capture()); - verifyResponseContentTypeAndCSPHeaders(servletResponse, "sha256-TClpsoWi64Z74Xuk4Fa3bdt7mY/7K+A2jHOgNpxDy2I="); + CSP_HEADERS.forEach(h -> verify(servletResponse).setHeader(eq(h), anyString())); assertThat(htmlProduced.getValue()).contains(validSample); assertThat(htmlProduced.getValue()).doesNotContain(""); @@ -129,9 +136,8 @@ public class SamlValidationRedirectionFilterTest { underTest.doFilter(servletRequest, servletResponse, filterChain); ArgumentCaptor htmlProduced = ArgumentCaptor.forClass(String.class); - verify(pw).print(htmlProduced.capture()); - verifyResponseContentTypeAndCSPHeaders(servletResponse, "sha256-TClpsoWi64Z74Xuk4Fa3bdt7mY/7K+A2jHOgNpxDy2I="); + CSP_HEADERS.forEach(h -> verify(servletResponse).setHeader(eq(h), anyString())); assertThat(htmlProduced.getValue()).doesNotContain(""); assertThat(htmlProduced.getValue()).contains("action=\"contextPath/saml/validation\""); } @@ -142,8 +148,8 @@ public class SamlValidationRedirectionFilterTest { HttpServletRequest servletRequest = mock(HttpServletRequest.class); HttpServletResponse servletResponse = mock(HttpServletResponse.class); FilterChain filterChain = mock(FilterChain.class); - doReturn(relayStateValue).when(servletRequest).getParameter("RelayState"); + underTest.doFilter(servletRequest, servletResponse, filterChain); verifyNoInteractions(servletResponse); @@ -158,12 +164,4 @@ public class SamlValidationRedirectionFilterTest { public static Object[] invalidRelayStateValues() { return new Object[]{"random_query", "validation-query", null}; } - - private static void verifyResponseContentTypeAndCSPHeaders(HttpServletResponse servletResponse, String hash) { - verify(servletResponse).setContentType("text/html"); - verify(servletResponse).setHeader("Content-Security-Policy", "default-src 'self'; base-uri 'none'; connect-src 'self' http: https:; img-src * data: blob:; object-src 'none'; script-src 'self' '" + hash + "'; style-src 'self' 'unsafe-inline'; worker-src 'none'"); - verify(servletResponse).setHeader("X-Content-Security-Policy", "default-src 'self'; base-uri 'none'; connect-src 'self' http: https:; img-src * data: blob:; object-src 'none'; script-src 'self' '" + hash + "'; style-src 'self' 'unsafe-inline'; worker-src 'none'"); - verify(servletResponse).setHeader("X-WebKit-CSP", "default-src 'self'; base-uri 'none'; connect-src 'self' http: https:; img-src * data: blob:; object-src 'none'; script-src 'self' '" + hash + "'; style-src 'self' 'unsafe-inline'; worker-src 'none'"); - } - } diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationAction.java index 98238ab0b40..c55a99f37f7 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationAction.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationAction.java @@ -36,13 +36,12 @@ import org.sonar.auth.saml.SamlIdentityProvider; import org.sonar.server.authentication.AuthenticationError; import org.sonar.server.authentication.OAuth2ContextFactory; import org.sonar.server.authentication.OAuthCsrfVerifier; +import org.sonar.server.authentication.SamlValidationCspHeaders; import org.sonar.server.authentication.SamlValidationRedirectionFilter; import org.sonar.server.authentication.event.AuthenticationException; import org.sonar.server.user.ThreadLocalUserSession; import org.sonar.server.ws.ServletFilterHandler; -import static org.sonar.server.authentication.SamlValidationCspHeaders.addCspHeadersToResponse; -import static org.sonar.server.authentication.SamlValidationCspHeaders.getHashForInlineScript; import static org.sonar.server.saml.ws.SamlValidationWs.SAML_VALIDATION_CONTROLLER; public class ValidationAction extends ServletFilter implements SamlAction { @@ -96,7 +95,8 @@ public class ValidationAction extends ServletFilter implements SamlAction { httpResponse.setContentType("text/html"); String htmlResponse = samlAuthenticator.getAuthenticationStatusPage(httpRequest, httpResponse); - addCspHeadersToResponse(httpResponse, getHashForInlineScript(htmlResponse)); + String nonce = SamlValidationCspHeaders.addCspHeadersWithNonceToResponse(httpResponse); + htmlResponse = htmlResponse.replace("%NONCE%", nonce); httpResponse.getWriter().print(htmlResponse); } diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/saml/ws/ValidationActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/saml/ws/ValidationActionTest.java index 405b35932b6..659f9dc956a 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/saml/ws/ValidationActionTest.java +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/saml/ws/ValidationActionTest.java @@ -22,6 +22,7 @@ package org.sonar.server.saml.ws; import java.io.IOException; import java.io.PrintWriter; import java.io.StringWriter; +import java.util.List; import javax.servlet.FilterChain; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; @@ -33,6 +34,7 @@ import org.sonar.auth.saml.SamlAuthenticator; import org.sonar.auth.saml.SamlIdentityProvider; import org.sonar.server.authentication.OAuth2ContextFactory; import org.sonar.server.authentication.OAuthCsrfVerifier; +import org.sonar.server.authentication.SamlValidationCspHeaders; import org.sonar.server.authentication.event.AuthenticationEvent; import org.sonar.server.authentication.event.AuthenticationException; import org.sonar.server.user.ThreadLocalUserSession; @@ -40,23 +42,23 @@ import org.sonar.server.user.ThreadLocalUserSession; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertEquals; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; -import static org.sonar.server.authentication.SamlValidationCspHeaders.getHashForInlineScript; public class ValidationActionTest { private ValidationAction underTest; private SamlAuthenticator samlAuthenticator; private ThreadLocalUserSession userSession; - private OAuthCsrfVerifier oAuthCsrfVerifier; - private SamlIdentityProvider samlIdentityProvider; + public static final List CSP_HEADERS = List.of("Content-Security-Policy", "X-Content-Security-Policy", "X-WebKit-CSP"); @Before public void setup() { @@ -68,6 +70,7 @@ public class ValidationActionTest { underTest = new ValidationAction(userSession, samlAuthenticator, oAuth2ContextFactory, samlIdentityProvider, oAuthCsrfVerifier); } + @Test public void do_get_pattern() { assertThat(underTest.doGetPattern().matches("/saml/validation")).isTrue(); @@ -87,14 +90,15 @@ public class ValidationActionTest { doReturn(true).when(userSession).hasSession(); doReturn(true).when(userSession).isSystemAdministrator(); - doReturn(getBasicHtmlWithScript()).when(samlAuthenticator).getAuthenticationStatusPage(any(), any()); + final String mockedHtmlContent = "mocked html content"; + doReturn(mockedHtmlContent).when(samlAuthenticator).getAuthenticationStatusPage(any(), any()); underTest.doFilter(servletRequest, servletResponse, filterChain); verify(samlAuthenticator).getAuthenticationStatusPage(any(), any()); verify(servletResponse).getWriter(); - verifyResponseTypeAndCSPHeaders(servletResponse, getHashForInlineScript(getBasicHtmlWithScript())); - assertEquals(stringWriter.toString(), getBasicHtmlWithScript()); + CSP_HEADERS.forEach(h -> verify(servletResponse).setHeader(eq(h), anyString())); + assertEquals(mockedHtmlContent, stringWriter.toString()); } @Test @@ -148,58 +152,4 @@ public class ValidationActionTest { assertThat(validationInitAction.description()).isNotEmpty(); assertThat(validationInitAction.handler()).isNotNull(); } - - private static void verifyResponseTypeAndCSPHeaders(HttpServletResponse servletResponse, String hash) { - verify(servletResponse).setContentType("text/html"); - verify(servletResponse).setHeader("Content-Security-Policy", "default-src 'self'; base-uri 'none'; connect-src 'self' http: https:; img-src * data: blob:; object-src 'none'; script-src 'self' '" + hash + "'; style-src 'self' 'unsafe-inline'; worker-src 'none'"); - verify(servletResponse).setHeader("X-Content-Security-Policy", "default-src 'self'; base-uri 'none'; connect-src 'self' http: https:; img-src * data: blob:; object-src 'none'; script-src 'self' '" + hash + "'; style-src 'self' 'unsafe-inline'; worker-src 'none'"); - verify(servletResponse).setHeader("X-WebKit-CSP", "default-src 'self'; base-uri 'none'; connect-src 'self' http: https:; img-src * data: blob:; object-src 'none'; script-src 'self' '" + hash + "'; style-src 'self' 'unsafe-inline'; worker-src 'none'"); - } - - private String getBasicHtmlWithScript() { - return """ - - - - - - - - - SAML Authentication Test - - - - - -
-

SAML Authentication Test

-
-
-
-
-
- - - - - """; - } } -- 2.39.5