From 5be3f8faccca5db9b528be365f1618ac4addb242 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sun, 8 Oct 2017 22:42:37 +0200 Subject: [PATCH] #1095 Update SMF hash algorithm to generate salt as SMF does - The salt isn't used for password hashing but SMF requires that there be one to generate the authentication cookie. This does not yet enable registration from Minecraft: SMF has other non-null columns that need to be tackled. This is a first step. --- .../fr/xephi/authme/security/crypts/Smf.java | 39 ++++++++++++++++++- .../crypts/AbstractEncryptionMethodTest.java | 20 ++++++---- .../xephi/authme/security/crypts/SmfTest.java | 27 +++++++++++++ 3 files changed, 76 insertions(+), 10 deletions(-) diff --git a/src/main/java/fr/xephi/authme/security/crypts/Smf.java b/src/main/java/fr/xephi/authme/security/crypts/Smf.java index 46114d2e9..24d28fe6c 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/Smf.java +++ b/src/main/java/fr/xephi/authme/security/crypts/Smf.java @@ -1,12 +1,47 @@ package fr.xephi.authme.security.crypts; import fr.xephi.authme.security.HashUtils; +import fr.xephi.authme.security.crypts.description.HasSalt; +import fr.xephi.authme.security.crypts.description.Recommendation; +import fr.xephi.authme.security.crypts.description.SaltType; +import fr.xephi.authme.security.crypts.description.Usage; +import fr.xephi.authme.util.RandomStringUtils; -public class Smf extends UsernameSaltMethod { +/** + * Hashing algorithm for SMF forums. + *

+ * The hash algorithm is {@code sha1(strtolower($username) . $password)}. However, an additional four-character + * salt is generated for each user, used to generate the login cookie. Therefore, this implementation generates a salt + * and declares that it requires a separate salt (the SMF members table has a not-null constraint on the salt column). + * + * @see Simple Machines Forum + */ +@Recommendation(Usage.DO_NOT_USE) +@HasSalt(SaltType.USERNAME) +public class Smf implements EncryptionMethod { @Override public HashedPassword computeHash(String password, String name) { - return new HashedPassword(HashUtils.sha1(name.toLowerCase() + password)); + return new HashedPassword(computeHash(password, null, name), generateSalt()); } + @Override + public String computeHash(String password, String salt, String name) { + return HashUtils.sha1(name.toLowerCase() + password); + } + + @Override + public boolean comparePassword(String password, HashedPassword hashedPassword, String name) { + return computeHash(password, null, name).equals(hashedPassword.getHash()); + } + + @Override + public String generateSalt() { + return RandomStringUtils.generate(4); + } + + @Override + public boolean hasSeparateSalt() { + return true; + } } diff --git a/src/test/java/fr/xephi/authme/security/crypts/AbstractEncryptionMethodTest.java b/src/test/java/fr/xephi/authme/security/crypts/AbstractEncryptionMethodTest.java index 24093721a..b42cd9268 100644 --- a/src/test/java/fr/xephi/authme/security/crypts/AbstractEncryptionMethodTest.java +++ b/src/test/java/fr/xephi/authme/security/crypts/AbstractEncryptionMethodTest.java @@ -64,10 +64,7 @@ public abstract class AbstractEncryptionMethodTest { */ public AbstractEncryptionMethodTest(EncryptionMethod method, String hash0, String hash1, String hash2, String hash3) { - if (method.hasSeparateSalt()) { - throw new UnsupportedOperationException("Test must be initialized with HashedPassword objects if " - + "the salt is stored separately. Use the other constructor"); - } + verifyCorrectConstructorIsUsed(method, false); this.method = method; hashes = ImmutableMap.of( @@ -89,10 +86,7 @@ public abstract class AbstractEncryptionMethodTest { */ public AbstractEncryptionMethodTest(EncryptionMethod method, HashedPassword result0, HashedPassword result1, HashedPassword result2, HashedPassword result3) { - if (!method.hasSeparateSalt()) { - throw new UnsupportedOperationException("Salt is not stored separately, so test should be initialized" - + " with the password hashes only. Use the other constructor"); - } + verifyCorrectConstructorIsUsed(method, true); this.method = method; hashes = ImmutableMap.of( @@ -107,6 +101,16 @@ public abstract class AbstractEncryptionMethodTest { TestHelper.setupLogger(); } + protected void verifyCorrectConstructorIsUsed(EncryptionMethod method, boolean isConstructorWithSalt) { + if (isConstructorWithSalt && !method.hasSeparateSalt()) { + throw new UnsupportedOperationException("Salt is not stored separately, so test should be initialized" + + " with the password hashes only. Use the other constructor"); + } else if (!isConstructorWithSalt && method.hasSeparateSalt()) { + throw new UnsupportedOperationException("Test must be initialized with HashedPassword objects if " + + "the salt is stored separately. Use the other constructor"); + } + } + @Test public void testGivenPasswords() { // Start with the 2nd to last password if we skip long tests diff --git a/src/test/java/fr/xephi/authme/security/crypts/SmfTest.java b/src/test/java/fr/xephi/authme/security/crypts/SmfTest.java index 53803334a..442d5194c 100644 --- a/src/test/java/fr/xephi/authme/security/crypts/SmfTest.java +++ b/src/test/java/fr/xephi/authme/security/crypts/SmfTest.java @@ -1,5 +1,11 @@ package fr.xephi.authme.security.crypts; +import org.junit.Test; + +import static fr.xephi.authme.AuthMeMatchers.stringWithLength; +import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertThat; + /** * Test for {@link Smf}. */ @@ -13,4 +19,25 @@ public class SmfTest extends AbstractEncryptionMethodTest { "03cca5af1eb0a93be47777651b2e7be4fd5d537d"); // âË_3(íù* } + @Override + protected void verifyCorrectConstructorIsUsed(EncryptionMethod method, boolean isSaltConstructor) { + // Smf declares to use a separate salt, but it's not used in the actual hashing mechanism, see Smf class Javadoc + assertThat(method.hasSeparateSalt(), equalTo(true)); + assertThat(isSaltConstructor, equalTo(false)); + } + + @Test + public void shouldGenerateFourCharSalt() { + // given + EncryptionMethod method = new Smf(); + + // when / then + assertThat(method.hasSeparateSalt(), equalTo(true)); + for (int i = 0; i < 3; ++i) { + HashedPassword hash = method.computeHash("pw", "name"); + String salt = hash.getSalt(); + assertThat(salt, stringWithLength(4)); + assertThat(salt.matches("[a-z0-9]{4}"), equalTo(true)); + } + } }