diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 876af0fa4c..34efbb937a 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -45,6 +45,7 @@ The type attribute can be add,update,fix,remove. + Sha2Crypt: tidy up salt Javadoc and validation. Bump org.apache.commons:commons-lang3 from 3.14.0 to 3.15.0 #296. diff --git a/src/main/java/org/apache/commons/codec/digest/Sha2Crypt.java b/src/main/java/org/apache/commons/codec/digest/Sha2Crypt.java index e20a3a2ce7..f9277b53a6 100644 --- a/src/main/java/org/apache/commons/codec/digest/Sha2Crypt.java +++ b/src/main/java/org/apache/commons/codec/digest/Sha2Crypt.java @@ -67,9 +67,12 @@ public class Sha2Crypt { /** The prefixes that can be used to identify this crypt() variant (SHA-512). */ static final String SHA512_PREFIX = "$6$"; - /** The pattern to match valid salt values. */ + /** + * The pattern to match valid salt values. + * $[56]$(rounds=nn$)?[./a-zA-Z0-9]{1,16}.* + */ private static final Pattern SALT_PATTERN = Pattern - .compile("^\\$([56])\\$(rounds=(\\d+)\\$)?([\\.\\/a-zA-Z0-9]{1,16}).*"); + .compile("^(\\$[56]\\$)(rounds=(\\d+)\\$)?([\\.\\/a-zA-Z0-9]{1,16}).*"); /** * Generates a libc crypt() compatible "$5$" hash value with random salt. @@ -98,9 +101,9 @@ public static String sha256Crypt(final byte[] keyBytes) { * @param keyBytes * plaintext to hash. Each array element is set to {@code 0} before returning. * @param salt - * real salt value without prefix or "rounds=". The salt may be null, in which case a salt - * is generated for you using {@link SecureRandom}. If one does not want to use {@link SecureRandom}, - * you can pass your own {@link Random} in {@link #sha256Crypt(byte[], String, Random)}. + * salt value including prefix ($5$) and optionally "rounds=". + * The salt may be null, in which case a salt is generated for you using {@link SecureRandom}. + * Or you can pass your own {@link Random} in {@link #sha256Crypt(byte[], String, Random)}. * @return complete hash value including salt * @throws IllegalArgumentException * if the salt does not match the allowed pattern @@ -122,7 +125,8 @@ public static String sha256Crypt(final byte[] keyBytes, String salt) { * @param keyBytes * plaintext to hash. Each array element is set to {@code 0} before returning. * @param salt - * real salt value without prefix or "rounds=". + * salt value including prefix ($5$) and optionally "rounds=". + * The salt may be null, in which case a salt is generated for you using the provided random generator * @param random * the instance of {@link Random} to use for generating the salt. * Consider using {@link SecureRandom} for more secure salts. @@ -153,9 +157,9 @@ public static String sha256Crypt(final byte[] keyBytes, String salt, final Rando * @param keyBytes * plaintext to hash. Each array element is set to {@code 0} before returning. * @param salt - * real salt value without prefix or "rounds="; may not be null + * salt value including prefix ($5$ or $6$) and optionally "rounds=". Not null. * @param saltPrefix - * either $5$ or $6$ + * either $5$ or $6$; must agree with prefix in salt itself * @param blocksize * a value that differs between $5$ and $6$ * @param algorithm @@ -183,6 +187,10 @@ private static String sha2Crypt(final byte[] keyBytes, final String salt, final if (!m.find()) { throw new IllegalArgumentException("Invalid salt value: " + salt); } + final String saltLeader = m.group(1); + if (!saltLeader.equals(saltPrefix)) { + throw new IllegalArgumentException("Expected salt to start with: " + saltPrefix + " but was: " + salt); + } if (m.group(3) != null) { rounds = Integer.parseInt(m.group(3)); rounds = Math.max(ROUNDS_MIN, Math.min(ROUNDS_MAX, rounds)); @@ -570,10 +578,9 @@ public static String sha512Crypt(final byte[] keyBytes) { * @param keyBytes * plaintext to hash. Each array element is set to {@code 0} before returning. * @param salt - * real salt value without prefix or "rounds=". The salt may be null, in which case a salt is generated - * for you using {@link SecureRandom}; if you want to use a {@link Random} object other than - * {@link SecureRandom} then we suggest you provide it using - * {@link #sha512Crypt(byte[], String, Random)}. + * salt value including prefix ($6$) and optionally "rounds=". + * The salt may be null, in which case a salt is generated for you using {@link SecureRandom}. + * Or you can pass your own {@link Random} to {@link #sha512Crypt(byte[], String, Random)}. * @return complete hash value including salt * @throws IllegalArgumentException * if the salt does not match the allowed pattern @@ -595,8 +602,8 @@ public static String sha512Crypt(final byte[] keyBytes, String salt) { * @param keyBytes * plaintext to hash. Each array element is set to {@code 0} before returning. * @param salt - * real salt value without prefix or "rounds=". The salt may be null, in which case a salt - * is generated for you using {@link SecureRandom}. + * salt value including prefix ($6$) and optionally "rounds=". + * The salt may be null, in which case a salt is generated for you using the provided random generator * @param random * the instance of {@link Random} to use for generating the salt. * Consider using {@link SecureRandom} for more secure salts. diff --git a/src/test/java/org/apache/commons/codec/digest/Sha256CryptTest.java b/src/test/java/org/apache/commons/codec/digest/Sha256CryptTest.java index cf68a473ed..f57f7131ff 100644 --- a/src/test/java/org/apache/commons/codec/digest/Sha256CryptTest.java +++ b/src/test/java/org/apache/commons/codec/digest/Sha256CryptTest.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertThrowsExactly; import static org.junit.jupiter.api.Assertions.assertTrue; import java.nio.charset.StandardCharsets; @@ -103,4 +104,8 @@ public void testZeroOutInput() { assertArrayEquals(new byte[buffer.length], buffer); } + @Test + public void testCodec182() { + assertThrowsExactly(IllegalArgumentException.class, () -> Sha2Crypt.sha256Crypt("secret".getBytes(StandardCharsets.UTF_8), "$6$abcd$")); + } } diff --git a/src/test/java/org/apache/commons/codec/digest/Sha512CryptTest.java b/src/test/java/org/apache/commons/codec/digest/Sha512CryptTest.java index 28e87d1e22..019987f834 100644 --- a/src/test/java/org/apache/commons/codec/digest/Sha512CryptTest.java +++ b/src/test/java/org/apache/commons/codec/digest/Sha512CryptTest.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertThrowsExactly; import static org.junit.jupiter.api.Assertions.assertTrue; import java.nio.charset.StandardCharsets; @@ -112,4 +113,8 @@ public void testZeroOutInput() { assertArrayEquals(new byte[buffer.length], buffer); } + @Test + public void testCodec182() { + assertThrowsExactly(IllegalArgumentException.class, () -> Sha2Crypt.sha512Crypt("secret".getBytes(StandardCharsets.UTF_8), "$5$abcd$")); + } }