From f60f75979c1174e3a1be8823717c6549f2198a76 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Sat, 13 Jul 2024 22:41:05 +0200 Subject: [PATCH] GH-524: Use built-in native AES from SunJCE if available Bouncy Castle apparently only has native implementations in its "LTS" releases. BC 1.78.1 has none. SunJCE uses native code. The "security registrar" architecture in Apache MINA sshd prefers registered providers over the platform providers, and it automatically registers Bouncy Castle if it's present. So with Bouncy Castle on the classpath and an SSH connection for which an AES cipher was specified, Apache MINA sshd would use the much slower Bouncy Castle AES. Add a new "SunJCEWrapper" registrar that brings the SunJCE AES and HmacSHA* to the front of the list, so that they are used even if Bouncy Castle is also registered. The new registrar can be disabled via a system property as usual, and it is only enabled if the JVM indeed has a "SunJCE" security provider registered. See also https://issues.apache.org/jira/browse/SSHD-719 (issue closed as "not needed"). Bug: https://github.com/apache/mina-sshd/issues/524 --- CHANGES.md | 12 +++ .../common/util/security/SecurityUtils.java | 1 + .../SunJCESecurityProviderRegistrar.java | 102 ++++++++++++++++++ .../sshd/common/cipher/BaseCipherTest.java | 10 +- .../org/apache/sshd/common/kex/SNTRUP761.java | 4 - 5 files changed, 124 insertions(+), 5 deletions(-) create mode 100644 sshd-common/src/main/java/org/apache/sshd/common/util/security/SunJCESecurityProviderRegistrar.java diff --git a/CHANGES.md b/CHANGES.md index 401e750e3..bfdb282b9 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -38,11 +38,23 @@ ## Bug Fixes +* [GH-524](https://github.com/apache/mina-sshd/issues/524) Performance improvements + ## New Features * New utility methods `SftpClient.put(Path localFile, String remoteFileName)` and `SftpClient.put(InputStream in, String remoteFileName)` facilitate SFTP file uploading. ## Potential compatibility issues +There is a new `SecurityProviderRegistrar` that is registered by default +if there is a `SunJCE` security provider and that uses the AES and +HmacSHA* implementations from `SunJCE` even if Bouncy Castle is also +registered. `SunJCE` has native implementation, whereas Bouncy Castle +may not. + +The new registrar has the name "SunJCEWrapper" and can be configured +like any other registrar. It can be disabled via the system property +`org.apache.sshd.security.provider.SunJCEWrapper.enabled=false`. + ## Major Code Re-factoring 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 index 6ce12254d..afb03a48b 100644 --- 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 @@ -134,6 +134,7 @@ public final class SecurityUtils { public static final String SECURITY_PROVIDER_REGISTRARS = "org.apache.sshd.security.registrars"; public static final List DEFAULT_SECURITY_PROVIDER_REGISTRARS = Collections.unmodifiableList( Arrays.asList( + "org.apache.sshd.common.util.security.SunJCESecurityProviderRegistrar", "org.apache.sshd.common.util.security.bouncycastle.BouncyCastleSecurityProviderRegistrar", "org.apache.sshd.common.util.security.eddsa.EdDSASecurityProviderRegistrar")); diff --git a/sshd-common/src/main/java/org/apache/sshd/common/util/security/SunJCESecurityProviderRegistrar.java b/sshd-common/src/main/java/org/apache/sshd/common/util/security/SunJCESecurityProviderRegistrar.java new file mode 100644 index 000000000..cb499d8a6 --- /dev/null +++ b/sshd-common/src/main/java/org/apache/sshd/common/util/security/SunJCESecurityProviderRegistrar.java @@ -0,0 +1,102 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.sshd.common.util.security; + +import java.security.Provider; +import java.security.Security; +import java.util.HashMap; +import java.util.Map; + +import org.apache.sshd.common.util.GenericUtils; + +/** + * This is registrar ensures that even if other registrars are active, we still use the Java built-in security provider + * at least for some security entities. + *

+ * The problem is that if the Bouncy Castle registrar is present and enabled, we'll end up using the Bouncy Castle + * implementations for just about anything. But not all Bouncy Castle versions have native implementations of the + * algorithms. If BC AES is used and is implemented in Java, performance will be very poor. SunJCE's AES uses native + * code and is much faster. + *

+ *

+ * If no Bouncy Castle is registered, this extra registrar will not have an effect. Like all registrars, this one can be + * disabled via a system property {@code org.apache.sshd.security.provider.SunJCEWrapper.enabled=false}. Note that this + * does not disable the fallback to the platform provider; it only disables this wrapper which can be used to + * force the use of the "SunJCE" standard Java provider even if some other registrar also supports an algorithm (and + * would thus normally be preferred). + *

+ *

+ * The registrar can be configured as usual. By default it has only the AES cipher and the SHA macs enabled, everything + * else is disabled. + *

+ * + * @author Apache MINA SSHD Project + */ +public class SunJCESecurityProviderRegistrar extends AbstractSecurityProviderRegistrar { + + private final Map defaultProperties = new HashMap<>(); + + public SunJCESecurityProviderRegistrar() { + super("SunJCEWrapper"); + String baseName = getBasePropertyName(); + defaultProperties.put(baseName + ".Cipher", "AES"); + defaultProperties.put(baseName + ".Mac", "HmacSha1,HmacSha224,HmacSha256,HmacSha384,HmacSha512"); + } + + @Override + public boolean isEnabled() { + if (!super.isEnabled()) { + return false; + } + return isSupported(); + } + + @Override + public String getDefaultSecurityEntitySupportValue(Class entityType) { + return ""; + } + + @Override + public String getString(String name) { + String configured = super.getString(name); + if (GenericUtils.isEmpty(configured)) { + String byDefault = defaultProperties.get(name); + if (byDefault != null) { + return byDefault; + } + } + return configured; + } + + @Override + public boolean isNamedProviderUsed() { + return false; + } + + @Override + public Provider getSecurityProvider() { + return Security.getProvider("SunJCE"); + } + + @Override + public boolean isSupported() { + return getSecurityProvider() != null; + } + +} diff --git a/sshd-common/src/test/java/org/apache/sshd/common/cipher/BaseCipherTest.java b/sshd-common/src/test/java/org/apache/sshd/common/cipher/BaseCipherTest.java index fd5305ce4..0203c07a9 100644 --- a/sshd-common/src/test/java/org/apache/sshd/common/cipher/BaseCipherTest.java +++ b/sshd-common/src/test/java/org/apache/sshd/common/cipher/BaseCipherTest.java @@ -22,8 +22,10 @@ import java.nio.charset.StandardCharsets; import java.security.GeneralSecurityException; import java.security.InvalidKeyException; +import java.security.spec.AlgorithmParameterSpec; import java.util.Arrays; +import javax.crypto.spec.GCMParameterSpec; import javax.crypto.spec.IvParameterSpec; import javax.crypto.spec.SecretKeySpec; @@ -78,9 +80,15 @@ protected void ensureKeySizeSupported( javax.crypto.Cipher cipher = SecurityUtils.getCipher(transformation); byte[] key = new byte[bsize]; byte[] iv = new byte[ivsize]; + AlgorithmParameterSpec params; + if (transformation.contains("/GCM/")) { + params = new GCMParameterSpec(128, iv); + } else { + params = new IvParameterSpec(iv); + } cipher.init(javax.crypto.Cipher.ENCRYPT_MODE, new SecretKeySpec(key, algorithm), - new IvParameterSpec(iv)); + params); } catch (GeneralSecurityException e) { if (e instanceof InvalidKeyException) { Assume.assumeTrue(algorithm + "/" + transformation + "[" + bsize + "/" + ivsize + "]", false /* diff --git a/sshd-core/src/main/java/org/apache/sshd/common/kex/SNTRUP761.java b/sshd-core/src/main/java/org/apache/sshd/common/kex/SNTRUP761.java index 81df2340c..313aa3dc7 100644 --- a/sshd-core/src/main/java/org/apache/sshd/common/kex/SNTRUP761.java +++ b/sshd-core/src/main/java/org/apache/sshd/common/kex/SNTRUP761.java @@ -21,7 +21,6 @@ import java.security.SecureRandom; import java.util.Arrays; -import org.apache.sshd.common.util.security.SecurityUtils; import org.bouncycastle.crypto.AsymmetricCipherKeyPair; import org.bouncycastle.crypto.SecretWithEncapsulation; import org.bouncycastle.pqc.crypto.ntruprime.SNTRUPrimeKEMExtractor; @@ -42,9 +41,6 @@ private SNTRUP761() { } static boolean isSupported() { - if (!SecurityUtils.isBouncyCastleRegistered()) { - return false; - } try { return SNTRUPrimeParameters.sntrup761.getSessionKeySize() == 256; // BC < 1.78 had only 128 } catch (Throwable e) {