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

Decouple the registrar name from the security provider name. In the BouncyCastleSecurityRegistrar, check also for BCFIPS if regular BC cannot be found. Also check whether the BC RandomGenerator exists at all; in BCFIPS, it doesn't.
tomaswolf · Sep 4, 2024 · e694310 · e694310
1 parent 890f93a
commit e694310
Show file tree

Hide file tree

Showing 10 changed files with 83 additions and 14 deletions.
diff --git a/...src/main/java/org/apache/sshd/common/util/security/AbstractSecurityProviderRegistrar.java b/...src/main/java/org/apache/sshd/common/util/security/AbstractSecurityProviderRegistrar.java
@@ -100,7 +100,7 @@ protected Provider getOrCreateProvider(String providerClassName) throws Reflecti
                 return provider;
             }
 
-            provider = Security.getProvider(getName());
+            provider = Security.getProvider(getProviderName());
             if (provider == null) {
                 provider = createProviderInstance(providerClassName);
                 created = true;

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/...-common/src/main/java/org/apache/sshd/common/util/security/SecurityProviderRegistrar.java b/...-common/src/main/java/org/apache/sshd/common/util/security/SecurityProviderRegistrar.java
@@ -307,14 +307,14 @@ static String getEffectiveSecurityEntityName(Class<?> entityType, String name) {
      */
     static boolean registerSecurityProvider(SecurityProviderRegistrar registrar) {
         String name = ValidateUtils.checkNotNullAndNotEmpty(
-                (registrar == null) ? null : registrar.getName(), "No name for registrar=%s", registrar);
+                (registrar == null) ? null : registrar.getProviderName(), "No name for registrar=%s", registrar);
         Provider p = Security.getProvider(name);
         if (p != null) {
             return false;
         }
 
         p = ValidateUtils.checkNotNull(
-                registrar.getSecurityProvider(), "No provider created for registrar of %s", name);
+                registrar.getSecurityProvider(), "No provider created for registrar %s of %s", registrar.getName(), name);
         if (registrar.isNamedProviderUsed()) {
             Security.addProvider(p);
         }

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";
 
@@ -552,7 +552,7 @@ public static Decryptor getBouncycastleEncryptedPrivateKeyInfoDecryptor() {
      *         {@link JceRandomFactory} one
      */
     public static RandomFactory getRandomFactory() {
-        if (isBouncyCastleRegistered()) {
+        if (isBouncyCastleRegistered() && BouncyCastleRandomFactory.INSTANCE.isSupported()) {
             return BouncyCastleRandomFactory.INSTANCE;
         } else {
             return JceRandomFactory.INSTANCE;

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,9 +18,11 @@
  */
 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;
+import java.security.Security;
 import java.security.Signature;
 import java.util.Objects;
 import java.util.concurrent.atomic.AtomicReference;
@@ -37,10 +39,18 @@
 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";
+    public static final String FIPS_PROVIDER_CLASS = "org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider";
+    private static final String BCFIPS_PROVIDER_NAME = "BCFIPS";
+    private static final String BC_PROVIDER_NAME = "BC";
+    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 providerClass;
+    private String providerName;
+
     public BouncyCastleSecurityProviderRegistrar() {
         super(SecurityUtils.BOUNCY_CASTLE);
     }
@@ -55,14 +65,19 @@ public boolean isEnabled() {
         return this.getBooleanProperty(SecurityUtils.REGISTER_BOUNCY_CASTLE_PROP, true);
     }
 
+    @Override
+    public String getProviderName() {
+        return providerName;
+    }
+
     @Override
     public Provider getSecurityProvider() {
         try {
-            return getOrCreateProvider(PROVIDER_CLASS);
+            return getOrCreateProvider(providerClass);
         } catch (ReflectiveOperationException t) {
             Throwable e = ExceptionUtils.peelException(t);
             log.error("getSecurityProvider({}) failed ({}) to instantiate {}: {}",
-                    getName(), e.getClass().getSimpleName(), PROVIDER_CLASS, e.getMessage());
+                    getName(), e.getClass().getSimpleName(), providerClass, e.getMessage());
             if (e instanceof RuntimeException) {
                 throw (RuntimeException) e;
             }
@@ -119,7 +134,40 @@ public boolean isSupported() {
             }
 
             Class<?> clazz = ThreadUtils.resolveDefaultClass(getClass(), PROVIDER_CLASS);
-            supported = clazz != null;
+            if (clazz == null) {
+                clazz = ThreadUtils.resolveDefaultClass(getClass(), FIPS_PROVIDER_CLASS);
+            }
+            if (clazz != null) {
+                // Apache MINA sshd assumes that if we can get at the provider class, we can also get any other class we
+                // need. However, and BC-based optional stuff should actually check if it does have the concrete
+                // classes it needs accessible. The FIPS version has only a subset of the full BC.
+                providerClass = clazz.getName();
+                Provider provider = Security.getProvider(BCFIPS_PROVIDER_NAME);
+                if (provider != null) {
+                    providerName = BCFIPS_PROVIDER_NAME;
+                } else {
+                    provider = Security.getProvider(BC_PROVIDER_NAME);
+                    if (provider != null) {
+                        providerName = BC_PROVIDER_NAME;
+                    }
+                }
+                if (providerName == 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.", providerClass, NAME_FIELD,
+                                e);
+                    }
+                }
+                supported = Boolean.valueOf(providerName != null);
+            } else {
+                supported = Boolean.FALSE;
+            }
             supportHolder.set(supported);
         }