This is an automated email from the ASF dual-hosted git repository. rzo1 pushed a commit to branch tomee-10.x in repository https://gitbox.apache.org/repos/asf/tomee.git
commit 7b937d668427a38bedc4cdafd13f600b0f8e716f Author: Richard Zowalla <[email protected]> AuthorDate: Tue Oct 13 08:48:04 2026 +0200 Validate stored parameters in TomEEPbkdf2PasswordHash (#3073) Only accept supported PBKDF2 algorithms and at least 1024 iterations; malformed hashes fail verification. Set tomee.security.pbkdf2.allow-weak-parameters=true to accept legacy hashes. (cherry picked from commit 5875ad780d59061467f9767ba9d2dce4786cf8b1) --- .../tomee/security/TomEEPbkdf2PasswordHash.java | 115 ++++++++--- .../security/TomEEPbkdf2PasswordHashTest.java | 216 +++++++++++++++++++++ 2 files changed, 308 insertions(+), 23 deletions(-) diff --git a/tomee/tomee-security/src/main/java/org/apache/tomee/security/TomEEPbkdf2PasswordHash.java b/tomee/tomee-security/src/main/java/org/apache/tomee/security/TomEEPbkdf2PasswordHash.java index aa639e4c6d..017aba3e92 100644 --- a/tomee/tomee-security/src/main/java/org/apache/tomee/security/TomEEPbkdf2PasswordHash.java +++ b/tomee/tomee-security/src/main/java/org/apache/tomee/security/TomEEPbkdf2PasswordHash.java @@ -16,6 +16,10 @@ */ package org.apache.tomee.security; +import org.apache.openejb.loader.SystemInstance; +import org.apache.openejb.util.LogCategory; +import org.apache.openejb.util.Logger; + import javax.crypto.SecretKeyFactory; import javax.crypto.spec.PBEKeySpec; import jakarta.enterprise.context.Dependent; @@ -25,6 +29,9 @@ import java.security.SecureRandom; import java.security.spec.InvalidKeySpecException; import java.util.Base64; import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; @Dependent // important because it's tight to the identity store it's injected into public class TomEEPbkdf2PasswordHash implements Pbkdf2PasswordHash { @@ -36,6 +43,26 @@ public class TomEEPbkdf2PasswordHash implements Pbkdf2PasswordHash { public static final int HASH_BYTE_SIZE = 18; public static final int PBKDF2_ITERATIONS = 64000; + // algorithms allowed by the spec, plus SHA1 which is the historical default of this class + private static final Set<String> SUPPORTED_ALGORITHMS = Set.of( + "PBKDF2WithHmacSHA1", + "PBKDF2WithHmacSHA224", + "PBKDF2WithHmacSHA256", + "PBKDF2WithHmacSHA384", + "PBKDF2WithHmacSHA512"); + + private static final int MIN_ITERATIONS = 1024; + + // opt-out to accept hashes (and configuration) with weak parameters, e.g. legacy hashes + public static final String ALLOW_WEAK_PARAMETERS = "tomee.security.pbkdf2.allow-weak-parameters"; + + private static final Logger LOGGER = Logger.getInstance(LogCategory.TOMEE_SECURITY, TomEEPbkdf2PasswordHash.class); + + // warn once per JVM when rejecting, once per distinct algorithm/iterations combination when accepting + private static final int MAX_WARNED_PARAMETERS = 64; + private static final AtomicBoolean WARNED_REJECTED = new AtomicBoolean(); + private static final Set<String> WARNED_PARAMETERS = ConcurrentHashMap.newKeySet(); + private SecureRandom random = new SecureRandom(); // These are configured by default to constants above, but can be overridden with parameters in initialize() @@ -46,15 +73,30 @@ public class TomEEPbkdf2PasswordHash implements Pbkdf2PasswordHash { @Override public void initialize(final Map<String, String> parameters) { - // todo read from parameters and set fields final String algorithmParameter = parameters.get("Pbkdf2PasswordHash.Algorithm"); - if (algorithmParameter != null) { // todo also check withing a list of supported algorithm maybe + if (algorithmParameter != null) { + if (!SUPPORTED_ALGORITHMS.contains(algorithmParameter)) { + final String message = "Unsupported algorithm " + algorithmParameter + + ". Supported algorithms are " + SUPPORTED_ALGORITHMS; + if (!allowWeakParameters()) { + throw new IllegalArgumentException(message); + } + LOGGER.warning(message + ". Accepted because " + ALLOW_WEAK_PARAMETERS + " is true"); + } configuredAlgorithm = algorithmParameter; } final String iterationsParameter = parameters.get("Pbkdf2PasswordHash.Iterations"); if (iterationsParameter != null) { - configuredIterations = Integer.parseInt(iterationsParameter); + final int iterations = Integer.parseInt(iterationsParameter); + if (iterations < MIN_ITERATIONS) { + final String message = "Invalid number of iterations " + iterations + ". Must be >= " + MIN_ITERATIONS; + if (iterations < 1 || !allowWeakParameters()) { + throw new IllegalArgumentException(message); + } + LOGGER.warning(message + ". Accepted because " + ALLOW_WEAK_PARAMETERS + " is true"); + } + configuredIterations = iterations; } final String saltSizeParameter = parameters.get("Pbkdf2PasswordHash.SaltSizeBytes"); @@ -80,47 +122,74 @@ public class TomEEPbkdf2PasswordHash implements Pbkdf2PasswordHash { @Override public boolean verify(final char[] password, final String hashedPassword) { + if (password == null || hashedPassword == null) { + return false; + } - // todo introduce a pojo that has all values and can serialize and deserialize from/to a String - // todo introduce strongly typed exceptions - - final String[] params = hashedPassword.split(":"); + // format: algorithm:iterations:salt:hash, every part comes from storage so validate it + final String[] params = hashedPassword.split(":", -1); if (params.length != 4) { - throw new RuntimeException("Missing fields in hashed password."); + return false; } final String algorithm = params[0]; - // todo check supported algorithms in a finite list? - - int iterations = 0; + final int iterations; try { iterations = Integer.parseInt(params[1]); } catch (final NumberFormatException ex) { - throw new RuntimeException("Could not parse the iteration as an integer.", ex); + return false; + } + if (algorithm.isEmpty() || iterations < 1) { + return false; } - if (iterations < 1) { - throw new RuntimeException("Invalid number of iterations. Must be >= 1."); + final boolean weak = !SUPPORTED_ALGORITHMS.contains(algorithm) || iterations < MIN_ITERATIONS; + if (weak && !allowWeakParameters()) { + if (WARNED_REJECTED.compareAndSet(false, true)) { + LOGGER.warning("Rejected a stored PBKDF2 hash with an unsupported algorithm or fewer than " + + MIN_ITERATIONS + " iterations. Set " + ALLOW_WEAK_PARAMETERS + + "=true to accept legacy hashes"); + } + return false; } - byte[] salt = null; + final byte[] salt; + final byte[] expectedHash; try { salt = fromBase64(params[2]); - } catch (IllegalArgumentException ex) { - throw new RuntimeException("Base64 decoding of salt failed.", ex ); + expectedHash = fromBase64(params[3]); + } catch (final IllegalArgumentException ex) { + return false; + } + if (salt.length == 0 || expectedHash.length == 0) { + return false; } - byte[] expectedHash = null; + final byte[] actual; try { - expectedHash = fromBase64(params[3]); - } catch (IllegalArgumentException ex) { - throw new RuntimeException("Base64 decoding of pbkdf2 output failed.", ex); + actual = pbkdf2(password, salt, iterations, expectedHash.length, algorithm); + } catch (final RuntimeException ex) { + return false; + } + if (weak) { + warnWeak(algorithm, iterations); } - - final byte[] actual = pbkdf2(password, salt, iterations, expectedHash.length, algorithm); return slowEquals(expectedHash, actual); } + private static boolean allowWeakParameters() { + return Boolean.parseBoolean(SystemInstance.get().getProperty(ALLOW_WEAK_PARAMETERS, "false")); + } + + private static void warnWeak(final String algorithm, final int iterations) { + final String key = algorithm + ":" + iterations; + if (WARNED_PARAMETERS.size() < MAX_WARNED_PARAMETERS && WARNED_PARAMETERS.add(key)) { + LOGGER.warning("Accepted a stored PBKDF2 hash with weak parameters (algorithm " + algorithm + + ", " + iterations + " iterations) because " + ALLOW_WEAK_PARAMETERS + + " is true. The hash should be regenerated"); + } + } + private byte[] pbkdf2(final char[] password, final byte[] salt, final int iterations, final int length, final String algorithm) { try { final PBEKeySpec spec = new PBEKeySpec(password, salt, iterations, length * 8); diff --git a/tomee/tomee-security/src/test/java/org/apache/tomee/security/TomEEPbkdf2PasswordHashTest.java b/tomee/tomee-security/src/test/java/org/apache/tomee/security/TomEEPbkdf2PasswordHashTest.java new file mode 100644 index 0000000000..7307b5df97 --- /dev/null +++ b/tomee/tomee-security/src/test/java/org/apache/tomee/security/TomEEPbkdf2PasswordHashTest.java @@ -0,0 +1,216 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomee.security; + +import org.apache.openejb.loader.SystemInstance; +import org.junit.After; +import org.junit.Test; + +import javax.crypto.SecretKeyFactory; +import javax.crypto.spec.PBEKeySpec; +import java.util.Base64; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class TomEEPbkdf2PasswordHashTest { + + private static final char[] PASSWORD = "s3cret".toCharArray(); + + @After + public void resetProperty() { + SystemInstance.get().getProperties().remove(TomEEPbkdf2PasswordHash.ALLOW_WEAK_PARAMETERS); + } + + private static void allowWeakParameters() { + SystemInstance.get().setProperty(TomEEPbkdf2PasswordHash.ALLOW_WEAK_PARAMETERS, "true"); + } + + @Test + public void defaultHashVerifies() { + final TomEEPbkdf2PasswordHash hash = new TomEEPbkdf2PasswordHash(); + final String stored = hash.generate(PASSWORD); + + assertTrue(stored.startsWith("PBKDF2WithHmacSHA1:64000:")); + assertTrue(hash.verify(PASSWORD, stored)); + assertFalse(hash.verify("wrong".toCharArray(), stored)); + } + + @Test + public void nonDefaultParametersVerify() { + final Map<String, String> parameters = new HashMap<>(); + parameters.put("Pbkdf2PasswordHash.Algorithm", "PBKDF2WithHmacSHA512"); + parameters.put("Pbkdf2PasswordHash.Iterations", "1024"); + parameters.put("Pbkdf2PasswordHash.SaltSizeBytes", "64"); + parameters.put("Pbkdf2PasswordHash.KeySizeBytes", "32"); + final TomEEPbkdf2PasswordHash generator = new TomEEPbkdf2PasswordHash(); + generator.initialize(parameters); + + final String stored = generator.generate(PASSWORD); + assertTrue(stored.startsWith("PBKDF2WithHmacSHA512:1024:")); + + // verification relies on the stored parameters, not on the configured ones + assertTrue(new TomEEPbkdf2PasswordHash().verify(PASSWORD, stored)); + assertTrue(generator.verify(PASSWORD, stored)); + } + + @Test + public void allSpecAlgorithmsVerify() throws Exception { + final TomEEPbkdf2PasswordHash hash = new TomEEPbkdf2PasswordHash(); + for (final String algorithm : new String[]{ + "PBKDF2WithHmacSHA224", "PBKDF2WithHmacSHA256", "PBKDF2WithHmacSHA384", "PBKDF2WithHmacSHA512"}) { + assertTrue(algorithm, hash.verify(PASSWORD, stored(algorithm, 2048, PASSWORD))); + } + } + + @Test + public void weakIterationsRejected() throws Exception { + final TomEEPbkdf2PasswordHash hash = new TomEEPbkdf2PasswordHash(); + // correctly computed hashes, but with an iteration count below the minimum + assertFalse(hash.verify(PASSWORD, stored("PBKDF2WithHmacSHA256", 1, PASSWORD))); + assertFalse(hash.verify(PASSWORD, stored("PBKDF2WithHmacSHA256", 1023, PASSWORD))); + assertFalse(hash.verify(PASSWORD, "PBKDF2WithHmacSHA256:-5:AAAA:AAAA")); + } + + @Test + public void unknownAlgorithmRejected() throws Exception { + final TomEEPbkdf2PasswordHash hash = new TomEEPbkdf2PasswordHash(); + final String valid = stored("PBKDF2WithHmacSHA256", 2048, PASSWORD); + assertTrue(hash.verify(PASSWORD, valid)); + assertFalse(hash.verify(PASSWORD, valid.replaceFirst("PBKDF2WithHmacSHA256", "PBKDF2WithHmacMD5"))); + assertFalse(hash.verify(PASSWORD, "HmacSHA256:2048:AAAAAAAA:AAAAAAAA")); + assertFalse(hash.verify(PASSWORD, "DoesNotExist:2048:AAAAAAAA:AAAAAAAA")); + } + + @Test + public void malformedRejected() { + final TomEEPbkdf2PasswordHash hash = new TomEEPbkdf2PasswordHash(); + assertFalse(hash.verify(PASSWORD, null)); + assertFalse(hash.verify(PASSWORD, "")); + assertFalse(hash.verify(PASSWORD, "garbage")); + assertFalse(hash.verify(PASSWORD, "PBKDF2WithHmacSHA256:2048:AAAAAAAA")); + assertFalse(hash.verify(PASSWORD, "PBKDF2WithHmacSHA256:2048:AAAAAAAA:")); + assertFalse(hash.verify(PASSWORD, "PBKDF2WithHmacSHA256:2048:AAAAAAAA:AAAAAAAA:extra")); + assertFalse(hash.verify(PASSWORD, "PBKDF2WithHmacSHA256:abc:AAAAAAAA:AAAAAAAA")); + assertFalse(hash.verify(PASSWORD, "PBKDF2WithHmacSHA256:2048:not*base64:AAAAAAAA")); + assertFalse(hash.verify(PASSWORD, "PBKDF2WithHmacSHA256:2048::AAAAAAAA")); + } + + @Test(expected = IllegalArgumentException.class) + public void initializeRejectsUnknownAlgorithm() { + final Map<String, String> parameters = new HashMap<>(); + parameters.put("Pbkdf2PasswordHash.Algorithm", "PBKDF2WithHmacMD5"); + new TomEEPbkdf2PasswordHash().initialize(parameters); + } + + @Test(expected = IllegalArgumentException.class) + public void initializeRejectsWeakIterations() { + final Map<String, String> parameters = new HashMap<>(); + parameters.put("Pbkdf2PasswordHash.Iterations", "1000"); + new TomEEPbkdf2PasswordHash().initialize(parameters); + } + + @Test + public void weakParametersAcceptedWhenAllowed() throws Exception { + allowWeakParameters(); + final TomEEPbkdf2PasswordHash hash = new TomEEPbkdf2PasswordHash(); + final String fewIterations = stored("PBKDF2WithHmacSHA256", 1, PASSWORD); + assertTrue(hash.verify(PASSWORD, fewIterations)); + assertTrue(hash.verify(PASSWORD, stored("PBKDF2WithHmacSHA256", 1023, PASSWORD))); + assertFalse(hash.verify("wrong".toCharArray(), fewIterations)); + + // an algorithm outside the allowed set but available in the JVM + final String sha512_224 = "PBKDF2WithHmacSHA512/224"; + if (isAvailable(sha512_224)) { + assertTrue(hash.verify(PASSWORD, stored(sha512_224, 2048, PASSWORD))); + } + } + + @Test + public void weakParametersStillRejectedByDefault() throws Exception { + final TomEEPbkdf2PasswordHash hash = new TomEEPbkdf2PasswordHash(); + assertFalse(hash.verify(PASSWORD, stored("PBKDF2WithHmacSHA256", 1, PASSWORD))); + SystemInstance.get().setProperty(TomEEPbkdf2PasswordHash.ALLOW_WEAK_PARAMETERS, "false"); + assertFalse(hash.verify(PASSWORD, stored("PBKDF2WithHmacSHA256", 1, PASSWORD))); + } + + @Test + public void malformedAndUnknownRejectedWhenAllowed() { + allowWeakParameters(); + final TomEEPbkdf2PasswordHash hash = new TomEEPbkdf2PasswordHash(); + assertFalse(hash.verify(PASSWORD, "garbage")); + assertFalse(hash.verify(PASSWORD, "PBKDF2WithHmacSHA256:2048:AAAAAAAA")); + assertFalse(hash.verify(PASSWORD, "PBKDF2WithHmacSHA256:abc:AAAAAAAA:AAAAAAAA")); + assertFalse(hash.verify(PASSWORD, "PBKDF2WithHmacSHA256:2048:not*base64:AAAAAAAA")); + assertFalse(hash.verify(PASSWORD, "PBKDF2WithHmacSHA256:2048::AAAAAAAA")); + assertFalse(hash.verify(PASSWORD, "PBKDF2WithHmacSHA256:0:AAAAAAAA:AAAAAAAA")); + assertFalse(hash.verify(PASSWORD, "PBKDF2WithHmacSHA256:-5:AAAAAAAA:AAAAAAAA")); + assertFalse(hash.verify(PASSWORD, ":2048:AAAAAAAA:AAAAAAAA")); + // not supported by the JVM: fails safely + assertFalse(hash.verify(PASSWORD, "PBKDF2WithHmacMD5:2048:AAAAAAAA:AAAAAAAA")); + assertFalse(hash.verify(PASSWORD, "DoesNotExist:2048:AAAAAAAA:AAAAAAAA")); + } + + @Test + public void initializeAcceptsWeakParametersWhenAllowed() { + allowWeakParameters(); + final Map<String, String> parameters = new HashMap<>(); + parameters.put("Pbkdf2PasswordHash.Iterations", "1000"); + final TomEEPbkdf2PasswordHash generator = new TomEEPbkdf2PasswordHash(); + generator.initialize(parameters); + + final String stored = generator.generate(PASSWORD); + assertEquals("PBKDF2WithHmacSHA1:1000", stored.substring(0, stored.indexOf(':', stored.indexOf(':') + 1))); + assertTrue(generator.verify(PASSWORD, stored)); + + parameters.clear(); + parameters.put("Pbkdf2PasswordHash.Algorithm", "PBKDF2WithHmacMD5"); + new TomEEPbkdf2PasswordHash().initialize(parameters); + } + + @Test(expected = IllegalArgumentException.class) + public void initializeRejectsNonPositiveIterationsWhenAllowed() { + allowWeakParameters(); + final Map<String, String> parameters = new HashMap<>(); + parameters.put("Pbkdf2PasswordHash.Iterations", "0"); + new TomEEPbkdf2PasswordHash().initialize(parameters); + } + + private static boolean isAvailable(final String algorithm) { + try { + SecretKeyFactory.getInstance(algorithm); + return true; + } catch (final Exception e) { + return false; + } + } + + private static String stored(final String algorithm, final int iterations, final char[] password) throws Exception { + final byte[] salt = new byte[24]; + for (int i = 0; i < salt.length; i++) { + salt[i] = (byte) i; + } + final byte[] derived = SecretKeyFactory.getInstance(algorithm) + .generateSecret(new PBEKeySpec(password, salt, iterations, 32 * 8)) + .getEncoded(); + final Base64.Encoder encoder = Base64.getEncoder(); + return algorithm + ":" + iterations + ":" + encoder.encodeToString(salt) + ":" + encoder.encodeToString(derived); + } +}
