#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
This commit is contained in:
ljacqu 2015-12-29 13:29:26 +01:00
parent 531327dd9b
commit 922082f312
13 changed files with 123 additions and 76 deletions

View File

@ -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

View File

@ -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

View File

@ -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;
}
}

View File

@ -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;
}
}

View File

@ -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

View File

@ -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;
}
}

View File

@ -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)

View File

@ -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 {
}

View File

@ -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<Class<? extends EncryptionMethod>> 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));
}
}
}
}

View File

@ -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<String> 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<String> 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;
}
}

View File

@ -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() {

View File

@ -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() {

View File

@ -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;
}
}