SONAR-16181 fix SSF-227

4 files changed, 96 insertions, 19 deletions

diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsAuthentication.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsAuthentication.java
index edf7bfed1be..5654f0a74a3 100644
--- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsAuthentication.java
+++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsAuthentication.java
@@ -67,10 +67,12 @@ public class CredentialsAuthentication {
     if (externalUser.isPresent()) {
       return externalUser.get();
     }
+    localAuthentication.generateHashToAvoidEnumerationAttack();
     throw AuthenticationException.newBuilder()
       .setSource(Source.local(method))
       .setLogin(credentials.getLogin())
       .setMessage(localUser != null && !localUser.isLocal() ? "User is not local" : "No active user for login")
       .build();
   }
+
 }
diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java
index 398d79779a9..757dd01075a 100644
--- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java
+++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java
@@ -28,6 +28,7 @@ import javax.annotation.Nullable;
 import javax.crypto.SecretKeyFactory;
 import javax.crypto.spec.PBEKeySpec;
 import org.apache.commons.codec.digest.DigestUtils;
+import org.apache.commons.lang.RandomStringUtils;
 import org.mindrot.jbcrypt.BCrypt;
 import org.sonar.api.config.Configuration;
 import org.sonar.db.DbClient;
@@ -46,15 +47,16 @@ import static java.util.Objects.requireNonNull;
  * database).
  */
 public class CredentialsLocalAuthentication {
-  private static final SecureRandom SECURE_RANDOM = new SecureRandom();
-  private static final HashMethod DEFAULT = HashMethod.PBKDF2;
-  private static final String PBKDF2_ITERATIONS_PROP = "sonar.internal.pbkdf2.iterations";
   public static final String ERROR_NULL_HASH_METHOD = "null hash method";
   public static final String ERROR_NULL_PASSWORD_IN_DB = "null password in DB";
   public static final String ERROR_NULL_SALT = "null salt";
   public static final String ERROR_WRONG_PASSWORD = "wrong password";
   public static final String ERROR_PASSWORD_CANNOT_BE_NULL = "Password cannot be null";
   public static final String ERROR_UNKNOWN_HASH_METHOD = "Unknown hash method [%s]";
+  private static final SecureRandom SECURE_RANDOM = new SecureRandom();
+  private static final String PBKDF2_ITERATIONS_PROP = "sonar.internal.pbkdf2.iterations";
+  private static final HashMethod DEFAULT = HashMethod.PBKDF2;
+  private static final int DUMMY_PASSWORD_AND_SALT_SIZE = 100;
 
   private final DbClient dbClient;
   private final EnumMap<HashMethod, HashFunction> hashFunctions = new EnumMap<>(HashMethod.class);
@@ -68,7 +70,12 @@ public class CredentialsLocalAuthentication {
     hashFunctions.put(HashMethod.BCRYPT, new BcryptFunction());
     hashFunctions.put(HashMethod.SHA1, new Sha1Function());
     hashFunctions.put(HashMethod.PBKDF2, new PBKDF2Function(configuration.getInt(PBKDF2_ITERATIONS_PROP).orElse(null)));
+  }
 
+  void generateHashToAvoidEnumerationAttack(){
+    String randomSalt = RandomStringUtils.randomAlphabetic(DUMMY_PASSWORD_AND_SALT_SIZE);
+    String randomPassword = RandomStringUtils.randomAlphabetic(DUMMY_PASSWORD_AND_SALT_SIZE);
+    hashFunctions.get(HashMethod.PBKDF2).encryptPassword(randomSalt, randomPassword);
   }
 
   /**
@@ -90,6 +97,7 @@ public class CredentialsLocalAuthentication {
     try {
       hashMethod = HashMethod.valueOf(user.getHashMethod());
     } catch (IllegalArgumentException ex) {
+      generateHashToAvoidEnumerationAttack();
       throw AuthenticationException.newBuilder()
         .setSource(Source.local(method))
         .setLogin(user.getLogin())
@@ -156,6 +164,10 @@ public class CredentialsLocalAuthentication {
     AuthenticationResult checkCredentials(UserDto user, String password);
 
     void storeHashPassword(UserDto user, String password);
+
+    default String encryptPassword(String salt, String password) {
+      throw new IllegalStateException("This method is not supported for this hash function");
+    }
   }
 
   /**
@@ -193,15 +205,16 @@ public class CredentialsLocalAuthentication {
     }
   }
 
-  private static final class PBKDF2Function implements HashFunction {
+  static final class PBKDF2Function implements HashFunction {
+    private static final char ITERATIONS_HASH_SEPARATOR = '$';
     private static final int DEFAULT_ITERATIONS = 100_000;
     private static final String ALGORITHM = "PBKDF2WithHmacSHA512";
     private static final int KEY_LEN = 512;
-    public static final String ERROR_INVALID_HASH_STORED = "invalid hash stored";
-    private final int gen_iterations;
+    private static final String ERROR_INVALID_HASH_STORED = "invalid hash stored";
+    private final int generationIterations;
 
-    public PBKDF2Function(@Nullable Integer gen_iterations) {
-      this.gen_iterations = gen_iterations != null ? gen_iterations : DEFAULT_ITERATIONS;
+    public PBKDF2Function(@Nullable Integer generationIterations) {
+      this.generationIterations = generationIterations != null ? generationIterations : DEFAULT_ITERATIONS;
     }
 
     @Override
@@ -213,7 +226,7 @@ public class CredentialsLocalAuthentication {
         return new AuthenticationResult(false, ERROR_NULL_SALT);
       }
 
-      int pos = user.getCryptedPassword().indexOf('$');
+      int pos = user.getCryptedPassword().indexOf(ITERATIONS_HASH_SEPARATOR);
       if (pos < 1) {
         return new AuthenticationResult(false, ERROR_INVALID_HASH_STORED);
       }
@@ -229,7 +242,7 @@ public class CredentialsLocalAuthentication {
       if (!hash.equals(hash(salt, password, iterations))) {
         return new AuthenticationResult(false, ERROR_WRONG_PASSWORD);
       }
-      boolean needsUpdate = iterations != gen_iterations;
+      boolean needsUpdate = iterations != generationIterations;
       return new AuthenticationResult(true, "", needsUpdate);
     }
 
@@ -237,13 +250,23 @@ public class CredentialsLocalAuthentication {
     public void storeHashPassword(UserDto user, String password) {
       byte[] salt = new byte[20];
       SECURE_RANDOM.nextBytes(salt);
-      String hashStr = hash(salt, password, gen_iterations);
+      String hashStr = hash(salt, password, generationIterations);
       String saltStr = Base64.getEncoder().encodeToString(salt);
       user.setHashMethod(HashMethod.PBKDF2.name())
-        .setCryptedPassword(gen_iterations + "$" + hashStr)
+        .setCryptedPassword(composeEncryptedPassword(hashStr))
         .setSalt(saltStr);
     }
 
+    @Override
+    public String encryptPassword(String saltStr, String password) {
+      byte[] salt = Base64.getDecoder().decode(saltStr);
+      return composeEncryptedPassword(hash(salt, password, generationIterations));
+    }
+
+    private String composeEncryptedPassword(String hashStr) {
+      return format("%d%c%s", generationIterations, ITERATIONS_HASH_SEPARATOR, hashStr);
+    }
+
     private static String hash(byte[] salt, String password, int iterations) {
       try {
         SecretKeyFactory skf = SecretKeyFactory.getInstance(ALGORITHM);
diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsAuthenticationTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsAuthenticationTest.java
index e883f59fc55..340a2fc3d3c 100644
--- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsAuthenticationTest.java
+++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsAuthenticationTest.java
@@ -23,6 +23,7 @@ import java.util.Optional;
 import javax.servlet.http.HttpServletRequest;
 import org.junit.Rule;
 import org.junit.Test;
+import org.mockito.Mockito;
 import org.sonar.api.config.internal.MapSettings;
 import org.sonar.api.utils.System2;
 import org.sonar.db.DbClient;
@@ -36,7 +37,7 @@ import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.verifyZeroInteractions;
+import static org.mockito.Mockito.verifyNoInteractions;
 import static org.mockito.Mockito.when;
 import static org.sonar.db.user.UserTesting.newUserDto;
 import static org.sonar.server.authentication.event.AuthenticationEvent.Method.BASIC;
@@ -58,7 +59,7 @@ public class CredentialsAuthenticationTest {
   private AuthenticationEvent authenticationEvent = mock(AuthenticationEvent.class);
   private MapSettings settings = new MapSettings().setProperty("sonar.internal.pbkdf2.iterations", "1");
   private CredentialsExternalAuthentication externalAuthentication = mock(CredentialsExternalAuthentication.class);
-  private CredentialsLocalAuthentication localAuthentication = new CredentialsLocalAuthentication(dbClient, settings.asConfig());
+  private CredentialsLocalAuthentication localAuthentication = Mockito.spy(new CredentialsLocalAuthentication(dbClient, settings.asConfig()));
   private CredentialsAuthentication underTest = new CredentialsAuthentication(dbClient, authenticationEvent, externalAuthentication, localAuthentication);
 
   @Test
@@ -90,7 +91,7 @@ public class CredentialsAuthenticationTest {
       .hasFieldOrPropertyWithValue("source", Source.local(BASIC))
       .hasFieldOrPropertyWithValue("login", LOGIN);
 
-    verifyZeroInteractions(authenticationEvent);
+    verifyNoInteractions(authenticationEvent);
 
   }
 
@@ -104,7 +105,7 @@ public class CredentialsAuthenticationTest {
     executeAuthenticate(BASIC);
 
     verify(externalAuthentication).authenticate(new Credentials(LOGIN, PASSWORD), request, BASIC);
-    verifyZeroInteractions(authenticationEvent);
+    verifyNoInteractions(authenticationEvent);
   }
 
   @Test
@@ -120,7 +121,7 @@ public class CredentialsAuthenticationTest {
       .hasFieldOrPropertyWithValue("source", Source.local(BASIC_TOKEN))
       .hasFieldOrPropertyWithValue("login", LOGIN);
 
-    verifyZeroInteractions(authenticationEvent);
+    verifyNoInteractions(authenticationEvent);
   }
 
   @Test
@@ -138,7 +139,7 @@ public class CredentialsAuthenticationTest {
       .hasFieldOrPropertyWithValue("source", Source.local(BASIC))
       .hasFieldOrPropertyWithValue("login", LOGIN);
 
-    verifyZeroInteractions(authenticationEvent);
+    verifyNoInteractions(authenticationEvent);
   }
 
   @Test
@@ -156,8 +157,20 @@ public class CredentialsAuthenticationTest {
       .hasFieldOrPropertyWithValue("source", Source.local(BASIC_TOKEN))
       .hasFieldOrPropertyWithValue("login", LOGIN);
 
-    verifyZeroInteractions(authenticationEvent);
+    verifyNoInteractions(authenticationEvent);
+  }
+
+  @Test
+  public void fail_to_authenticate_unknown_user_after_forcing_hash() {
+    assertThatThrownBy(() -> executeAuthenticate(BASIC))
+      .hasMessage("No active user for login")
+      .isInstanceOf(AuthenticationException.class)
+      .hasFieldOrPropertyWithValue("source", Source.local(BASIC))
+      .hasFieldOrPropertyWithValue("login", LOGIN);
+
+    verify(localAuthentication).generateHashToAvoidEnumerationAttack();
 
+    verifyNoInteractions(authenticationEvent);
   }
 
   private UserDto executeAuthenticate(AuthenticationEvent.Method method) {
diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/PBKDF2FunctionTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/PBKDF2FunctionTest.java
new file mode 100644
index 00000000000..218a3550885
--- /dev/null
+++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/PBKDF2FunctionTest.java
@@ -0,0 +1,39 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2022 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.server.authentication;
+
+import org.junit.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class PBKDF2FunctionTest {
+
+  private static final int GENERATION_ITERATIONS = 1000;
+
+  private final CredentialsLocalAuthentication.PBKDF2Function pbkdf2Function = new CredentialsLocalAuthentication.PBKDF2Function(GENERATION_ITERATIONS);
+
+  @Test
+  public void encryptPassword_returnsCorrectEncryptedPassword() {
+    String encryptedPassword = pbkdf2Function.encryptPassword("salt", "test_password");
+    assertThat(encryptedPassword)
+      .isEqualTo("%d$%s", GENERATION_ITERATIONS, "Yz4QzaROW6N9dqr47NtsDgVJERKC3gTec4rMHonb885IVvTb6OYelaAvMXxoc5QT+4SAjiEmDKaUa2cAC9Ne8Q==");
+  }
+
+}