Merge pull request #30 from canonical/concurrency

Make OpenSSLDRBG thread-safe
canonical · Sep 3, 2024 · 2aeadd3 · 2aeadd3
2 parents 50e3ae0 + 7d5888b
commit 2aeadd3
Show file tree

Hide file tree

Showing 8 changed files with 100 additions and 32 deletions.
diff --git 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/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) {

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 {