From 922082f3126a2b34ae468263fc87125464c52eb2 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Tue, 29 Dec 2015 13:29:26 +0100 Subject: [PATCH] #364 Add HashAlgorithm integration test, fix failing tests - Create integration test for the HashAlgorithm enum - Create AsciiRestricted annotation and make test aware of it - Add option to skip "same hash for same salt" test (for wordpress) - Change some EncryptionMethods to extend from a common superclass --- .../xephi/authme/security/crypts/BCRYPT.java | 2 +- .../security/crypts/CryptPBKDF2Django.java | 2 + .../fr/xephi/authme/security/crypts/IPB3.java | 17 +----- .../fr/xephi/authme/security/crypts/MYBB.java | 17 +----- .../xephi/authme/security/crypts/PHPBB.java | 13 ++-- .../authme/security/crypts/PHPFUSION.java | 19 +----- .../authme/security/crypts/SALTED2MD5.java | 6 -- .../crypts/description/AsciiRestricted.java | 15 +++++ .../HashAlgorithmIntegrationTest.java | 61 +++++++++++++++++++ .../crypts/AbstractEncryptionMethodTest.java | 29 ++++++++- .../crypts/CryptPBKDF2DjangoTest.java | 4 -- .../authme/security/crypts/PHPFUSIONTest.java | 4 -- .../authme/security/crypts/WORDPRESSTest.java | 10 +-- 13 files changed, 123 insertions(+), 76 deletions(-) create mode 100644 src/main/java/fr/xephi/authme/security/crypts/description/AsciiRestricted.java create mode 100644 src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java diff --git a/src/main/java/fr/xephi/authme/security/crypts/BCRYPT.java b/src/main/java/fr/xephi/authme/security/crypts/BCRYPT.java index b85a83df4..3c0bc785f 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/BCRYPT.java +++ b/src/main/java/fr/xephi/authme/security/crypts/BCRYPT.java @@ -522,7 +522,7 @@ public class BCRYPT implements EncryptionMethod { @Override public HashResult computeHash(String password, String name) { String salt = generateSalt(); - return new HashResult(hashpw(password, salt), salt); + return new HashResult(hashpw(password, salt), null); } @Override diff --git a/src/main/java/fr/xephi/authme/security/crypts/CryptPBKDF2Django.java b/src/main/java/fr/xephi/authme/security/crypts/CryptPBKDF2Django.java index dd20c1de6..3c9b5d602 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/CryptPBKDF2Django.java +++ b/src/main/java/fr/xephi/authme/security/crypts/CryptPBKDF2Django.java @@ -1,10 +1,12 @@ package fr.xephi.authme.security.crypts; +import fr.xephi.authme.security.crypts.description.AsciiRestricted; import fr.xephi.authme.security.pbkdf2.PBKDF2Engine; import fr.xephi.authme.security.pbkdf2.PBKDF2Parameters; import javax.xml.bind.DatatypeConverter; +@AsciiRestricted public class CryptPBKDF2Django extends HexSaltedMethod { @Override diff --git a/src/main/java/fr/xephi/authme/security/crypts/IPB3.java b/src/main/java/fr/xephi/authme/security/crypts/IPB3.java index b89c8f32e..85789b881 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/IPB3.java +++ b/src/main/java/fr/xephi/authme/security/crypts/IPB3.java @@ -10,31 +10,16 @@ import static fr.xephi.authme.security.HashUtils.md5; @Recommendation(Usage.DO_NOT_USE) @HasSalt(value = SaltType.TEXT, length = 5) -public class IPB3 implements EncryptionMethod { +public class IPB3 extends SeparateSaltMethod { @Override public String computeHash(String password, String salt, String name) { return md5(md5(salt) + md5(password)); } - @Override - public HashResult computeHash(String password, String name) { - String salt = generateSalt(); - return new HashResult(computeHash(password, salt, name), salt); - } - - @Override - public boolean comparePassword(String hash, String password, String salt, String name) { - return hash.equals(computeHash(password, salt, name)); - } - @Override public String generateSalt() { return RandomString.generateHex(5); } - @Override - public boolean hasSeparateSalt() { - return true; - } } diff --git a/src/main/java/fr/xephi/authme/security/crypts/MYBB.java b/src/main/java/fr/xephi/authme/security/crypts/MYBB.java index fe4b287ec..97418640c 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/MYBB.java +++ b/src/main/java/fr/xephi/authme/security/crypts/MYBB.java @@ -4,31 +4,16 @@ import fr.xephi.authme.security.RandomString; import static fr.xephi.authme.security.HashUtils.md5; -public class MYBB implements EncryptionMethod { +public class MYBB extends SeparateSaltMethod { @Override public String computeHash(String password, String salt, String name) { return md5(md5(salt) + md5(password)); } - @Override - public HashResult computeHash(String password, String name) { - String salt = generateSalt(); - return new HashResult(computeHash(password, salt, name), salt); - } - - @Override - public boolean comparePassword(String hash, String password, String salt, String name) { - return hash.equals(computeHash(password, salt, name)); - } - @Override public String generateSalt() { return RandomString.generateHex(8); } - @Override - public boolean hasSeparateSalt() { - return true; - } } diff --git a/src/main/java/fr/xephi/authme/security/crypts/PHPBB.java b/src/main/java/fr/xephi/authme/security/crypts/PHPBB.java index cd0396158..753f419a9 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/PHPBB.java +++ b/src/main/java/fr/xephi/authme/security/crypts/PHPBB.java @@ -4,10 +4,10 @@ */ package fr.xephi.authme.security.crypts; -import fr.xephi.authme.security.RandomString; +import fr.xephi.authme.security.HashUtils; +import fr.xephi.authme.security.MessageDigestAlgorithm; import java.io.UnsupportedEncodingException; -import java.security.GeneralSecurityException; import java.security.MessageDigest; /** @@ -20,10 +20,10 @@ public class PHPBB extends HexSaltedMethod { private static String md5(String data) { try { byte[] bytes = data.getBytes("ISO-8859-1"); - MessageDigest md5er = MessageDigest.getInstance("MD5"); + MessageDigest md5er = HashUtils.getDigest(MessageDigestAlgorithm.MD5); byte[] hash = md5er.digest(bytes); return bytes2hex(hash); - } catch (GeneralSecurityException | UnsupportedEncodingException e) { + } catch (UnsupportedEncodingException e) { throw new RuntimeException(e); } } @@ -132,9 +132,10 @@ public class PHPBB extends HexSaltedMethod { } private boolean phpbb_check_hash(String password, String hash) { - if (hash.length() == 34) + if (hash.length() == 34) { return _hash_crypt_private(password, hash).equals(hash); - else return md5(password).equals(hash); + } + return md5(password).equals(hash); } @Override diff --git a/src/main/java/fr/xephi/authme/security/crypts/PHPFUSION.java b/src/main/java/fr/xephi/authme/security/crypts/PHPFUSION.java index 596f994e6..2950fe670 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/PHPFUSION.java +++ b/src/main/java/fr/xephi/authme/security/crypts/PHPFUSION.java @@ -2,6 +2,7 @@ package fr.xephi.authme.security.crypts; import fr.xephi.authme.security.HashUtils; import fr.xephi.authme.security.RandomString; +import fr.xephi.authme.security.crypts.description.AsciiRestricted; import fr.xephi.authme.security.crypts.description.Recommendation; import fr.xephi.authme.security.crypts.description.Usage; @@ -12,7 +13,8 @@ import java.security.InvalidKeyException; import java.security.NoSuchAlgorithmException; @Recommendation(Usage.DO_NOT_USE) -public class PHPFUSION implements EncryptionMethod { +@AsciiRestricted +public class PHPFUSION extends SeparateSaltMethod { @Override public String computeHash(String password, String salt, String name) { @@ -37,25 +39,10 @@ public class PHPFUSION implements EncryptionMethod { } } - @Override - public HashResult computeHash(String password, String name) { - String salt = generateSalt(); - return new HashResult(computeHash(password, salt, name), salt); - } - - @Override - public boolean comparePassword(String hash, String password, String salt, String name) { - return hash.equals(computeHash(password, salt, name)); - } - @Override public String generateSalt() { return RandomString.generateHex(12); } - @Override - public boolean hasSeparateSalt() { - return true; - } } diff --git a/src/main/java/fr/xephi/authme/security/crypts/SALTED2MD5.java b/src/main/java/fr/xephi/authme/security/crypts/SALTED2MD5.java index 331c33fa9..613bcd3f8 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/SALTED2MD5.java +++ b/src/main/java/fr/xephi/authme/security/crypts/SALTED2MD5.java @@ -1,7 +1,5 @@ package fr.xephi.authme.security.crypts; -import fr.xephi.authme.AuthMe; -import fr.xephi.authme.security.HashUtils; import fr.xephi.authme.security.RandomString; import fr.xephi.authme.security.crypts.description.HasSalt; import fr.xephi.authme.security.crypts.description.Recommendation; @@ -9,10 +7,6 @@ import fr.xephi.authme.security.crypts.description.SaltType; import fr.xephi.authme.security.crypts.description.Usage; import fr.xephi.authme.settings.Settings; -import java.math.BigInteger; -import java.security.MessageDigest; -import java.security.NoSuchAlgorithmException; - import static fr.xephi.authme.security.HashUtils.md5; @Recommendation(Usage.ACCEPTABLE) // presuming that length is something sensible (>= 8) diff --git a/src/main/java/fr/xephi/authme/security/crypts/description/AsciiRestricted.java b/src/main/java/fr/xephi/authme/security/crypts/description/AsciiRestricted.java new file mode 100644 index 000000000..f515717e7 --- /dev/null +++ b/src/main/java/fr/xephi/authme/security/crypts/description/AsciiRestricted.java @@ -0,0 +1,15 @@ +package fr.xephi.authme.security.crypts.description; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Denotes an encryption algorithm that is restricted to the ASCII charset. + */ +@Target(ElementType.TYPE) +@Retention(RetentionPolicy.RUNTIME) +public @interface AsciiRestricted { + +} diff --git a/src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java b/src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java new file mode 100644 index 000000000..35e3b5776 --- /dev/null +++ b/src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java @@ -0,0 +1,61 @@ +package fr.xephi.authme.security; + +import fr.xephi.authme.security.crypts.EncryptionMethod; +import fr.xephi.authme.security.crypts.HashResult; +import fr.xephi.authme.settings.Settings; +import fr.xephi.authme.util.StringUtils; +import fr.xephi.authme.util.WrapperMock; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.util.HashSet; +import java.util.Set; + +import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; + +/** + * Integration test for {@link HashAlgorithm}. + */ +public class HashAlgorithmIntegrationTest { + + @BeforeClass + public static void setUpWrapper() { + WrapperMock.createInstance(); + Settings.bCryptLog2Rounds = 8; + Settings.saltLength = 16; + } + + @Test + public void shouldHaveUniqueClassForEntries() { + // given + Set> classes = new HashSet<>(); + + // when / then + for (HashAlgorithm algorithm : HashAlgorithm.values()) { + if (!HashAlgorithm.CUSTOM.equals(algorithm)) { + if (classes.contains(algorithm.getClazz())) { + fail("Found class '" + algorithm.getClazz() + "' twice!"); + } + classes.add(algorithm.getClazz()); + } + } + } + + @Test + public void shouldBeAbleToInstantiateEncryptionAlgorithms() throws InstantiationException, IllegalAccessException { + // given / when / then + for (HashAlgorithm algorithm : HashAlgorithm.values()) { + if (!HashAlgorithm.CUSTOM.equals(algorithm)) { + EncryptionMethod method = algorithm.getClazz().newInstance(); + HashResult hashResult = method.computeHash("pwd", "name"); + assertThat("Salt should not be null if method.hasSeparateSalt(), and vice versa. Method: '" + + method + "'", StringUtils.isEmpty(hashResult.getSalt()), equalTo(!method.hasSeparateSalt())); + assertThat("Hash should not be empty for method '" + method + "'", + StringUtils.isEmpty(hashResult.getHash()), equalTo(false)); + } + } + } + +} 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 a6d8163db..ad3d183ff 100644 --- a/src/test/java/fr/xephi/authme/security/crypts/AbstractEncryptionMethodTest.java +++ b/src/test/java/fr/xephi/authme/security/crypts/AbstractEncryptionMethodTest.java @@ -1,8 +1,11 @@ package fr.xephi.authme.security.crypts; +import com.google.common.collect.ImmutableList; +import fr.xephi.authme.security.crypts.description.AsciiRestricted; import org.junit.Test; import java.util.HashMap; +import java.util.List; import java.util.Map; import static org.hamcrest.Matchers.equalTo; @@ -27,7 +30,8 @@ public abstract class AbstractEncryptionMethodTest { * List of passwords that are hashed at runtime and then tested against; this verifies that hashes that are * generated are valid. */ - private static final String[] INTERNAL_PASSWORDS = {"test1234", "Ab_C73", "(!#&$~`_-Aa0", "Ûïé1&?+A"}; + private static final List INTERNAL_PASSWORDS = + ImmutableList.of("test1234", "Ab_C73", "(!#&$~`_-Aa0", "Ûïé1&?+A"); /** The encryption method to test. */ private EncryptionMethod method; @@ -97,12 +101,19 @@ public abstract class AbstractEncryptionMethodTest { @Test public void testPasswordEquality() { - for (String password : INTERNAL_PASSWORDS) { + List internalPasswords = method.getClass().isAnnotationPresent(AsciiRestricted.class) + ? INTERNAL_PASSWORDS.subList(0, INTERNAL_PASSWORDS.size() - 1) + : INTERNAL_PASSWORDS; + + for (String password : internalPasswords) { final String salt = method.generateSalt(); final String hash = method.computeHash(password, salt, USERNAME); // Check that the computeHash(password, salt, name) method has the same output for the returned salt - assertThat(hash, equalTo(method.computeHash(password, salt, USERNAME))); + if (testHashEqualityForSameSalt()) { + assertThat("Computing a hash with the same salt will generate the same hash", + hash, equalTo(method.computeHash(password, salt, USERNAME))); + } assertTrue("Generated hash for '" + password + "' should match password (hash = '" + hash + "')", method.comparePassword(hash, password, salt, USERNAME)); @@ -151,4 +162,16 @@ public abstract class AbstractEncryptionMethodTest { System.out.println("\n}"); } + /** + * Return whether an encryption algorithm should be tested that it generates the same + * hash for the same salt. If {@code true}, we call {@link EncryptionMethod#computeHash(String, String)} + * and verify that {@link EncryptionMethod#computeHash(String, String, String)} generates + * the same hash for the salt returned in the first call. + * + * @return Whether or not to test that the hash is the same for the same salt + */ + protected boolean testHashEqualityForSameSalt() { + return true; + } + } diff --git a/src/test/java/fr/xephi/authme/security/crypts/CryptPBKDF2DjangoTest.java b/src/test/java/fr/xephi/authme/security/crypts/CryptPBKDF2DjangoTest.java index f2f22edfe..afff6bf87 100644 --- a/src/test/java/fr/xephi/authme/security/crypts/CryptPBKDF2DjangoTest.java +++ b/src/test/java/fr/xephi/authme/security/crypts/CryptPBKDF2DjangoTest.java @@ -1,12 +1,8 @@ package fr.xephi.authme.security.crypts; -import org.junit.Ignore; - /** * Test for {@link CryptPBKDF2Django}. */ -@Ignore -// TODO ljacqu 20151220: testPasswordEquality fails - password matches hash for uppercase password...? public class CryptPBKDF2DjangoTest extends AbstractEncryptionMethodTest { public CryptPBKDF2DjangoTest() { diff --git a/src/test/java/fr/xephi/authme/security/crypts/PHPFUSIONTest.java b/src/test/java/fr/xephi/authme/security/crypts/PHPFUSIONTest.java index 828f6d482..fe2259d23 100644 --- a/src/test/java/fr/xephi/authme/security/crypts/PHPFUSIONTest.java +++ b/src/test/java/fr/xephi/authme/security/crypts/PHPFUSIONTest.java @@ -1,12 +1,8 @@ package fr.xephi.authme.security.crypts; -import org.junit.Ignore; - /** * Test for {@link PHPFUSION}. */ -@Ignore -// TODO #364: Need to skip lowercase/uppercase password test for the non-ASCII one public class PHPFUSIONTest extends AbstractEncryptionMethodTest { public PHPFUSIONTest() { diff --git a/src/test/java/fr/xephi/authme/security/crypts/WORDPRESSTest.java b/src/test/java/fr/xephi/authme/security/crypts/WORDPRESSTest.java index 071659344..49a16d65b 100644 --- a/src/test/java/fr/xephi/authme/security/crypts/WORDPRESSTest.java +++ b/src/test/java/fr/xephi/authme/security/crypts/WORDPRESSTest.java @@ -1,12 +1,8 @@ package fr.xephi.authme.security.crypts; -import org.junit.Ignore; - /** * Test for {@link WORDPRESS}. */ -@Ignore -// TODO #364: Need to skip an assertion due to the "internal salt" of Wordpress public class WORDPRESSTest extends AbstractEncryptionMethodTest { public WORDPRESSTest() { @@ -16,4 +12,10 @@ public class WORDPRESSTest extends AbstractEncryptionMethodTest { "$P$BjzPjjzPjrAOyB1V0WFdpisgCTFx.N/", // &^%te$t?Pw@_ "$P$BjzPjxxyjp2QdKcab/oTW8l/W0AgE21"); // âË_3(íù* } + + @Override + protected boolean testHashEqualityForSameSalt() { + // We need to skip the test because Wordpress uses an "internal salt" that is not exposed to the outside + return false; + } }