SONAR-17694 fix SSF-323

4 files changed, 68 insertions, 8 deletions

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 4b58458513c..3b1c5e37a86 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
@@ -34,7 +34,9 @@ import org.sonar.auth.saml.SamlAuthenticator;
 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.SamlValidationRedirectionFilter;
+import org.sonar.server.authentication.event.AuthenticationException;
 import org.sonar.server.user.ThreadLocalUserSession;
 import org.sonar.server.ws.ServletFilterHandler;
 
@@ -44,11 +46,16 @@ public class ValidationAction extends ServletFilter implements SamlAction {
   private final ThreadLocalUserSession userSession;
   private final SamlAuthenticator samlAuthenticator;
   private final OAuth2ContextFactory oAuth2ContextFactory;
+  private final SamlIdentityProvider samlIdentityProvider;
+  private final OAuthCsrfVerifier oAuthCsrfVerifier;
 
-  public ValidationAction(ThreadLocalUserSession userSession, SamlAuthenticator samlAuthenticator, OAuth2ContextFactory oAuth2ContextFactory) {
+  public ValidationAction(ThreadLocalUserSession userSession, SamlAuthenticator samlAuthenticator, OAuth2ContextFactory oAuth2ContextFactory,
+    SamlIdentityProvider samlIdentityProvider, OAuthCsrfVerifier oAuthCsrfVerifier) {
     this.samlAuthenticator = samlAuthenticator;
     this.userSession = userSession;
     this.oAuth2ContextFactory = oAuth2ContextFactory;
+    this.samlIdentityProvider = samlIdentityProvider;
+    this.oAuthCsrfVerifier = oAuthCsrfVerifier;
   }
 
   @Override
@@ -60,6 +67,14 @@ public class ValidationAction extends ServletFilter implements SamlAction {
   public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
     HttpServletResponse httpResponse = (HttpServletResponse) response;
     HttpServletRequest httpRequest = (HttpServletRequest) request;
+
+    try {
+      oAuthCsrfVerifier.verifyState(httpRequest, httpResponse, samlIdentityProvider, "CSRFToken");
+    } catch (AuthenticationException exception) {
+      AuthenticationError.handleError(httpRequest, httpResponse, exception.getMessage());
+      return;
+    }
+
     if (!userSession.hasSession() || !userSession.isSystemAdministrator()) {
       AuthenticationError.handleError(httpRequest, httpResponse, "User needs to be logged in as system administrator to access this page.");
       return;
diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationInitAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationInitAction.java
index 4181fe6d83f..d25122d5ea8 100644
--- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationInitAction.java
+++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationInitAction.java
@@ -32,6 +32,7 @@ import org.sonar.auth.saml.SamlAuthenticator;
 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.exceptions.ForbiddenException;
 import org.sonar.server.user.UserSession;
 import org.sonar.server.ws.ServletFilterHandler;
@@ -44,12 +45,13 @@ public class ValidationInitAction extends ServletFilter implements SamlAction {
   public static final String VALIDATION_RELAY_STATE = "validation-query";
   public static final String VALIDATION_INIT_KEY = "validation_init";
   private final SamlAuthenticator samlAuthenticator;
-
+  private final OAuthCsrfVerifier oAuthCsrfVerifier;
   private final OAuth2ContextFactory oAuth2ContextFactory;
   private final UserSession userSession;
 
-  public ValidationInitAction(SamlAuthenticator samlAuthenticator, OAuth2ContextFactory oAuth2ContextFactory, UserSession userSession) {
+  public ValidationInitAction(SamlAuthenticator samlAuthenticator, OAuthCsrfVerifier oAuthCsrfVerifier, OAuth2ContextFactory oAuth2ContextFactory, UserSession userSession) {
     this.samlAuthenticator = samlAuthenticator;
+    this.oAuthCsrfVerifier = oAuthCsrfVerifier;
     this.oAuth2ContextFactory = oAuth2ContextFactory;
     this.userSession = userSession;
   }
@@ -82,9 +84,11 @@ public class ValidationInitAction extends ServletFilter implements SamlAction {
       return;
     }
 
+    String csrfState = oAuthCsrfVerifier.generateState(request,response);
+
     try {
       samlAuthenticator.initLogin(oAuth2ContextFactory.generateCallbackUrl(SamlIdentityProvider.KEY),
-        VALIDATION_RELAY_STATE, request, response);
+        VALIDATION_RELAY_STATE + "/" + csrfState, request, response);
     } catch (IllegalStateException e) {
       response.sendRedirect("/" + SAML_VALIDATION_CONTROLLER_CONTEXT + "/" + SAML_VALIDATION_KEY);
     }
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 a00371db1d6..beb73abc605 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
@@ -31,12 +31,17 @@ import org.junit.Before;
 import org.junit.Test;
 import org.sonar.api.server.ws.WebService;
 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.event.AuthenticationEvent;
+import org.sonar.server.authentication.event.AuthenticationException;
 import org.sonar.server.user.ThreadLocalUserSession;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.ArgumentMatchers.any;
 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;
@@ -48,12 +53,18 @@ public class ValidationActionTest {
   private SamlAuthenticator samlAuthenticator;
   private ThreadLocalUserSession userSession;
 
+  private OAuthCsrfVerifier oAuthCsrfVerifier;
+
+  private SamlIdentityProvider samlIdentityProvider;
+
   @Before
   public void setup() {
     samlAuthenticator = mock(SamlAuthenticator.class);
     userSession = mock(ThreadLocalUserSession.class);
+    oAuthCsrfVerifier = mock(OAuthCsrfVerifier.class);
+    samlIdentityProvider = mock(SamlIdentityProvider.class);
     var oAuth2ContextFactory = mock(OAuth2ContextFactory.class);
-    underTest = new ValidationAction(userSession, samlAuthenticator, oAuth2ContextFactory);
+    underTest = new ValidationAction(userSession, samlAuthenticator, oAuth2ContextFactory, samlIdentityProvider, oAuthCsrfVerifier);
   }
 
   @Test
@@ -99,6 +110,27 @@ public class ValidationActionTest {
   }
 
   @Test
+  public void do_filter_failed_csrf_verification() throws ServletException, IOException {
+    HttpServletRequest servletRequest = spy(HttpServletRequest.class);
+    HttpServletResponse servletResponse = mock(HttpServletResponse.class);
+    StringWriter stringWriter = new StringWriter();
+    doReturn(new PrintWriter(stringWriter)).when(servletResponse).getWriter();
+    FilterChain filterChain = mock(FilterChain.class);
+
+    doReturn("IdentityProviderName").when(samlIdentityProvider).getName();
+    doThrow(AuthenticationException.newBuilder()
+      .setSource(AuthenticationEvent.Source.oauth2(samlIdentityProvider))
+      .setMessage("Cookie is missing").build()).when(oAuthCsrfVerifier).verifyState(any(),any(),any(), any());
+
+    doReturn(true).when(userSession).hasSession();
+    doReturn(true).when(userSession).isSystemAdministrator();
+
+    underTest.doFilter(servletRequest, servletResponse, filterChain);
+
+    verifyNoInteractions(samlAuthenticator);
+  }
+
+  @Test
   public void verify_definition() {
     String controllerKey = "foo";
     WebService.Context context = new WebService.Context();
diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/saml/ws/ValidationInitActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/saml/ws/ValidationInitActionTest.java
index c284e4defec..a12e56dbb66 100644
--- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/saml/ws/ValidationInitActionTest.java
+++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/saml/ws/ValidationInitActionTest.java
@@ -30,6 +30,7 @@ import org.junit.Test;
 import org.sonar.api.server.ws.WebService;
 import org.sonar.auth.saml.SamlAuthenticator;
 import org.sonar.server.authentication.OAuth2ContextFactory;
+import org.sonar.server.authentication.OAuthCsrfVerifier;
 import org.sonar.server.tester.UserSessionRule;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -48,12 +49,14 @@ public class ValidationInitActionTest {
   private ValidationInitAction underTest;
   private SamlAuthenticator samlAuthenticator;
   private OAuth2ContextFactory oAuth2ContextFactory;
+  private OAuthCsrfVerifier oAuthCsrfVerifier;
 
   @Before
   public void setUp() throws Exception {
     samlAuthenticator = mock(SamlAuthenticator.class);
     oAuth2ContextFactory = mock(OAuth2ContextFactory.class);
-    underTest = new ValidationInitAction(samlAuthenticator, oAuth2ContextFactory, userSession);
+    oAuthCsrfVerifier = mock(OAuthCsrfVerifier.class);
+    underTest = new ValidationInitAction(samlAuthenticator, oAuthCsrfVerifier, oAuth2ContextFactory, userSession);
   }
 
   @Test
@@ -71,8 +74,9 @@ public class ValidationInitActionTest {
     HttpServletResponse servletResponse = mock(HttpServletResponse.class);
     FilterChain filterChain = mock(FilterChain.class);
     String callbackUrl = "http://localhost:9000/api/validation_test";
-    when(oAuth2ContextFactory.generateCallbackUrl(anyString()))
-      .thenReturn(callbackUrl);
+
+    mockCsrfTokenGeneration(servletRequest, servletResponse);
+    when(oAuth2ContextFactory.generateCallbackUrl(anyString())).thenReturn(callbackUrl);
 
     underTest.doFilter(servletRequest, servletResponse, filterChain);
 
@@ -91,6 +95,7 @@ public class ValidationInitActionTest {
     when(oAuth2ContextFactory.generateCallbackUrl(anyString()))
       .thenReturn(callbackUrl);
 
+    mockCsrfTokenGeneration(servletRequest, servletResponse);
     doThrow(new IllegalStateException()).when(samlAuthenticator).initLogin(any(), any(), any(), any());
 
     underTest.doFilter(servletRequest, servletResponse, filterChain);
@@ -143,4 +148,8 @@ public class ValidationInitActionTest {
     assertThat(validationInitAction.description()).isNotEmpty();
     assertThat(validationInitAction.handler()).isNotNull();
   }
+
+  private void mockCsrfTokenGeneration(HttpServletRequest servletRequest, HttpServletResponse servletResponse) {
+    when(oAuthCsrfVerifier.generateState(servletRequest, servletResponse)).thenReturn("CSRF_TOKEN");
+  }
 }