From f72a0d2f1610819693061df741d47a245fcb8ad5 Mon Sep 17 00:00:00 2001 From: Lai Jiang Date: Mon, 26 Feb 2024 10:28:12 -0500 Subject: [PATCH] Remove SHA256 as a supported password hashing algorithm (#2310) We introduced Scrypt as the default password hashing algorithm in November 2023 and have been auto-converting saved hashes whenever a successful EPP login or registry lock/unlock request is processed. We will send comms to registrars to inform them the upcoming removal of SHA256 support and urge them to log in at least once before the change. Otherwise, they will need to contact support to reset the password out of band after the change. This PR will NOT be submitted until comms are out and the effective date is immediate. Co-authored-by: Weimin Yu --- .../registry/flows/session/LoginFlow.java | 14 +-- .../google/registry/model/console/User.java | 3 +- .../registry/model/registrar/Registrar.java | 5 - .../model/registrar/RegistrarPoc.java | 5 - .../registrar/RegistryLockPostAction.java | 14 --- .../flows/session/LoginFlowTestCase.java | 32 ------ .../registrar/RegistryLockPostActionTest.java | 50 -------- .../google/registry/util/PasswordUtils.java | 108 ++++-------------- .../registry/util/PasswordUtilsTest.java | 19 +-- 9 files changed, 27 insertions(+), 223 deletions(-) diff --git a/core/src/main/java/google/registry/flows/session/LoginFlow.java b/core/src/main/java/google/registry/flows/session/LoginFlow.java index 369960b28..1d2d04e88 100644 --- a/core/src/main/java/google/registry/flows/session/LoginFlow.java +++ b/core/src/main/java/google/registry/flows/session/LoginFlow.java @@ -47,7 +47,6 @@ import google.registry.model.eppinput.EppInput.Options; import google.registry.model.eppinput.EppInput.Services; import google.registry.model.eppoutput.EppResponse; import google.registry.model.registrar.Registrar; -import google.registry.util.PasswordUtils.HashAlgorithm; import java.util.Optional; import java.util.Set; import javax.inject.Inject; @@ -142,17 +141,8 @@ public class LoginFlow implements MutatingFlow { throw new RegistrarAccountNotActiveException(); } - if (login.getNewPassword().isPresent() - || registrar.get().getCurrentHashAlgorithm(login.getPassword()).orElse(null) - != HashAlgorithm.SCRYPT) { - String newPassword = - login - .getNewPassword() - .orElseGet( - () -> { - logger.atInfo().log("Rehashing existing registrar password with Scrypt"); - return login.getPassword(); - }); + if (login.getNewPassword().isPresent()) { + String newPassword = login.getNewPassword().get(); // Load fresh from database (bypassing the cache) to ensure we don't save stale data. Optional freshRegistrar = Registrar.loadByRegistrarId(login.getClientId()); if (freshRegistrar.isEmpty()) { diff --git a/core/src/main/java/google/registry/model/console/User.java b/core/src/main/java/google/registry/model/console/User.java index 5c5fce604..efa72f176 100644 --- a/core/src/main/java/google/registry/model/console/User.java +++ b/core/src/main/java/google/registry/model/console/User.java @@ -86,8 +86,7 @@ public class User extends UpdateAutoTimestampEntity implements Buildable { return false; } return PasswordUtils.verifyPassword( - registryLockPassword, registryLockPasswordHash, registryLockPasswordSalt) - .isPresent(); + registryLockPassword, registryLockPasswordHash, registryLockPasswordSalt); } /** diff --git a/core/src/main/java/google/registry/model/registrar/Registrar.java b/core/src/main/java/google/registry/model/registrar/Registrar.java index 84ca2c17b..972e2bac4 100644 --- a/core/src/main/java/google/registry/model/registrar/Registrar.java +++ b/core/src/main/java/google/registry/model/registrar/Registrar.java @@ -62,7 +62,6 @@ import google.registry.model.tld.Tld.TldType; import google.registry.persistence.VKey; import google.registry.util.CidrAddressBlock; import google.registry.util.PasswordUtils; -import google.registry.util.PasswordUtils.HashAlgorithm; import java.security.cert.CertificateParsingException; import java.util.Comparator; import java.util.List; @@ -642,10 +641,6 @@ public class Registrar extends UpdateAutoTimestampEntity implements Buildable, J } public boolean verifyPassword(String password) { - return getCurrentHashAlgorithm(password).isPresent(); - } - - public Optional getCurrentHashAlgorithm(String password) { return PasswordUtils.verifyPassword(password, passwordHash, salt); } diff --git a/core/src/main/java/google/registry/model/registrar/RegistrarPoc.java b/core/src/main/java/google/registry/model/registrar/RegistrarPoc.java index f008d86b0..f67377283 100644 --- a/core/src/main/java/google/registry/model/registrar/RegistrarPoc.java +++ b/core/src/main/java/google/registry/model/registrar/RegistrarPoc.java @@ -38,7 +38,6 @@ import google.registry.model.UnsafeSerializable; import google.registry.model.registrar.RegistrarPoc.RegistrarPocId; import google.registry.persistence.VKey; import google.registry.util.PasswordUtils; -import google.registry.util.PasswordUtils.HashAlgorithm; import java.io.Serializable; import java.util.Map; import java.util.Optional; @@ -242,10 +241,6 @@ public class RegistrarPoc extends ImmutableObject implements Jsonifiable, Unsafe || isNullOrEmpty(registryLockPasswordHash)) { return false; } - return getCurrentHashAlgorithm(registryLockPassword).isPresent(); - } - - public Optional getCurrentHashAlgorithm(String registryLockPassword) { return PasswordUtils.verifyPassword( registryLockPassword, registryLockPasswordHash, registryLockPasswordSalt); } diff --git a/core/src/main/java/google/registry/ui/server/registrar/RegistryLockPostAction.java b/core/src/main/java/google/registry/ui/server/registrar/RegistryLockPostAction.java index beb4f6361..4012d056f 100644 --- a/core/src/main/java/google/registry/ui/server/registrar/RegistryLockPostAction.java +++ b/core/src/main/java/google/registry/ui/server/registrar/RegistryLockPostAction.java @@ -47,7 +47,6 @@ import google.registry.request.auth.UserAuthInfo; import google.registry.security.JsonResponseHelper; import google.registry.tools.DomainLockUtils; import google.registry.util.EmailMessage; -import google.registry.util.PasswordUtils.HashAlgorithm; import java.net.URISyntaxException; import java.util.Map; import java.util.Optional; @@ -223,19 +222,6 @@ public class RegistryLockPostAction implements Runnable, JsonActionRunner.JsonAc checkArgument( registrarPoc.verifyRegistryLockPassword(postInput.password), "Incorrect registry lock password for contact"); - if (registrarPoc.getCurrentHashAlgorithm(postInput.password).orElse(null) - != HashAlgorithm.SCRYPT) { - logger.atInfo().log("Rehashing existing registry lock password with Scrypt."); - tm().transact( - () -> { - tm().update( - tm().loadByEntity(registrarPoc) - .asBuilder() - .setAllowedToSetRegistryLockPassword(true) - .setRegistryLockPassword(postInput.password) - .build()); - }); - } return registrarPoc .getRegistryLockEmailAddress() .orElseThrow( diff --git a/core/src/test/java/google/registry/flows/session/LoginFlowTestCase.java b/core/src/test/java/google/registry/flows/session/LoginFlowTestCase.java index 2cb5c3df6..fbb338bcc 100644 --- a/core/src/test/java/google/registry/flows/session/LoginFlowTestCase.java +++ b/core/src/test/java/google/registry/flows/session/LoginFlowTestCase.java @@ -14,15 +14,11 @@ package google.registry.flows.session; -import static com.google.common.io.BaseEncoding.base64; import static com.google.common.truth.Truth.assertThat; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.testing.DatabaseHelper.deleteResource; import static google.registry.testing.DatabaseHelper.loadRegistrar; import static google.registry.testing.DatabaseHelper.persistResource; import static google.registry.testing.EppExceptionSubject.assertAboutEppExceptions; -import static google.registry.util.PasswordUtils.HashAlgorithm.SCRYPT; -import static google.registry.util.PasswordUtils.HashAlgorithm.SHA256; import static org.junit.jupiter.api.Assertions.assertThrows; import com.google.common.collect.ImmutableMap; @@ -42,7 +38,6 @@ import google.registry.model.eppoutput.EppOutput; import google.registry.model.registrar.Registrar; import google.registry.model.registrar.Registrar.State; import google.registry.testing.DatabaseHelper; -import google.registry.util.PasswordUtils; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -191,33 +186,6 @@ public abstract class LoginFlowTestCase extends FlowTestCase { doFailingTest("login_valid.xml", RegistrarAccountNotActiveException.class); } - @Test - void testSuccess_sha256Password() throws Exception { - String password = "foo-BAR2"; - tm().transact( - () -> { - // The salt is not exposed by Registrar (nor should it be), so we query it - // directly. - String encodedSalt = - tm().query("SELECT salt FROM Registrar WHERE registrarId = :id", String.class) - .setParameter("id", registrar.getRegistrarId()) - .getSingleResult(); - byte[] salt = base64().decode(encodedSalt); - String newHash = PasswordUtils.hashPassword(password, salt, SHA256); - // Set password directly, as the Java method would have used Scrypt. - tm().query("UPDATE Registrar SET passwordHash = :hash WHERE registrarId = :id") - .setParameter("id", registrar.getRegistrarId()) - .setParameter("hash", newHash) - .executeUpdate(); - }); - assertThat(loadRegistrar("NewRegistrar").getCurrentHashAlgorithm(password).get()) - .isEqualTo(SHA256); - doSuccessfulTest("login_valid.xml"); - // Verifies that after successfully login, the password is re-hased with Scrypt. - assertThat(loadRegistrar("NewRegistrar").getCurrentHashAlgorithm(password).get()) - .isEqualTo(SCRYPT); - } - @Test void testFailure_incorrectPassword() { persistResource(getRegistrarBuilder().setPassword("diff password").build()); diff --git a/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java b/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java index 429b798b6..65411647c 100644 --- a/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java +++ b/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java @@ -15,10 +15,8 @@ package google.registry.ui.server.registrar; import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap; -import static com.google.common.io.BaseEncoding.base64; import static com.google.common.truth.Truth.assertThat; import static google.registry.model.EppResourceUtils.loadByForeignKey; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.testing.DatabaseHelper.createTld; import static google.registry.testing.DatabaseHelper.loadRegistrar; import static google.registry.testing.DatabaseHelper.persistResource; @@ -27,8 +25,6 @@ import static google.registry.testing.SqlHelper.getRegistryLockByVerificationCod import static google.registry.testing.SqlHelper.saveRegistryLock; import static google.registry.tools.LockOrUnlockDomainCommand.REGISTRY_LOCK_STATUSES; import static google.registry.ui.server.registrar.RegistryLockGetActionTest.userFromRegistrarPoc; -import static google.registry.util.PasswordUtils.HashAlgorithm.SCRYPT; -import static google.registry.util.PasswordUtils.HashAlgorithm.SHA256; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; @@ -42,9 +38,6 @@ import google.registry.model.console.RegistrarRole; import google.registry.model.console.UserRoles; import google.registry.model.domain.Domain; import google.registry.model.domain.RegistryLock; -import google.registry.model.registrar.RegistrarPoc; -import google.registry.model.registrar.RegistrarPoc.RegistrarPocId; -import google.registry.persistence.VKey; import google.registry.persistence.transaction.JpaTestExtensions; import google.registry.persistence.transaction.JpaTestExtensions.JpaIntegrationTestExtension; import google.registry.persistence.transaction.JpaTransactionManagerExtension; @@ -61,7 +54,6 @@ import google.registry.testing.DeterministicStringGenerator; import google.registry.testing.FakeClock; import google.registry.tools.DomainLockUtils; import google.registry.util.EmailMessage; -import google.registry.util.PasswordUtils; import google.registry.util.StringGenerator.Alphabets; import java.util.Map; import java.util.Optional; @@ -131,48 +123,6 @@ final class RegistryLockPostActionTest { assertSuccess(response, "lock", "Marla.Singer.RegistryLock@crr.com"); } - @Test - void testSuccess_lock_sha256Password() throws Exception { - tm().transact( - () -> { - // The salt is not exposed by RegistrarPoc (nor should it be), so we query - // it directly. - String encodedSalt = - tm().query( - "SELECT registryLockPasswordSalt FROM RegistrarPoc " - + "WHERE emailAddress = :email " - + "AND registrarId = :registrarId", - String.class) - .setParameter("email", "Marla.Singer@crr.com") - .setParameter("registrarId", "TheRegistrar") - .getSingleResult(); - byte[] salt = base64().decode(encodedSalt); - String newHash = PasswordUtils.hashPassword("hi", salt, SHA256); - // Set password directly, as the Java method would have used Scrypt. - tm().query("UPDATE RegistrarPoc SET registryLockPasswordHash = :hash") - .setParameter("hash", newHash) - .executeUpdate(); - }); - RegistrarPoc registrarPoc = - tm().transact( - () -> - tm().loadByKey( - VKey.create( - RegistrarPoc.class, - new RegistrarPocId("Marla.Singer@crr.com", "TheRegistrar")))); - assertThat(registrarPoc.getCurrentHashAlgorithm("hi").get()).isEqualTo(SHA256); - Map response = action.handleJsonRequest(lockRequest()); - RegistrarPoc updatedRegistrarPoc = - tm().transact( - () -> - tm().loadByKey( - VKey.create( - RegistrarPoc.class, - new RegistrarPocId("Marla.Singer@crr.com", "TheRegistrar")))); - assertThat(updatedRegistrarPoc.getCurrentHashAlgorithm("hi").get()).isEqualTo(SCRYPT); - assertSuccess(response, "lock", "Marla.Singer.RegistryLock@crr.com"); - } - @Test void testSuccess_unlock() throws Exception { saveRegistryLock(createLock().asBuilder().setLockCompletionTime(clock.nowUtc()).build()); diff --git a/util/src/main/java/google/registry/util/PasswordUtils.java b/util/src/main/java/google/registry/util/PasswordUtils.java index 365aec9a5..af590a901 100644 --- a/util/src/main/java/google/registry/util/PasswordUtils.java +++ b/util/src/main/java/google/registry/util/PasswordUtils.java @@ -15,82 +15,29 @@ package google.registry.util; import static com.google.common.io.BaseEncoding.base64; -import static google.registry.util.PasswordUtils.HashAlgorithm.SCRYPT; -import static google.registry.util.PasswordUtils.HashAlgorithm.SHA256; import static java.nio.charset.StandardCharsets.US_ASCII; import com.google.common.base.Supplier; -import com.google.common.base.Suppliers; -import com.google.common.flogger.FluentLogger; import com.google.common.primitives.Bytes; -import java.security.MessageDigest; -import java.security.NoSuchAlgorithmException; import java.security.SecureRandom; import java.util.Arrays; -import java.util.Optional; import org.bouncycastle.crypto.generators.SCrypt; -/** Common utility class to handle password hashing and salting */ +/** + * Common utility class to handle password hashing and salting /* + * + *

We use a memory-hard hashing algorithm (Scrypt) to prevent brute-force attacks on passwords. + * + *

Note that in tests, we simply concatenate the password and salt which is much faster and + * reduces the overall test run time by a half. Our tests are not verifying that SCRYPT is + * implemented correctly anyway. + * + * @see Scrypt + */ public final class PasswordUtils { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - private static final Supplier SHA256_DIGEST_SUPPLIER = - Suppliers.memoize( - () -> { - try { - return MessageDigest.getInstance("SHA-256"); - } catch (NoSuchAlgorithmException e) { - // All implementations of MessageDigest are required to support SHA-256. - throw new RuntimeException( - "All MessageDigest implementations are required to support SHA-256 but this one" - + " didn't", - e); - } - }); - private PasswordUtils() {} - /** - * Password hashing algorithm that takes a password and a salt (both as {@code byte[]}) and - * returns a hash. - */ - public enum HashAlgorithm { - /** - * SHA-2 that returns a 256-bit digest. - * - * @see SHA-2 - */ - @Deprecated - SHA256 { - @Override - byte[] hash(byte[] password, byte[] salt) { - return SHA256_DIGEST_SUPPLIER - .get() - .digest((new String(password, US_ASCII) + base64().encode(salt)).getBytes(US_ASCII)); - } - }, - - /** - * Memory-hard hashing algorithm, preferred over SHA-256. - * - *

Note that in tests, we simply concatenate the password and salt which is much faster and - * reduces the overall test run time by a half. Our tests are not verifying that SCRYPT is - * implemented correctly anyway. - * - * @see Scrypt - */ - SCRYPT { - @Override - byte[] hash(byte[] password, byte[] salt) { - return RegistryEnvironment.get() == RegistryEnvironment.UNITTEST - ? Bytes.concat(password, salt) - : SCrypt.generate(password, salt, 32768, 8, 1, 256); - } - }; - - abstract byte[] hash(byte[] password, byte[] salt); - } - public static final Supplier SALT_SUPPLIER = () -> { // The generated hashes are 256 bits, and the salt should generally be of the same size. @@ -99,38 +46,25 @@ public final class PasswordUtils { return salt; }; - public static String hashPassword(String password, byte[] salt) { - return hashPassword(password, salt, SCRYPT); + private static byte[] hashPassword(byte[] password, byte[] salt) { + return RegistryEnvironment.get() == RegistryEnvironment.UNITTEST + ? Bytes.concat(password, salt) + : SCrypt.generate(password, salt, 32768, 8, 1, 256); } - /** Returns the hash of the password using the provided salt and {@link HashAlgorithm}. */ - public static String hashPassword(String password, byte[] salt, HashAlgorithm algorithm) { - return base64().encode(algorithm.hash(password.getBytes(US_ASCII), salt)); + /** Returns the hash of the password using the provided salt. */ + public static String hashPassword(String password, byte[] salt) { + return base64().encode(hashPassword(password.getBytes(US_ASCII), salt)); } /** * Verifies a password by regenerating the hash with the provided salt and comparing it to the * provided hash. - * - *

This method will first try to use {@link HashAlgorithm#SCRYPT} to verify the password, and - * falls back to {@link HashAlgorithm#SHA256} if the former fails. - * - * @return the {@link HashAlgorithm} used to successfully verify the password, or {@link - * Optional#empty()} if neither works. */ - public static Optional verifyPassword(String password, String hash, String salt) { + public static boolean verifyPassword(String password, String hash, String salt) { byte[] decodedHash = base64().decode(hash); byte[] decodedSalt = base64().decode(salt); - byte[] calculatedHash = SCRYPT.hash(password.getBytes(US_ASCII), decodedSalt); - if (Arrays.equals(decodedHash, calculatedHash)) { - logger.atInfo().log("Scrypt hash verified."); - return Optional.of(SCRYPT); - } - calculatedHash = SHA256.hash(password.getBytes(US_ASCII), decodedSalt); - if (Arrays.equals(decodedHash, calculatedHash)) { - logger.atInfo().log("SHA256 hash verified."); - return Optional.of(SHA256); - } - return Optional.empty(); + byte[] calculatedHash = hashPassword(password.getBytes(US_ASCII), decodedSalt); + return Arrays.equals(decodedHash, calculatedHash); } } diff --git a/util/src/test/java/google/registry/util/PasswordUtilsTest.java b/util/src/test/java/google/registry/util/PasswordUtilsTest.java index 80cad0f42..bc59db284 100644 --- a/util/src/test/java/google/registry/util/PasswordUtilsTest.java +++ b/util/src/test/java/google/registry/util/PasswordUtilsTest.java @@ -16,8 +16,6 @@ package google.registry.util; import static com.google.common.io.BaseEncoding.base64; import static com.google.common.truth.Truth.assertThat; -import static google.registry.util.PasswordUtils.HashAlgorithm.SCRYPT; -import static google.registry.util.PasswordUtils.HashAlgorithm.SHA256; import static google.registry.util.PasswordUtils.SALT_SUPPLIER; import static google.registry.util.PasswordUtils.hashPassword; import static google.registry.util.PasswordUtils.verifyPassword; @@ -53,18 +51,8 @@ final class PasswordUtilsTest { byte[] salt = SALT_SUPPLIER.get(); String password = "mySuperSecurePassword"; String hashedPassword = hashPassword(password, salt); - assertThat(hashedPassword).isEqualTo(hashPassword(password, salt, SCRYPT)); - assertThat(verifyPassword(password, hashedPassword, base64().encode(salt)).get()) - .isEqualTo(SCRYPT); - } - - @Test - void testVerify_sha256() { - byte[] salt = SALT_SUPPLIER.get(); - String password = "mySuperSecurePassword"; - String hashedPassword = hashPassword(password, salt, SHA256); - assertThat(verifyPassword(password, hashedPassword, base64().encode(salt)).get()) - .isEqualTo(SHA256); + assertThat(hashedPassword).isEqualTo(hashPassword(password, salt)); + assertThat(verifyPassword(password, hashedPassword, base64().encode(salt))).isTrue(); } @Test @@ -72,7 +60,6 @@ final class PasswordUtilsTest { byte[] salt = SALT_SUPPLIER.get(); String password = "mySuperSecurePassword"; String hashedPassword = hashPassword(password, salt); - assertThat(verifyPassword(password + "a", hashedPassword, base64().encode(salt)).isPresent()) - .isFalse(); + assertThat(verifyPassword(password + "a", hashedPassword, base64().encode(salt))).isFalse(); } }