From c1339977e896fa2e2a8c219a65f44f519d5f83cd Mon Sep 17 00:00:00 2001 From: Chris Conlon Date: Mon, 27 Jul 2026 13:06:22 -0600 Subject: [PATCH] F-5890: avoid cross-key lock nesting in WolfCryptPBEKey.equals() --- .../wolfssl/provider/jce/WolfCryptPBEKey.java | 66 +++++-- .../test/WolfCryptSecretKeyFactoryTest.java | 165 ++++++++++++++++++ 2 files changed, 216 insertions(+), 15 deletions(-) diff --git a/src/main/java/com/wolfssl/provider/jce/WolfCryptPBEKey.java b/src/main/java/com/wolfssl/provider/jce/WolfCryptPBEKey.java index 71db426d..f8d60954 100644 --- a/src/main/java/com/wolfssl/provider/jce/WolfCryptPBEKey.java +++ b/src/main/java/com/wolfssl/provider/jce/WolfCryptPBEKey.java @@ -24,6 +24,8 @@ package com.wolfssl.provider.jce; import java.io.IOException; import java.io.ObjectInputStream; import java.util.Arrays; +import java.util.Objects; +import java.security.MessageDigest; import java.security.spec.InvalidKeySpecException; import javax.crypto.interfaces.PBEKey; @@ -242,35 +244,52 @@ public class WolfCryptPBEKey implements PBEKey { } } + /* equals() and hashCode() do not throw after destroy(), but the hash + * changes when key bytes are cleared, remove from maps before destroy */ @Override public synchronized int hashCode() { return Arrays.hashCode(encoded); } @Override - public synchronized boolean equals(Object obj) { + public boolean equals(Object obj) { - PBEKey pKey = null; + byte[] pKeyEncoded = null; + byte[] thisEncoded = null; + byte[] pKeySalt = null; + byte[] thisSalt = null; + char[] pKeyPass = null; + char[] thisPass = null; + PBEKey pKey; - synchronized (destroyedLock) { - if (obj == this) { - return true; - } + if (obj == this) { + return true; + } - if (!(obj instanceof PBEKey)) { - return false; - } - pKey = (PBEKey)obj; + if (!(obj instanceof PBEKey)) { + return false; + } + pKey = (PBEKey)obj; - if (!Arrays.equals(pKey.getEncoded(), getEncoded())) { + try { + pKeyEncoded = pKey.getEncoded(); + pKeySalt = pKey.getSalt(); + pKeyPass = pKey.getPassword(); + + thisEncoded = getEncoded(); + thisSalt = getSalt(); + thisPass = getPassword(); + + /* MessageDigest.isEqual() for constant-time comparison */ + if (!MessageDigest.isEqual(pKeyEncoded, thisEncoded)) { return false; } - if (!Arrays.equals(pKey.getSalt(), getSalt())) { + if (!Arrays.equals(pKeySalt, thisSalt)) { return false; } - if (!Arrays.equals(pKey.getPassword(), getPassword())) { + if (!Arrays.equals(pKeyPass, thisPass)) { return false; } @@ -278,15 +297,32 @@ public class WolfCryptPBEKey implements PBEKey { return false; } - if (!pKey.getAlgorithm().equals(getAlgorithm())) { + if (!Objects.equals(pKey.getAlgorithm(), getAlgorithm())) { return false; } - if (!pKey.getFormat().equals(getFormat())) { + if (!Objects.equals(pKey.getFormat(), getFormat())) { return false; } return true; + + } catch (Exception e) { + /* Destroyed keys cannot be compared, treat as not equal */ + return false; + + } finally { + /* Only our own copies, the other key's accessors may return + * internal references and zeroizing those would destroy it */ + if (thisEncoded != null) { + Arrays.fill(thisEncoded, (byte)0); + } + if (thisSalt != null) { + Arrays.fill(thisSalt, (byte)0); + } + if (thisPass != null) { + Arrays.fill(thisPass, (char)0); + } } } diff --git a/src/test/java/com/wolfssl/provider/jce/test/WolfCryptSecretKeyFactoryTest.java b/src/test/java/com/wolfssl/provider/jce/test/WolfCryptSecretKeyFactoryTest.java index 15a93072..e6a143ec 100644 --- a/src/test/java/com/wolfssl/provider/jce/test/WolfCryptSecretKeyFactoryTest.java +++ b/src/test/java/com/wolfssl/provider/jce/test/WolfCryptSecretKeyFactoryTest.java @@ -50,6 +50,7 @@ import javax.crypto.Cipher; import javax.crypto.spec.PBEKeySpec; import javax.crypto.spec.SecretKeySpec; import javax.crypto.spec.IvParameterSpec; +import javax.security.auth.DestroyFailedException; import com.wolfssl.wolfcrypt.Fips; import com.wolfssl.wolfcrypt.Aes; @@ -1149,6 +1150,170 @@ public class WolfCryptSecretKeyFactoryTest { } } + /** + * Test PBEKey equals() and hashCode() behavior, including that + * destroyed keys compare as not equal instead of throwing. + */ + @Test + public void testPBKDF2WithHmacSHA256_KeyEquals() + throws NoSuchAlgorithmException, InvalidKeySpecException, + NoSuchProviderException, DestroyFailedException { + + char[] pass = "passwordpassword".toCharArray(); + byte[] saltA = { + (byte)0x78, (byte)0x57, (byte)0x8E, (byte)0x5a, + (byte)0x5d, (byte)0x63, (byte)0xcb, (byte)0x06 + }; + byte[] saltB = { + (byte)0x78, (byte)0x57, (byte)0x8E, (byte)0x5a, + (byte)0x5d, (byte)0x63, (byte)0xcb, (byte)0x07 + }; + int iterations = 2048; + int kLen = 192; + + if (!FeatureDetect.Pbkdf2Enabled() || + !FeatureDetect.HmacSha256Enabled() || + !algoSupported("PBKDF2WithHmacSHA256")) { + System.out.println( + "Skipped: SecretKeyFactory PBEKey equals test"); + Assume.assumeTrue(false); + } + + SecretKeyFactory sf = + SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256", provider); + + SecretKey keyA = sf.generateSecret( + new PBEKeySpec(pass, saltA, iterations, kLen)); + SecretKey keyB = sf.generateSecret( + new PBEKeySpec(pass, saltA, iterations, kLen)); + SecretKey keyC = sf.generateSecret( + new PBEKeySpec(pass, saltB, iterations, kLen)); + + assertTrue(keyA.equals(keyA)); + assertTrue(keyA.equals(keyB)); + assertTrue(keyB.equals(keyA)); + assertEquals(keyA.hashCode(), keyB.hashCode()); + + assertFalse(keyA.equals(keyC)); + assertFalse(keyC.equals(keyA)); + assertFalse(keyA.equals(null)); + assertFalse(keyA.equals("not a key")); + + keyB.destroy(); + assertFalse(keyA.equals(keyB)); + assertFalse(keyB.equals(keyA)); + } + + /** + * Test that PBEKey equals() does not deadlock when two threads + * compare the same pair of keys in opposite order. + */ + @Test + public void testPBKDF2WithHmacSHA256_ThreadedKeyEquals() + throws NoSuchAlgorithmException, InvalidKeySpecException, + NoSuchProviderException, InterruptedException { + + char[] pass = "passwordpassword".toCharArray(); + byte[] salt = { + (byte)0x78, (byte)0x57, (byte)0x8E, (byte)0x5a, + (byte)0x5d, (byte)0x63, (byte)0xcb, (byte)0x06 + }; + int iterations = 2048; + int kLen = 192; + + if (!FeatureDetect.Pbkdf2Enabled() || + !FeatureDetect.HmacSha256Enabled() || + !algoSupported("PBKDF2WithHmacSHA256")) { + System.out.println( + "Skipped: SecretKeyFactory PBEKey equals threaded test"); + Assume.assumeTrue(false); + } + + SecretKeyFactory sf = + SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256", provider); + + final SecretKey keyA = sf.generateSecret( + new PBEKeySpec(pass, salt, iterations, kLen)); + final SecretKey keyB = sf.generateSecret( + new PBEKeySpec(pass, salt, iterations, kLen)); + + assertNoDeadlockOnCrossThreadEquals(keyA, keyB, "PBEKey"); + } + + /** + * Compare two equal keys from two threads in opposite order, so both + * are inside equals() on both keys at once and the opposite lock + * ordering can deadlock. + */ + private void assertNoDeadlockOnCrossThreadEquals(final SecretKey keyA, + final SecretKey keyB, final String label) + throws InterruptedException { + + final LinkedBlockingQueue results = + new LinkedBlockingQueue<>(); + final CountDownLatch startGate = new CountDownLatch(1); + + Thread threadA = new Thread(new Runnable() { + @Override public void run() { + results.add(compareRepeatedly(startGate, keyA, keyB)); + } + }); + Thread threadB = new Thread(new Runnable() { + @Override public void run() { + results.add(compareRepeatedly(startGate, keyB, keyA)); + } + }); + + /* Daemon threads so the JVM can exit if equals() deadlocks */ + threadA.setDaemon(true); + threadB.setDaemon(true); + threadA.start(); + threadB.start(); + startGate.countDown(); + threadA.join(30000); + threadB.join(30000); + + if (threadA.isAlive() || threadB.isAlive()) { + fail("Deadlock in " + label + + ".equals() cross-thread comparison"); + } + + /* Both threads must have reported, a thread that died on an unchecked + * throwable never reaches results.add() */ + assertEquals("Both threads should report a result for " + label, + 2, results.size()); + + Iterator listIterator = results.iterator(); + while (listIterator.hasNext()) { + Integer cur = listIterator.next(); + if (cur == 1) { + fail(label + ".equals() returned false for equal keys"); + } + } + } + + /** + * Wait on the start gate then compare repeatedly, returning 0 if every + * comparison matched and 1 otherwise. + */ + private static int compareRepeatedly(CountDownLatch startGate, + SecretKey first, SecretKey second) { + + try { + startGate.await(); + } catch (InterruptedException e) { + return 1; + } + + for (int i = 0; i < 10000; i++) { + if (!first.equals(second)) { + return 1; + } + } + + return 0; + } + @Test public void testGetAESSecretKeyFactoryFromProvider() throws NoSuchProviderException, NoSuchAlgorithmException {