From 30691384824b3f2ac1fa7e41982ae47ee1a4bdc5 Mon Sep 17 00:00:00 2001 From: exceptionfactory Date: Thu, 20 Jul 2023 01:43:25 -0500 Subject: [PATCH] Add DefaultSecurityProviderConfig with Bouncy Castle disabled (#861) * Added DefaultSecurityProviderConfig with Bouncy Castle disabled * Upgrade test to junit jupiter --------- Co-authored-by: Jeroen van Erp --- .../java/net/schmizz/sshj/AndroidConfig.java | 9 -- .../java/net/schmizz/sshj/DefaultConfig.java | 114 ++++++++---------- .../sshj/DefaultSecurityProviderConfig.java | 28 +++++ .../sshj/common/ECDSAVariationsAdapter.java | 3 - .../schmizz/sshj/common/SecurityUtils.java | 5 + .../DefaultSecurityProviderConfigTest.java | 65 ++++++++++ .../sshj/keyprovider/OpenSSHKeyFileTest.java | 8 -- 7 files changed, 150 insertions(+), 82 deletions(-) create mode 100644 src/main/java/net/schmizz/sshj/DefaultSecurityProviderConfig.java create mode 100644 src/test/java/net/schmizz/sshj/DefaultSecurityProviderConfigTest.java diff --git a/src/main/java/net/schmizz/sshj/AndroidConfig.java b/src/main/java/net/schmizz/sshj/AndroidConfig.java index 9d0c4b61..e6e9c1b9 100644 --- a/src/main/java/net/schmizz/sshj/AndroidConfig.java +++ b/src/main/java/net/schmizz/sshj/AndroidConfig.java @@ -19,8 +19,6 @@ import com.hierynomus.sshj.key.KeyAlgorithm; import com.hierynomus.sshj.key.KeyAlgorithms; import net.schmizz.sshj.common.Factory; import net.schmizz.sshj.common.SecurityUtils; -import net.schmizz.sshj.transport.random.JCERandom; -import net.schmizz.sshj.transport.random.SingletonRandomFactory; import java.util.Arrays; @@ -34,7 +32,6 @@ public class AndroidConfig SecurityUtils.registerSecurityProvider("org.spongycastle.jce.provider.BouncyCastleProvider"); } - @Override protected void initKeyAlgorithms() { setKeyAlgorithms(Arrays.>asList( @@ -43,10 +40,4 @@ public class AndroidConfig KeyAlgorithms.SSHDSA() )); } - - @Override - protected void initRandomFactory(boolean ignored) { - setRandomFactory(new SingletonRandomFactory(new JCERandom.Factory())); - } - } diff --git a/src/main/java/net/schmizz/sshj/DefaultConfig.java b/src/main/java/net/schmizz/sshj/DefaultConfig.java index e6086c12..4de9958a 100644 --- a/src/main/java/net/schmizz/sshj/DefaultConfig.java +++ b/src/main/java/net/schmizz/sshj/DefaultConfig.java @@ -29,7 +29,6 @@ import com.hierynomus.sshj.userauth.keyprovider.OpenSSHKeyV1KeyFile; import net.schmizz.keepalive.KeepAliveProvider; import net.schmizz.sshj.common.Factory; import net.schmizz.sshj.common.LoggerFactory; -import net.schmizz.sshj.common.SecurityUtils; import net.schmizz.sshj.transport.cipher.Cipher; import net.schmizz.sshj.transport.compression.NoneCompression; import net.schmizz.sshj.transport.kex.Curve25519SHA256; @@ -43,7 +42,11 @@ import net.schmizz.sshj.userauth.keyprovider.PKCS8KeyFile; import net.schmizz.sshj.userauth.keyprovider.PuTTYKeyFile; import org.slf4j.Logger; -import java.util.*; +import java.util.Arrays; +import java.util.List; +import java.util.LinkedList; +import java.util.ListIterator; +import java.util.Properties; /** * A {@link net.schmizz.sshj.Config} that is initialized as follows. Items marked with an asterisk are added to the config only if @@ -62,8 +65,6 @@ import java.util.*; *
  • {@link net.schmizz.sshj.ConfigImpl#setVersion Client version}: {@code "NET_3_0"}
    • * *

    - * [1] It is worth noting that Sun's JRE does not have the unlimited cryptography extension enabled by default. This - * prevents using ciphers with strength greater than 128. */ public class DefaultConfig extends ConfigImpl { @@ -73,11 +74,10 @@ public class DefaultConfig public DefaultConfig() { setLoggerFactory(LoggerFactory.DEFAULT); setVersion(readVersionFromProperties()); - final boolean bouncyCastleRegistered = SecurityUtils.isBouncyCastleRegistered(); - initKeyExchangeFactories(bouncyCastleRegistered); + initKeyExchangeFactories(); initKeyAlgorithms(); - initRandomFactory(bouncyCastleRegistered); - initFileKeyProviderFactories(bouncyCastleRegistered); + initRandomFactory(); + initFileKeyProviderFactories(); initCipherFactories(); initCompressionFactories(); initMACFactories(); @@ -102,35 +102,32 @@ public class DefaultConfig log = loggerFactory.getLogger(getClass()); } - protected void initKeyExchangeFactories(boolean bouncyCastleRegistered) { - if (bouncyCastleRegistered) { - setKeyExchangeFactories( - new Curve25519SHA256.Factory(), - new Curve25519SHA256.FactoryLibSsh(), - new DHGexSHA256.Factory(), - new ECDHNistP.Factory521(), - new ECDHNistP.Factory384(), - new ECDHNistP.Factory256(), - new DHGexSHA1.Factory(), - DHGroups.Group1SHA1(), - DHGroups.Group14SHA1(), - DHGroups.Group14SHA256(), - DHGroups.Group15SHA512(), - DHGroups.Group16SHA512(), - DHGroups.Group17SHA512(), - DHGroups.Group18SHA512(), - ExtendedDHGroups.Group14SHA256AtSSH(), - ExtendedDHGroups.Group15SHA256(), - ExtendedDHGroups.Group15SHA256AtSSH(), - ExtendedDHGroups.Group15SHA384AtSSH(), - ExtendedDHGroups.Group16SHA256(), - ExtendedDHGroups.Group16SHA384AtSSH(), - ExtendedDHGroups.Group16SHA512AtSSH(), - ExtendedDHGroups.Group18SHA512AtSSH(), - new ExtInfoClientFactory()); - } else { - setKeyExchangeFactories(DHGroups.Group1SHA1(), new DHGexSHA1.Factory()); - } + protected void initKeyExchangeFactories() { + setKeyExchangeFactories( + new Curve25519SHA256.Factory(), + new Curve25519SHA256.FactoryLibSsh(), + new DHGexSHA256.Factory(), + new ECDHNistP.Factory521(), + new ECDHNistP.Factory384(), + new ECDHNistP.Factory256(), + new DHGexSHA1.Factory(), + DHGroups.Group1SHA1(), + DHGroups.Group14SHA1(), + DHGroups.Group14SHA256(), + DHGroups.Group15SHA512(), + DHGroups.Group16SHA512(), + DHGroups.Group17SHA512(), + DHGroups.Group18SHA512(), + ExtendedDHGroups.Group14SHA256AtSSH(), + ExtendedDHGroups.Group15SHA256(), + ExtendedDHGroups.Group15SHA256AtSSH(), + ExtendedDHGroups.Group15SHA384AtSSH(), + ExtendedDHGroups.Group16SHA256(), + ExtendedDHGroups.Group16SHA384AtSSH(), + ExtendedDHGroups.Group16SHA512AtSSH(), + ExtendedDHGroups.Group18SHA512AtSSH(), + new ExtInfoClientFactory() + ); } protected void initKeyAlgorithms() { @@ -151,21 +148,19 @@ public class DefaultConfig KeyAlgorithms.SSHDSA())); } - protected void initRandomFactory(boolean bouncyCastleRegistered) { + protected void initRandomFactory() { setRandomFactory(new SingletonRandomFactory(new JCERandom.Factory())); } - protected void initFileKeyProviderFactories(boolean bouncyCastleRegistered) { - if (bouncyCastleRegistered) { - setFileKeyProviderFactories( - new OpenSSHKeyV1KeyFile.Factory(), - new PKCS8KeyFile.Factory(), - new OpenSSHKeyFile.Factory(), - new PuTTYKeyFile.Factory()); - } + protected void initFileKeyProviderFactories() { + setFileKeyProviderFactories( + new OpenSSHKeyV1KeyFile.Factory(), + new PKCS8KeyFile.Factory(), + new OpenSSHKeyFile.Factory(), + new PuTTYKeyFile.Factory() + ); } - protected void initCipherFactories() { List> avail = new LinkedList>(Arrays.>asList( ChachaPolyCiphers.CHACHA_POLY_OPENSSH(), @@ -203,27 +198,22 @@ public class DefaultConfig StreamCiphers.Arcfour256()) ); - boolean warn = false; - // Ref. https://issues.apache.org/jira/browse/SSHD-24 - // "AES256 and AES192 requires unlimited cryptography extension" - for (Iterator> i = avail.iterator(); i.hasNext(); ) { - final Factory.Named f = i.next(); + final ListIterator> factories = avail.listIterator(); + while (factories.hasNext()) { + final Factory.Named factory = factories.next(); try { - final Cipher c = f.create(); - final byte[] key = new byte[c.getBlockSize()]; - final byte[] iv = new byte[c.getIVSize()]; - c.init(Cipher.Mode.Encrypt, key, iv); + final Cipher cipher = factory.create(); + final byte[] key = new byte[cipher.getBlockSize()]; + final byte[] iv = new byte[cipher.getIVSize()]; + cipher.init(Cipher.Mode.Encrypt, key, iv); } catch (Exception e) { - warn = true; - log.warn(e.getCause().getMessage()); - i.remove(); + log.info("Cipher [{}] disabled: {}", factory.getName(), e.getCause().getMessage()); + factories.remove(); } } - if (warn) - log.warn("Disabling high-strength ciphers: cipher strengths apparently limited by JCE policy"); setCipherFactories(avail); - log.debug("Available cipher factories: {}", avail); + log.debug("Available Ciphers {}", avail); } protected void initMACFactories() { diff --git a/src/main/java/net/schmizz/sshj/DefaultSecurityProviderConfig.java b/src/main/java/net/schmizz/sshj/DefaultSecurityProviderConfig.java new file mode 100644 index 00000000..98e799a0 --- /dev/null +++ b/src/main/java/net/schmizz/sshj/DefaultSecurityProviderConfig.java @@ -0,0 +1,28 @@ +/* + * Copyright (C)2009 - SSHJ Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package net.schmizz.sshj; + +import net.schmizz.sshj.common.SecurityUtils; + +/** + * SSHJ Configuration that uses the default Security Provider configuration from java.security and disables Bouncy Castle registration + */ +public class DefaultSecurityProviderConfig extends DefaultConfig { + static { + // Disable Bouncy Castle Provider registration prior to invoking constructors + SecurityUtils.setRegisterBouncyCastle(false); + } +} diff --git a/src/main/java/net/schmizz/sshj/common/ECDSAVariationsAdapter.java b/src/main/java/net/schmizz/sshj/common/ECDSAVariationsAdapter.java index fd73f38f..6636ea93 100644 --- a/src/main/java/net/schmizz/sshj/common/ECDSAVariationsAdapter.java +++ b/src/main/java/net/schmizz/sshj/common/ECDSAVariationsAdapter.java @@ -57,9 +57,6 @@ class ECDSAVariationsAdapter { static PublicKey readPubKeyFromBuffer(Buffer buf, String variation) throws GeneralSecurityException { String algorithm = BASE_ALGORITHM_NAME + variation; - if (!SecurityUtils.isBouncyCastleRegistered()) { - throw new GeneralSecurityException("BouncyCastle is required to read a key of type " + algorithm); - } try { // final String algo = buf.readString(); it has been already read final String curveName = buf.readString(); diff --git a/src/main/java/net/schmizz/sshj/common/SecurityUtils.java b/src/main/java/net/schmizz/sshj/common/SecurityUtils.java index 5d6ac05b..2492b256 100644 --- a/src/main/java/net/schmizz/sshj/common/SecurityUtils.java +++ b/src/main/java/net/schmizz/sshj/common/SecurityUtils.java @@ -259,6 +259,11 @@ public class SecurityUtils { return false; } + /** + * Configure whether to register the Bouncy Castle Security Provider. Must be called prior to other methods + * + * @param registerBouncyCastle Enable or disable Bouncy Castle Provider registration on subsequent method invocation + */ public static synchronized void setRegisterBouncyCastle(boolean registerBouncyCastle) { SecurityUtils.registerBouncyCastle = registerBouncyCastle; registrationDone = false; diff --git a/src/test/java/net/schmizz/sshj/DefaultSecurityProviderConfigTest.java b/src/test/java/net/schmizz/sshj/DefaultSecurityProviderConfigTest.java new file mode 100644 index 00000000..74378d8a --- /dev/null +++ b/src/test/java/net/schmizz/sshj/DefaultSecurityProviderConfigTest.java @@ -0,0 +1,65 @@ +/* + * Copyright (C)2009 - SSHJ Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package net.schmizz.sshj; + +import net.schmizz.sshj.common.SecurityUtils; + +import static org.junit.jupiter.api.Assertions.assertFalse; + +import java.security.Provider; +import java.security.Security; +import java.util.Arrays; +import java.util.Optional; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +public class DefaultSecurityProviderConfigTest { + + private static Provider bouncyCastleProvider; + + @BeforeAll + public static void removeProviders() { + bouncyCastleProvider = Security.getProvider(SecurityUtils.BOUNCY_CASTLE); + if (bouncyCastleProvider != null) { + Security.removeProvider(SecurityUtils.BOUNCY_CASTLE); + } + } + + @AfterAll + public static void addProviders() { + if (bouncyCastleProvider != null) { + Security.addProvider(bouncyCastleProvider); + } + } + + @Test + public void testBouncyCastleNotRegistered() { + new DefaultSecurityProviderConfig(); + + assertBouncyCastleNotRegistered(); + } + + private void assertBouncyCastleNotRegistered() { + final Optional bouncyCastleFound = Arrays.stream(Security.getProviders()) + .map(Provider::getName) + .filter(SecurityUtils.BOUNCY_CASTLE::contentEquals) + .findFirst(); + + assertFalse(bouncyCastleFound.isPresent()); + } +} diff --git a/src/test/java/net/schmizz/sshj/keyprovider/OpenSSHKeyFileTest.java b/src/test/java/net/schmizz/sshj/keyprovider/OpenSSHKeyFileTest.java index 1320fca2..0834d4c1 100644 --- a/src/test/java/net/schmizz/sshj/keyprovider/OpenSSHKeyFileTest.java +++ b/src/test/java/net/schmizz/sshj/keyprovider/OpenSSHKeyFileTest.java @@ -19,7 +19,6 @@ import com.hierynomus.sshj.common.KeyDecryptionFailedException; import com.hierynomus.sshj.userauth.certificate.Certificate; import com.hierynomus.sshj.userauth.keyprovider.OpenSSHKeyV1KeyFile; import net.schmizz.sshj.common.KeyType; -import net.schmizz.sshj.common.SecurityUtils; import net.schmizz.sshj.userauth.keyprovider.FileKeyProvider; import net.schmizz.sshj.userauth.keyprovider.OpenSSHKeyFile; import net.schmizz.sshj.userauth.password.PasswordFinder; @@ -448,13 +447,6 @@ public class OpenSSHKeyFileTest { assertThrows(IOException.class, keyProvider::getPrivate, "This key is not in 'openssh-key-v1' format"); } - @BeforeEach - public void checkBCRegistration() { - if (!SecurityUtils.isBouncyCastleRegistered()) { - throw new AssertionError("bouncy castle needed"); - } - } - private String readFile(String pathname) throws IOException { StringBuilder fileContents = new StringBuilder();