From 73d29134eb139947c9e79c6a3330a32e145bda6c Mon Sep 17 00:00:00 2001 From: Aurelien <100427063+aurelien-poscia-sonarsource@users.noreply.github.com> Date: Tue, 29 Mar 2022 17:58:59 +0200 Subject: [PATCH] SONAR-16181 fix SSF-227 --- .../CredentialsAuthentication.java | 2 + .../CredentialsLocalAuthentication.java | 47 ++++++++++++++----- .../CredentialsAuthenticationTest.java | 27 ++++++++--- .../authentication/PBKDF2FunctionTest.java | 39 +++++++++++++++ 4 files changed, 96 insertions(+), 19 deletions(-) create mode 100644 server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/PBKDF2FunctionTest.java 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 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=="); + } + +} -- 2.39.5