Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CODEC-182 - Sha2Crypt: tidy up salt Javadoc and validation. #301

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

CODEC-182 - Sha2Crypt: tidy up salt Javadoc and validation. #301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a9af847
CODEC-182 - check salt prefix for consistency
sebbASF Jul 30, 2024
6061d15
Change doc
sebbASF Jul 30, 2024
e2f8ea4
Doc tweak [skip ci]
sebbASF Jul 30, 2024
394cde5
Must use correct prefix for algorithm
sebbASF Aug 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/changes/changes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ The <action> type attribute can be add,update,fix,remove.
<body>
<release version="1.17.2" date="YYYY-MM-DD" description="This is a feature and maintenance release. Java 8 or later is required.">
<!-- FIX -->
<action issue="CODEC-182" dev="sebb" type="fix" due-to="Felix Kaser">Sha2Crypt: tidy up salt Javadoc and validation.</action>
<!-- ADD -->
<!-- UPDATE -->
<action type="update" dev="ggregory" due-to="Dependabot">Bump org.apache.commons:commons-lang3 from 3.14.0 to 3.15.0 #296.</action>
Expand Down
35 changes: 21 additions & 14 deletions src/main/java/org/apache/commons/codec/digest/Sha2Crypt.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
* <code> $[56]$(rounds=nn$)?[./a-zA-Z0-9]{1,16}.* </code>
*/
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.
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down
5 changes: 5 additions & 0 deletions src/test/java/org/apache/commons/codec/digest/Sha256CryptTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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$"));
}
}
5 changes: 5 additions & 0 deletions src/test/java/org/apache/commons/codec/digest/Sha512CryptTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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$"));
}
}