From e50141d0a11ba19085b884e7bf1b99fc83be9b7f Mon Sep 17 00:00:00 2001 From: Wouter Admiraal Date: Tue, 27 Apr 2021 10:46:36 +0200 Subject: [PATCH] SONAR-14320 Do not throw an exception if password is NULL and hash is BCrypt --- .../pages/instance-administration/security.md | 2 +- .../CredentialsLocalAuthentication.java | 39 ++++--- .../CredentialsLocalAuthenticationTest.java | 101 ++++++++++-------- 3 files changed, 85 insertions(+), 57 deletions(-) diff --git a/server/sonar-docs/src/pages/instance-administration/security.md b/server/sonar-docs/src/pages/instance-administration/security.md index e2bd9d47971..7ccb7268b34 100644 --- a/server/sonar-docs/src/pages/instance-administration/security.md +++ b/server/sonar-docs/src/pages/instance-administration/security.md @@ -94,7 +94,7 @@ When installing SonarQube, a default user with Administer System permission is c ## Reinstating Admin Access If you changed and then lost the `admin` password, you can reset it using the following query: ``` -update users set crypted_password='100000$t2h8AtNs1AlCHuLobDjHQTn9XppwTIx88UjqUm4s8RsfTuXQHSd/fpFexAnewwPsO6jGFQUv/24DnO55hY6Xew==', salt='k9x9eN127/3e/hf38iNiKwVfaVk=', hash_method='PBKDF2', reset_password='true' where login='admin' +update users set crypted_password='100000$t2h8AtNs1AlCHuLobDjHQTn9XppwTIx88UjqUm4s8RsfTuXQHSd/fpFexAnewwPsO6jGFQUv/24DnO55hY6Xew==', salt='k9x9eN127/3e/hf38iNiKwVfaVk=', hash_method='PBKDF2', reset_password='true', user_local='true' where login='admin'; ``` If you've deleted `admin` and subsequently locked out the other users with global administrative permissions, you'll need to re-grant `admin` to a user with the following query: ``` 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 e54b106c156..c7aa4d5acae 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 @@ -49,6 +49,12 @@ 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 final DbClient dbClient; private final EnumMap hashFunctions = new EnumMap<>(HashMethod.class); @@ -76,7 +82,7 @@ public class CredentialsLocalAuthentication { throw AuthenticationException.newBuilder() .setSource(Source.local(method)) .setLogin(user.getLogin()) - .setMessage("null hash method") + .setMessage(ERROR_NULL_HASH_METHOD) .build(); } @@ -87,7 +93,7 @@ public class CredentialsLocalAuthentication { throw AuthenticationException.newBuilder() .setSource(Source.local(method)) .setLogin(user.getLogin()) - .setMessage(format("Unknown hash method [%s]", user.getHashMethod())) + .setMessage(format(ERROR_UNKNOWN_HASH_METHOD, user.getHashMethod())) .build(); } @@ -159,20 +165,20 @@ public class CredentialsLocalAuthentication { @Override public AuthenticationResult checkCredentials(UserDto user, String password) { if (user.getCryptedPassword() == null) { - return new AuthenticationResult(false, "null password in DB"); + return new AuthenticationResult(false, ERROR_NULL_PASSWORD_IN_DB); } if (user.getSalt() == null) { - return new AuthenticationResult(false, "null salt"); + return new AuthenticationResult(false, ERROR_NULL_SALT); } if (!user.getCryptedPassword().equals(hash(user.getSalt(), password))) { - return new AuthenticationResult(false, "wrong password"); + return new AuthenticationResult(false, ERROR_WRONG_PASSWORD); } return new AuthenticationResult(true, ""); } @Override public void storeHashPassword(UserDto user, String password) { - requireNonNull(password, "Password cannot be null"); + requireNonNull(password, ERROR_PASSWORD_CANNOT_BE_NULL); byte[] saltRandom = new byte[20]; SECURE_RANDOM.nextBytes(saltRandom); String salt = DigestUtils.sha1Hex(saltRandom); @@ -191,6 +197,7 @@ public class CredentialsLocalAuthentication { 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; public PBKDF2Function(@Nullable Integer gen_iterations) { @@ -200,27 +207,27 @@ public class CredentialsLocalAuthentication { @Override public AuthenticationResult checkCredentials(UserDto user, String password) { if (user.getCryptedPassword() == null) { - return new AuthenticationResult(false, "null password in DB"); + return new AuthenticationResult(false, ERROR_NULL_PASSWORD_IN_DB); } if (user.getSalt() == null) { - return new AuthenticationResult(false, "null salt"); + return new AuthenticationResult(false, ERROR_NULL_SALT); } int pos = user.getCryptedPassword().indexOf('$'); if (pos < 1) { - return new AuthenticationResult(false, "invalid hash stored"); + return new AuthenticationResult(false, ERROR_INVALID_HASH_STORED); } int iterations; try { iterations = Integer.parseInt(user.getCryptedPassword().substring(0, pos)); } catch (NumberFormatException e) { - return new AuthenticationResult(false, "invalid hash stored"); + return new AuthenticationResult(false, ERROR_INVALID_HASH_STORED); } String hash = user.getCryptedPassword().substring(pos + 1); byte[] salt = Base64.getDecoder().decode(user.getSalt()); if (!hash.equals(hash(salt, password, iterations))) { - return new AuthenticationResult(false, "wrong password"); + return new AuthenticationResult(false, ERROR_WRONG_PASSWORD); } boolean needsUpdate = iterations != gen_iterations; return new AuthenticationResult(true, "", needsUpdate); @@ -255,16 +262,20 @@ public class CredentialsLocalAuthentication { private static final class BcryptFunction implements HashFunction { @Override public AuthenticationResult checkCredentials(UserDto user, String password) { - // This behavior is overridden in most of integration tests for performance reasons, any changes to BCrypt calls should be propagated to Byteman classes + if (user.getCryptedPassword() == null) { + return new AuthenticationResult(false, ERROR_NULL_PASSWORD_IN_DB); + } + // This behavior is overridden in most of integration tests for performance reasons, any changes to BCrypt calls should be propagated to + // Byteman classes if (!BCrypt.checkpw(password, user.getCryptedPassword())) { - return new AuthenticationResult(false, "wrong password"); + return new AuthenticationResult(false, ERROR_WRONG_PASSWORD); } return new AuthenticationResult(true, ""); } @Override public void storeHashPassword(UserDto user, String password) { - requireNonNull(password, "Password cannot be null"); + requireNonNull(password, ERROR_PASSWORD_CANNOT_BE_NULL); user.setHashMethod(HashMethod.BCRYPT.name()) .setCryptedPassword(BCrypt.hashpw(password, BCrypt.gensalt(12))) .setSalt(null); diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsLocalAuthenticationTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsLocalAuthenticationTest.java index e7cafffc61a..7cac4072035 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsLocalAuthenticationTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsLocalAuthenticationTest.java @@ -25,24 +25,24 @@ import org.apache.commons.codec.digest.DigestUtils; import org.junit.Before; import org.junit.Rule; import org.junit.Test; -import org.junit.rules.ExpectedException; import org.mindrot.jbcrypt.BCrypt; import org.sonar.api.config.internal.MapSettings; +import org.sonar.db.DbSession; import org.sonar.db.DbTester; import org.sonar.db.user.UserDto; import org.sonar.server.authentication.event.AuthenticationEvent; import org.sonar.server.authentication.event.AuthenticationException; +import static java.lang.String.format; import static org.apache.commons.lang.RandomStringUtils.randomAlphanumeric; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.sonar.db.user.UserTesting.newUserDto; import static org.sonar.server.authentication.CredentialsLocalAuthentication.HashMethod.BCRYPT; import static org.sonar.server.authentication.CredentialsLocalAuthentication.HashMethod.PBKDF2; import static org.sonar.server.authentication.CredentialsLocalAuthentication.HashMethod.SHA1; public class CredentialsLocalAuthenticationTest { - @Rule - public ExpectedException expectedException = ExpectedException.none(); @Rule public DbTester db = DbTester.create(); @@ -58,23 +58,23 @@ public class CredentialsLocalAuthenticationTest { @Test public void incorrect_hash_should_throw_AuthenticationException() { + DbSession dbSession = db.getSession(); UserDto user = newUserDto() .setHashMethod("ALGON2"); - expectedException.expect(AuthenticationException.class); - expectedException.expectMessage("Unknown hash method [ALGON2]"); - - underTest.authenticate(db.getSession(), user, "whatever", AuthenticationEvent.Method.BASIC); + assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "whatever", AuthenticationEvent.Method.BASIC)) + .isInstanceOf(AuthenticationException.class) + .hasMessage(format(CredentialsLocalAuthentication.ERROR_UNKNOWN_HASH_METHOD, "ALGON2")); } @Test public void null_hash_should_throw_AuthenticationException() { + DbSession dbSession = db.getSession(); UserDto user = newUserDto(); - expectedException.expect(AuthenticationException.class); - expectedException.expectMessage("null hash method"); - - underTest.authenticate(db.getSession(), user, "whatever", AuthenticationEvent.Method.BASIC); + assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "whatever", AuthenticationEvent.Method.BASIC)) + .isInstanceOf(AuthenticationException.class) + .hasMessage(CredentialsLocalAuthentication.ERROR_NULL_HASH_METHOD); } @Test @@ -107,6 +107,7 @@ public class CredentialsLocalAuthenticationTest { @Test public void authentication_with_sha1_with_incorrect_password_should_throw_AuthenticationException() { String password = randomAlphanumeric(60); + DbSession dbSession = db.getSession(); byte[] saltRandom = new byte[20]; RANDOM.nextBytes(saltRandom); @@ -117,14 +118,14 @@ public class CredentialsLocalAuthenticationTest { .setCryptedPassword(DigestUtils.sha1Hex("--" + salt + "--" + password + "--")) .setSalt(salt); - expectedException.expect(AuthenticationException.class); - expectedException.expectMessage("wrong password"); - - underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC); + assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC)) + .isInstanceOf(AuthenticationException.class) + .hasMessage(CredentialsLocalAuthentication.ERROR_WRONG_PASSWORD); } @Test public void authentication_with_sha1_with_empty_password_should_throw_AuthenticationException() { + DbSession dbSession = db.getSession(); byte[] saltRandom = new byte[20]; RANDOM.nextBytes(saltRandom); String salt = DigestUtils.sha1Hex(saltRandom); @@ -134,14 +135,14 @@ public class CredentialsLocalAuthenticationTest { .setHashMethod(SHA1.name()) .setSalt(salt); - expectedException.expect(AuthenticationException.class); - expectedException.expectMessage("null password in DB"); - - underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC); + assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC)) + .isInstanceOf(AuthenticationException.class) + .hasMessage(CredentialsLocalAuthentication.ERROR_NULL_PASSWORD_IN_DB); } @Test public void authentication_with_sha1_with_empty_salt_should_throw_AuthenticationException() { + DbSession dbSession = db.getSession(); String password = randomAlphanumeric(60); UserDto user = newUserDto() @@ -149,24 +150,35 @@ public class CredentialsLocalAuthenticationTest { .setCryptedPassword(DigestUtils.sha1Hex("--0242b0b4c0a93ddfe09dd886de50bc25ba000b51--" + password + "--")) .setSalt(null); - expectedException.expect(AuthenticationException.class); - expectedException.expectMessage("null salt"); - - underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC); + assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC)) + .isInstanceOf(AuthenticationException.class) + .hasMessage(CredentialsLocalAuthentication.ERROR_NULL_SALT); } @Test public void authentication_with_bcrypt_with_incorrect_password_should_throw_AuthenticationException() { + DbSession dbSession = db.getSession(); String password = randomAlphanumeric(60); UserDto user = newUserDto() .setHashMethod(BCRYPT.name()) .setCryptedPassword(BCrypt.hashpw(password, BCrypt.gensalt(12))); - expectedException.expect(AuthenticationException.class); - expectedException.expectMessage("wrong password"); + assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC)) + .isInstanceOf(AuthenticationException.class) + .hasMessage(CredentialsLocalAuthentication.ERROR_WRONG_PASSWORD); + } - underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC); + @Test + public void authentication_with_bcrypt_with_empty_password_should_throw_AuthenticationException() { + DbSession dbSession = db.getSession(); + UserDto user = newUserDto() + .setCryptedPassword(null) + .setHashMethod(BCRYPT.name()); + + assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC)) + .isInstanceOf(AuthenticationException.class) + .hasMessage(CredentialsLocalAuthentication.ERROR_NULL_PASSWORD_IN_DB); } @Test @@ -273,34 +285,39 @@ public class CredentialsLocalAuthenticationTest { @Test public void authentication_with_pbkdf2_with_incorrect_password_should_throw_AuthenticationException() { + DbSession dbSession = db.getSession(); UserDto user = newUserDto() .setHashMethod(PBKDF2.name()) .setCryptedPassword("1$hash") .setSalt("salt"); - expectedException.expect(AuthenticationException.class); - expectedException.expectMessage("wrong password"); - - underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC); + assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC)) + .isInstanceOf(AuthenticationException.class) + .hasMessage(CredentialsLocalAuthentication.ERROR_WRONG_PASSWORD); } @Test public void authentication_with_pbkdf2_with_invalid_password_should_throw_AuthenticationException() { + DbSession dbSession = db.getSession(); String password = randomAlphanumeric(60); byte[] saltRandom = new byte[20]; RANDOM.nextBytes(saltRandom); String salt = DigestUtils.sha1Hex(saltRandom); - UserDto user = newUserDto() + UserDto userInvalidHash = newUserDto() .setHashMethod(PBKDF2.name()) .setCryptedPassword(DigestUtils.sha1Hex("--" + salt + "--" + password + "--")) .setSalt(salt); - expectedException.expect(AuthenticationException.class); - expectedException.expectMessage("invalid hash stored"); + UserDto userInvalidIterations = newUserDto() + .setHashMethod(PBKDF2.name()) + .setCryptedPassword("$$") + .setSalt(salt); - underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC); + assertThatThrownBy(() -> underTest.authenticate(dbSession, userInvalidIterations, "WHATEVER", AuthenticationEvent.Method.BASIC)) + .isInstanceOf(AuthenticationException.class) + .hasMessage("invalid hash stored"); } @Test @@ -308,30 +325,30 @@ public class CredentialsLocalAuthenticationTest { byte[] saltRandom = new byte[20]; RANDOM.nextBytes(saltRandom); String salt = DigestUtils.sha1Hex(saltRandom); + DbSession dbSession = db.getSession(); UserDto user = newUserDto() .setCryptedPassword(null) .setHashMethod(PBKDF2.name()) .setSalt(salt); - expectedException.expect(AuthenticationException.class); - expectedException.expectMessage("null password in DB"); - - underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC); + assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC)) + .isInstanceOf(AuthenticationException.class) + .hasMessage(CredentialsLocalAuthentication.ERROR_NULL_PASSWORD_IN_DB); } @Test public void authentication_with_pbkdf2_with_empty_salt_should_throw_AuthenticationException() { String password = randomAlphanumeric(60); + DbSession dbSession = db.getSession(); UserDto user = newUserDto() .setHashMethod(PBKDF2.name()) .setCryptedPassword(DigestUtils.sha1Hex("--0242b0b4c0a93ddfe09dd886de50bc25ba000b51--" + password + "--")) .setSalt(null); - expectedException.expect(AuthenticationException.class); - expectedException.expectMessage("null salt"); - - underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC); + assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC)) + .isInstanceOf(AuthenticationException.class) + .hasMessage(CredentialsLocalAuthentication.ERROR_NULL_SALT); } } -- 2.39.5