From a9af84752b585aaa952338083927c1bfc14db788 Mon Sep 17 00:00:00 2001 From: Sebb Date: Tue, 30 Jul 2024 20:55:01 +0100 Subject: [PATCH 1/4] CODEC-182 - check salt prefix for consistency --- .../commons/codec/digest/Sha2Crypt.java | 30 +++++++++++-------- .../commons/codec/digest/Sha256CryptTest.java | 5 ++++ .../commons/codec/digest/Sha512CryptTest.java | 5 ++++ 3 files changed, 27 insertions(+), 13 deletions(-) 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..2bfe62bdb9 100644 --- a/src/main/java/org/apache/commons/codec/digest/Sha2Crypt.java +++ b/src/main/java/org/apache/commons/codec/digest/Sha2Crypt.java @@ -69,7 +69,7 @@ public class Sha2Crypt { /** The pattern to match valid salt values. */ 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 +98,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$ or $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} 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 +122,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$ or $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. @@ -153,9 +154,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 +184,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 +575,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 ($5$ or $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 +599,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 ($5$ or $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$")); + } } From 6061d157a5594562cb298c50722c07ade629a830 Mon Sep 17 00:00:00 2001 From: Sebb Date: Tue, 30 Jul 2024 20:57:36 +0100 Subject: [PATCH 2/4] Change doc --- src/changes/changes.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 876af0fa4c..58e66a2836 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -45,6 +45,7 @@ The type attribute can be add,update,fix,remove. + Tidy up salt Javadoc and validation. Bump org.apache.commons:commons-lang3 from 3.14.0 to 3.15.0 #296. From e2f8ea48d81d7c96d5ee631995b58d894de259bf Mon Sep 17 00:00:00 2001 From: Sebb Date: Tue, 30 Jul 2024 20:59:55 +0100 Subject: [PATCH 3/4] Doc tweak [skip ci] --- src/changes/changes.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 58e66a2836..34efbb937a 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -45,7 +45,7 @@ The type attribute can be add,update,fix,remove. - Tidy up salt Javadoc and validation. + Sha2Crypt: tidy up salt Javadoc and validation. Bump org.apache.commons:commons-lang3 from 3.14.0 to 3.15.0 #296. From 394cde5ec5a1994abeff457ff36685e8dca3c88f Mon Sep 17 00:00:00 2001 From: Sebb Date: Fri, 2 Aug 2024 17:14:14 +0100 Subject: [PATCH 4/4] Must use correct prefix for algorithm --- .../org/apache/commons/codec/digest/Sha2Crypt.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) 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 2bfe62bdb9..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,7 +67,10 @@ 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}).*"); @@ -98,7 +101,7 @@ public static String sha256Crypt(final byte[] keyBytes) { * @param keyBytes * plaintext to hash. Each array element is set to {@code 0} before returning. * @param salt - * salt value including prefix ($5$ or $6$) and optionally "rounds=". + * 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 @@ -122,7 +125,7 @@ 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 - * salt value including prefix ($5$ or $6$) and optionally "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. @@ -575,7 +578,7 @@ public static String sha512Crypt(final byte[] keyBytes) { * @param keyBytes * plaintext to hash. Each array element is set to {@code 0} before returning. * @param salt - * salt value including prefix ($5$ or $6$) and optionally "rounds=". + * 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 @@ -599,7 +602,7 @@ 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 - * salt value including prefix ($5$ or $6$) and optionally "rounds=". + * 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.