From ee026fe6cb873b2cf4c15db6f163b4921de3096d Mon Sep 17 00:00:00 2001 From: Pushkar Kulkarni Date: Tue, 3 Sep 2024 11:41:06 +0530 Subject: [PATCH 1/2] Make OpenSSLDRBG thread-safe --- .../canonical/openssl/drbg/OpenSSLDrbg.java | 92 ++++++++++++------- 1 file changed, 60 insertions(+), 32 deletions(-) diff --git a/src/main/java/com/canonical/openssl/drbg/OpenSSLDrbg.java b/src/main/java/com/canonical/openssl/drbg/OpenSSLDrbg.java index 6127f75..22902b9 100644 --- a/src/main/java/com/canonical/openssl/drbg/OpenSSLDrbg.java +++ b/src/main/java/com/canonical/openssl/drbg/OpenSSLDrbg.java @@ -24,6 +24,11 @@ import java.security.DrbgParameters; import java.security.Provider; +/* This implementation will be exercised by the user through the + * java.security.SecureRandom API which is marked thread-safe. + * This implementation is also thread-safe. + */ + public class OpenSSLDrbg extends SecureRandomSpi { public static int DEFAULT_STRENGTH = 128; @@ -34,10 +39,10 @@ public class OpenSSLDrbg extends SecureRandomSpi { long drbgContext; SecureRandomParameters params; - private static class DRBGState implements Runnable { + private static class NativeDRBG implements Runnable { private long nativeHandle; - DRBGState(long handle) { + NativeDRBG(long handle) { this.nativeHandle = handle; } @@ -50,11 +55,20 @@ public void run() { private Cleaner cleaner = NativeMemoryCleaner.cleaner; private Cleaner.Cleanable cleanable; + // mutex lock for nextBytes(), borrowed from openjdk's NativePRNG + private final Object LOCK_GET_BYTES = new Object(); + + // mutex lock for generateSeed(), borrowed from openjdk's NativePRNG + private final Object LOCK_GET_SEED = new Object(); + + // mutex lock for setSeed(), borrowed from openjdk's NativePRNG + private final Object LOCK_SET_SEED = new Object(); + private OpenSSLDrbg() { } protected OpenSSLDrbg(String name) { drbgContext = init(name, DEFAULT_STRENGTH, false, false, null); - cleanable = cleaner.register(this, new DRBGState(drbgContext)); + cleanable = cleaner.register(this, new NativeDRBG(drbgContext)); } protected OpenSSLDrbg(String name, SecureRandomParameters params) throws IllegalArgumentException { @@ -70,7 +84,7 @@ protected OpenSSLDrbg(String name, SecureRandomParameters params) throws Illegal } else { this.drbgContext = init(name, DEFAULT_STRENGTH, false, false, null); } - cleanable = cleaner.register(this, new DRBGState(drbgContext)); + cleanable = cleaner.register(this, new NativeDRBG(drbgContext)); } boolean isInitialized() { @@ -84,58 +98,72 @@ protected SecureRandomParameters engineGetParameters() { @Override protected byte[] engineGenerateSeed(int numBytes) { - return generateSeed0(numBytes); + synchronized (LOCK_GET_SEED) { + return generateSeed0(numBytes); + } } @Override protected void engineNextBytes(byte[] bytes) { - nextBytes0(bytes, DEFAULT_STRENGTH, false, null); + synchronized (LOCK_GET_BYTES) { + nextBytes0(bytes, DEFAULT_STRENGTH, false, null); + } } protected void engineNextBytes(byte[] bytes, SecureRandomParameters params) throws IllegalArgumentException { - if (params == null) { - engineNextBytes(bytes); - return; + synchronized (LOCK_GET_BYTES) { + if (params == null) { + engineNextBytes(bytes); + return; + } + + if (!(params instanceof DrbgParameters.NextBytes)) { + throw new IllegalArgumentException("Parameters of type DrbgParameters.NextByte expected, passed " + params.getClass()); + } + + DrbgParameters.NextBytes nb = (DrbgParameters.NextBytes)params; + nextBytes0(bytes, nb.getStrength(), nb.getPredictionResistance(), nb.getAdditionalInput()); } - - if (!(params instanceof DrbgParameters.NextBytes)) { - throw new IllegalArgumentException("Parameters of type DrbgParameters.NextByte expected, passed " + params.getClass()); - } - - DrbgParameters.NextBytes nb = (DrbgParameters.NextBytes)params; - nextBytes0(bytes, nb.getStrength(), nb.getPredictionResistance(), nb.getAdditionalInput()); } protected void engineReseed() { - reseed0(null, false, null); + synchronized (LOCK_SET_SEED) { + reseed0(null, false, null); + } } @Override protected void engineReseed(SecureRandomParameters params) throws IllegalArgumentException { - if (params == null) { - engineReseed(); - return; - } + synchronized (LOCK_SET_SEED) { + if (params == null) { + engineReseed(); + return; + } - if (!(params instanceof DrbgParameters.Reseed)) { - throw new IllegalArgumentException("Parameters of type DrbgParameters.Reseed expected, passed " + params.getClass()); + if (!(params instanceof DrbgParameters.Reseed)) { + throw new IllegalArgumentException("Parameters of type DrbgParameters.Reseed expected, passed " + params.getClass()); + } + DrbgParameters.Reseed rs = (DrbgParameters.Reseed)params; + reseed0(null, rs.getPredictionResistance(), rs.getAdditionalInput()); } - DrbgParameters.Reseed rs = (DrbgParameters.Reseed)params; - reseed0(null, rs.getPredictionResistance(), rs.getAdditionalInput()); } protected void engineSetSeed(byte[] seed) { - reseed0(seed, false, null); + synchronized (LOCK_SET_SEED) { + reseed0(seed, false, null); + } } protected void engineSetSeed(long seed) { - byte [] seedBytes = new byte[8]; - for (int i = 0; i < 8; i++) { - seedBytes[i] = (byte)(seed & (long)0xff); - seed = seed >> 8; + synchronized (LOCK_SET_SEED) { + byte [] seedBytes = new byte[8]; + for (int i = 0; i < 8; i++) { + seedBytes[i] = (byte)(seed & (long)0xff); + seed = seed >> 8; + } + + engineSetSeed(seedBytes); } - - engineSetSeed(seedBytes); } private static void cleanupNativeMemory(long handle) { From 7d5888b34058c7f1b76e51ac0e3050f77dd040ba Mon Sep 17 00:00:00 2001 From: Pushkar Kulkarni Date: Tue, 3 Sep 2024 11:41:42 +0530 Subject: [PATCH 2/2] Document lack of thread-safety --- .../java/com/canonical/openssl/cipher/OpenSSLCipher.java | 6 ++++++ src/main/java/com/canonical/openssl/kdf/OpenSSLPBKDF2.java | 6 ++++++ .../canonical/openssl/keyagreement/OpenSSLKeyAgreement.java | 6 ++++++ .../canonical/openssl/keyencapsulation/OpenSSLKEMRSA.java | 6 ++++++ src/main/java/com/canonical/openssl/mac/OpenSSLMAC.java | 6 ++++++ src/main/java/com/canonical/openssl/md/OpenSSLMD.java | 5 +++++ .../com/canonical/openssl/signature/OpenSSLSignature.java | 5 +++++ 7 files changed, 40 insertions(+) diff --git a/src/main/java/com/canonical/openssl/cipher/OpenSSLCipher.java b/src/main/java/com/canonical/openssl/cipher/OpenSSLCipher.java index bd68996..e151241 100644 --- a/src/main/java/com/canonical/openssl/cipher/OpenSSLCipher.java +++ b/src/main/java/com/canonical/openssl/cipher/OpenSSLCipher.java @@ -30,6 +30,12 @@ import javax.crypto.IllegalBlockSizeException; import javax.crypto.BadPaddingException; +/* This implementation will be exercised by the user through the + * javax.crypto.Cipher API which isn't marked thread-safe. + * This implementation is also NOT thread-safe and applications need + * handle thread-safety concerns if need be. + */ + abstract public class OpenSSLCipher extends CipherSpi { static { diff --git a/src/main/java/com/canonical/openssl/kdf/OpenSSLPBKDF2.java b/src/main/java/com/canonical/openssl/kdf/OpenSSLPBKDF2.java index 4362328..8bf4f12 100644 --- a/src/main/java/com/canonical/openssl/kdf/OpenSSLPBKDF2.java +++ b/src/main/java/com/canonical/openssl/kdf/OpenSSLPBKDF2.java @@ -41,6 +41,12 @@ * can be represented by class PBEKeySpec. As a result, only PBKDF2 * is implemented in this prototype. */ + +/* This implementation will be exercised by the user through the + * javax.crypto.SecretKeyFactory API which isn't marked thread-safe. + * This implementation is also NOT thread-safe and applications need + * handle any concerns regarding the same. + */ public class OpenSSLPBKDF2 extends SecretKeyFactorySpi { static { diff --git a/src/main/java/com/canonical/openssl/keyagreement/OpenSSLKeyAgreement.java b/src/main/java/com/canonical/openssl/keyagreement/OpenSSLKeyAgreement.java index 425ac96..c26be29 100644 --- a/src/main/java/com/canonical/openssl/keyagreement/OpenSSLKeyAgreement.java +++ b/src/main/java/com/canonical/openssl/keyagreement/OpenSSLKeyAgreement.java @@ -26,6 +26,12 @@ import javax.crypto.SecretKey; import java.util.Base64; +/* This implementation will be exercised by the user through the + * javax.crypto.KeyAgreement API which isn't marked thread-safe. + * This implementation is also NOT thread-safe and applications need + * handle thread-safety concerns if need be. + */ + abstract public class OpenSSLKeyAgreement extends KeyAgreementSpi { static { NativeLibraryLoader.load(); diff --git a/src/main/java/com/canonical/openssl/keyencapsulation/OpenSSLKEMRSA.java b/src/main/java/com/canonical/openssl/keyencapsulation/OpenSSLKEMRSA.java index b621bc3..2b0d49d 100644 --- a/src/main/java/com/canonical/openssl/keyencapsulation/OpenSSLKEMRSA.java +++ b/src/main/java/com/canonical/openssl/keyencapsulation/OpenSSLKEMRSA.java @@ -36,6 +36,12 @@ import java.security.interfaces.RSAPublicKey; import javax.crypto.spec.SecretKeySpec; +/* This implementation will be exercised by the user through the + * javax.crypto.KEM API which isn't marked thread-safe. + * This implementation is also NOT thread-safe and applications need + * handle thread-safety concerns if need be. + */ + final public class OpenSSLKEMRSA implements KEMSpi { static { diff --git a/src/main/java/com/canonical/openssl/mac/OpenSSLMAC.java b/src/main/java/com/canonical/openssl/mac/OpenSSLMAC.java index c1e7a17..570a08c 100644 --- a/src/main/java/com/canonical/openssl/mac/OpenSSLMAC.java +++ b/src/main/java/com/canonical/openssl/mac/OpenSSLMAC.java @@ -26,6 +26,12 @@ import java.security.spec.AlgorithmParameterSpec; import javax.xml.crypto.dsig.spec.HMACParameterSpec; +/* This implementation will be exercised by the user through the + * javax.crypto.Mac API which isn't marked thread-safe. + * This implementation is also NOT thread-safe and applications need + * handle thread-safety concerns if need be. + */ + public abstract class OpenSSLMAC extends MacSpi { static { diff --git a/src/main/java/com/canonical/openssl/md/OpenSSLMD.java b/src/main/java/com/canonical/openssl/md/OpenSSLMD.java index e63b8f2..03617de 100644 --- a/src/main/java/com/canonical/openssl/md/OpenSSLMD.java +++ b/src/main/java/com/canonical/openssl/md/OpenSSLMD.java @@ -27,6 +27,11 @@ import java.util.Arrays; import java.util.Set; +/* This implementation will be exercised by the user through the + * java.security.MessageDigest API which isn't marked thread-safe. + * This implementation is also NOT thread-safe and applications need + * handle thread-safety concerns if need be. + */ public abstract class OpenSSLMD extends MessageDigestSpi { static { diff --git a/src/main/java/com/canonical/openssl/signature/OpenSSLSignature.java b/src/main/java/com/canonical/openssl/signature/OpenSSLSignature.java index 5d8c707..17528b5 100644 --- a/src/main/java/com/canonical/openssl/signature/OpenSSLSignature.java +++ b/src/main/java/com/canonical/openssl/signature/OpenSSLSignature.java @@ -31,6 +31,11 @@ import java.security.SignatureException; import java.security.SignatureSpi; +/* This implementation will be exercised by the user through the + * java.security.Signature API which isn't marked thread-safe. + * This implementation is also NOT thread-safe and applications need + * handle thread-safety concerns if need be. + */ public abstract class OpenSSLSignature extends SignatureSpi { static {