apacheGH-590: Be more careful with Bouncy Castle in FIPS environment

Decouple the registrar name from the security provider name. Also check whether the BC RandomGenerator exists at all; in BC-FIPS, it doesn't.
tomaswolf · Aug 31, 2024 · 5ccfe4f · 5ccfe4f
1 parent 40a08a4
commit 5ccfe4f
Show file tree

Hide file tree

Showing 8 changed files with 54 additions and 8 deletions.
diff --git a/sshd-common/src/main/java/org/apache/sshd/common/util/security/SecurityEntityFactory.java b/sshd-common/src/main/java/org/apache/sshd/common/util/security/SecurityEntityFactory.java
@@ -60,12 +60,12 @@ static <F> SecurityEntityFactory<F> toFactory(
             if ((defaultProvider == null) || (defaultProvider == SecurityProviderChoice.EMPTY)) {
                 return toDefaultFactory(entityType);
             } else if (defaultProvider.isNamedProviderUsed()) {
-                return toNamedProviderFactory(entityType, defaultProvider.getName());
+                return toNamedProviderFactory(entityType, defaultProvider.getProviderName());
             } else {
                 return toProviderInstanceFactory(entityType, defaultProvider.getSecurityProvider());
             }
         } else if (registrar.isNamedProviderUsed()) {
-            return toNamedProviderFactory(entityType, registrar.getName());
+            return toNamedProviderFactory(entityType, registrar.getProviderName());
         } else {
             return toProviderInstanceFactory(entityType, registrar.getSecurityProvider());
         }

diff --git a/sshd-common/src/main/java/org/apache/sshd/common/util/security/SecurityProviderChoice.java b/sshd-common/src/main/java/org/apache/sshd/common/util/security/SecurityProviderChoice.java
@@ -60,6 +60,17 @@ default boolean isNamedProviderUsed() {
         return true;
     }
 
+    /**
+     * Retrieves the underlying {@link Provider}'s name.
+     *
+     * @return the {@link Provider}'s name
+     */
+    default String getProviderName() {
+        // By default, assume choice name and provider name are the same. For a SecurityProviderRegistrar, these may be
+        // different!
+        return getName();
+    }
+
     /**
      * @return The security {@link Provider} to use in case {@link #isNamedProviderUsed()} is {@code false}. Can be
      *         {@code null} if {@link #isNamedProviderUsed()} is {@code true}, but not recommended.

diff --git a/sshd-common/src/main/java/org/apache/sshd/common/util/security/SecurityUtils.java b/sshd-common/src/main/java/org/apache/sshd/common/util/security/SecurityUtils.java
@@ -90,7 +90,7 @@
  */
 public final class SecurityUtils {
     /**
-     * Bouncycastle JCE provider name
+     * Bouncy Castle {@link SecurityProviderRegistrar} name.
      */
     public static final String BOUNCY_CASTLE = "BC";
 

diff --git a/...n/src/main/java/org/apache/sshd/common/util/security/SunJCESecurityProviderRegistrar.java b/...n/src/main/java/org/apache/sshd/common/util/security/SunJCESecurityProviderRegistrar.java
@@ -67,6 +67,11 @@ public boolean isEnabled() {
         return isSupported();
     }
 
+    @Override
+    public String getProviderName() {
+        return "SunJCE";
+    }
+
     @Override
     public String getDefaultSecurityEntitySupportValue(Class<?> entityType) {
         return "";
@@ -91,7 +96,7 @@ public boolean isNamedProviderUsed() {
 
     @Override
     public Provider getSecurityProvider() {
-        return Security.getProvider("SunJCE");
+        return Security.getProvider(getProviderName());
     }
 
     @Override

diff --git a/.../sshd/common/util/security/bouncycastle/BouncyCastleEncryptedPrivateKeyInfoDecryptor.java b/.../sshd/common/util/security/bouncycastle/BouncyCastleEncryptedPrivateKeyInfoDecryptor.java
@@ -52,7 +52,7 @@ public byte[] decrypt(byte[] encrypted, char[] password)
         try {
             JcePKCSPBEInputDecryptorProviderBuilder builder = new JcePKCSPBEInputDecryptorProviderBuilder();
             if (registrar.isNamedProviderUsed()) {
-                builder.setProvider(registrar.getName());
+                builder.setProvider(registrar.getProviderName());
             } else {
                 builder.setProvider(registrar.getSecurityProvider());
             }

diff --git a/.../org/apache/sshd/common/util/security/bouncycastle/BouncyCastleKeyPairResourceParser.java b/.../org/apache/sshd/common/util/security/bouncycastle/BouncyCastleKeyPairResourceParser.java
@@ -114,7 +114,7 @@ public static KeyPair loadKeyPair(
 
             JcaPEMKeyConverter pemConverter = new JcaPEMKeyConverter();
             if (registrar.isNamedProviderUsed()) {
-                pemConverter.setProvider(registrar.getName());
+                pemConverter.setProvider(registrar.getProviderName());
             } else {
                 pemConverter.setProvider(registrar.getSecurityProvider());
             }

diff --git a/...ain/java/org/apache/sshd/common/util/security/bouncycastle/BouncyCastleRandomFactory.java b/...ain/java/org/apache/sshd/common/util/security/bouncycastle/BouncyCastleRandomFactory.java
@@ -21,6 +21,7 @@
 import org.apache.sshd.common.random.AbstractRandomFactory;
 import org.apache.sshd.common.random.Random;
 import org.apache.sshd.common.util.security.SecurityUtils;
+import org.bouncycastle.crypto.prng.RandomGenerator;
 
 /**
  * Named factory for the BouncyCastle <code>Random</code>
@@ -35,7 +36,11 @@ public BouncyCastleRandomFactory() {
 
     @Override
     public boolean isSupported() {
-        return SecurityUtils.isBouncyCastleRegistered();
+        try {
+            return SecurityUtils.isBouncyCastleRegistered() && RandomGenerator.class.getName() != null;
+        } catch (Throwable e) {
+            return false;
+        }
     }
 
     @Override

diff --git a/.../apache/sshd/common/util/security/bouncycastle/BouncyCastleSecurityProviderRegistrar.java b/.../apache/sshd/common/util/security/bouncycastle/BouncyCastleSecurityProviderRegistrar.java
@@ -18,6 +18,7 @@
  */
 package org.apache.sshd.common.util.security.bouncycastle;
 
+import java.lang.reflect.Field;
 import java.security.KeyFactory;
 import java.security.KeyPairGenerator;
 import java.security.Provider;
@@ -37,10 +38,14 @@
 public class BouncyCastleSecurityProviderRegistrar extends AbstractSecurityProviderRegistrar {
     // We want to use reflection API so as not to require BouncyCastle to be present in the classpath
     public static final String PROVIDER_CLASS = "org.bouncycastle.jce.provider.BouncyCastleProvider";
+    private static final String NAME_FIELD = "PROVIDER_NAME";
+
     // Do not define a static registrar instance to minimize class loading issues
     private final AtomicReference<Boolean> supportHolder = new AtomicReference<>(null);
     private final AtomicReference<String> allSupportHolder = new AtomicReference<>();
 
+    private String providerName;
+
     public BouncyCastleSecurityProviderRegistrar() {
         super(SecurityUtils.BOUNCY_CASTLE);
     }
@@ -55,6 +60,11 @@ public boolean isEnabled() {
         return this.getBooleanProperty(SecurityUtils.REGISTER_BOUNCY_CASTLE_PROP, true);
     }
 
+    @Override
+    public String getProviderName() {
+        return providerName;
+    }
+
     @Override
     public Provider getSecurityProvider() {
         try {
@@ -119,7 +129,22 @@ public boolean isSupported() {
             }
 
             Class<?> clazz = ThreadUtils.resolveDefaultClass(getClass(), PROVIDER_CLASS);
-            supported = clazz != null;
+            if (clazz != null) {
+                Field f;
+                try {
+                    f = clazz.getField(NAME_FIELD);
+                    Object nameValue = f.get(null);
+                    if (nameValue instanceof String) {
+                        providerName = nameValue.toString();
+                    }
+                } catch (Exception e) {
+                    log.warn("Alleged Bouncy Castle class {} has no {}; ignoring this provider.", PROVIDER_CLASS, NAME_FIELD,
+                            e);
+                }
+                supported = Boolean.valueOf(providerName != null);
+            } else {
+                supported = Boolean.FALSE;
+            }
             supportHolder.set(supported);
         }