This is an automated email from the ASF dual-hosted git repository. lgoldstein pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
commit 6e63c9d0d42a33fb98a82d4b5dac66da08398db3 Author: Lyor Goldstein <lgoldst...@apache.org> AuthorDate: Mon Sep 23 09:12:49 2019 +0300 [SSHD-941] Lazy-load and cache internal moduli used in Diffie-Helman group key exchange --- CHANGES.md | 3 + README.md | 1 + .../org/apache/sshd/client/kex/DHGEXClient.java | 18 ++++- .../org/apache/sshd/server/kex/DHGEXServer.java | 87 ++++++++++++++-------- .../java/org/apache/sshd/server/kex/Moduli.java | 51 ++++++++++++- .../org/apache/sshd/server/kex/ModuliTest.java | 70 +++++++++++++++++ 6 files changed, 192 insertions(+), 38 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 1de1064..70c4c90 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -40,6 +40,9 @@ peer version data is received. by tracking the initialized channels identifiers and being lenient only if command is received for a channel that was initialized in the past. +* The internal moduli used in Diffie-Hellman group exchange are **cached** - lazy-loaded the 1st time such an exchange +occurs. The cache can be invalidated (and thus force a re-load) by invoking `Moduli#clearInternalModuliCache`. + ## Behavioral changes and enhancements * [SSHD-926](https://issues.apache.org/jira/browse/SSHD-930) - Add support for OpenSSH 'lsets...@openssh.com' SFTP protocol extension. diff --git a/README.md b/README.md index 49b4e38..ac35e7b 100644 --- a/README.md +++ b/README.md @@ -17,6 +17,7 @@ based applications requiring SSH support. * [RFC 4335 - The Secure Shell (SSH) Session Channel Break Extension](https://tools.ietf.org/html/rfc4335) * [RFC 4344 - The Secure Shell (SSH) Transport Layer Encryption Modes](https://tools.ietf.org/html/rfc4344) * [RFC 4345 - Improved Arcfour Modes for the Secure Shell (SSH) Transport Layer Protocol](https://tools.ietf.org/html/rfc4345) +* [RFC 4419 - Diffie-Hellman Group Exchange for the Secure Shell (SSH) Transport Layer Protocol](https://tools.ietf.org/html/rfc4419) * [RFC 4716 - The Secure Shell (SSH) Public Key File Format](https://tools.ietf.org/html/rfc4716) * [RFC 5480 - Elliptic Curve Cryptography Subject Public Key Information](https://tools.ietf.org/html/rfc5480) * [RFC 6668 - SHA-2 Data Integrity Verification for the Secure Shell (SSH) Transport Layer Protocol](https://tools.ietf.org/html/rfc6668) diff --git a/sshd-core/src/main/java/org/apache/sshd/client/kex/DHGEXClient.java b/sshd-core/src/main/java/org/apache/sshd/client/kex/DHGEXClient.java index 597079b..ced2125 100644 --- a/sshd-core/src/main/java/org/apache/sshd/client/kex/DHGEXClient.java +++ b/sshd-core/src/main/java/org/apache/sshd/client/kex/DHGEXClient.java @@ -87,8 +87,10 @@ public class DHGEXClient extends AbstractDHClientKeyExchange { public void init(Session s, byte[] v_s, byte[] v_c, byte[] i_s, byte[] i_c) throws Exception { super.init(s, v_s, v_c, i_s, i_c); if (log.isDebugEnabled()) { - log.debug("init({}) Send SSH_MSG_KEX_DH_GEX_REQUEST", s); + log.debug("init({})[{}] Send SSH_MSG_KEX_DH_GEX_REQUEST - min={}, prf={}, max={}", + this, s, min, prf, max); } + Buffer buffer = s.createBuffer(SshConstants.SSH_MSG_KEX_DH_GEX_REQUEST, Integer.SIZE); buffer.putInt(min); buffer.putInt(prf); @@ -104,8 +106,9 @@ public class DHGEXClient extends AbstractDHClientKeyExchange { Session session = getSession(); boolean debugEnabled = log.isDebugEnabled(); if (debugEnabled) { - log.debug("next({})[{}] process command={}", - this, session, KeyExchange.getGroupKexOpcodeName(cmd)); + log.debug("next({})[{}] process command={} (expected={})", + this, session, KeyExchange.getGroupKexOpcodeName(cmd), + KeyExchange.getGroupKexOpcodeName(expected)); } if (cmd != expected) { @@ -126,6 +129,7 @@ public class DHGEXClient extends AbstractDHClientKeyExchange { if (debugEnabled) { log.debug("next({})[{}] Send SSH_MSG_KEX_DH_GEX_INIT", this, session); } + buffer = session.createBuffer( SshConstants.SSH_MSG_KEX_DH_GEX_INIT, e.length + Byte.SIZE); buffer.putMPInt(e); @@ -135,6 +139,11 @@ public class DHGEXClient extends AbstractDHClientKeyExchange { } if (cmd == SshConstants.SSH_MSG_KEX_DH_GEX_REPLY) { + if (debugEnabled) { + log.debug("next({})[{}] validate SSH_MSG_KEX_DH_GEX_REPLY - min={}, prf={}, max={}", + this, session, min, prf, max); + } + byte[] k_s = buffer.getBytes(); f = buffer.getMPIntAsBytes(); byte[] sig = buffer.getBytes(); @@ -146,7 +155,8 @@ public class DHGEXClient extends AbstractDHClientKeyExchange { String keyAlg = KeyUtils.getKeyType(serverKey); if (GenericUtils.isEmpty(keyAlg)) { - throw new SshException("Unsupported server key type"); + throw new SshException("Unsupported server key type: " + serverKey.getAlgorithm() + + " [" + serverKey.getFormat() + "]"); } buffer = new ByteArrayBuffer(); diff --git a/sshd-core/src/main/java/org/apache/sshd/server/kex/DHGEXServer.java b/sshd-core/src/main/java/org/apache/sshd/server/kex/DHGEXServer.java index 914c5b0..55d791f 100644 --- a/sshd-core/src/main/java/org/apache/sshd/server/kex/DHGEXServer.java +++ b/sshd-core/src/main/java/org/apache/sshd/server/kex/DHGEXServer.java @@ -55,7 +55,6 @@ import org.apache.sshd.server.session.ServerSession; * @author <a href="mailto:d...@mina.apache.org">Apache MINA SSHD Project</a> */ public class DHGEXServer extends AbstractDHServerKeyExchange { - protected final DHFactory factory; protected DHG dh; protected int min; @@ -73,7 +72,7 @@ public class DHGEXServer extends AbstractDHServerKeyExchange { return factory.getName(); } - public static KeyExchangeFactory newFactory(final DHFactory factory) { + public static KeyExchangeFactory newFactory(DHFactory factory) { return new KeyExchangeFactory() { @Override public KeyExchange create() { @@ -105,10 +104,13 @@ public class DHGEXServer extends AbstractDHServerKeyExchange { ServerSession session = getServerSession(); boolean debugEnabled = log.isDebugEnabled(); if (debugEnabled) { - log.debug("next({})[{}] process command={}", this, session, KeyExchange.getGroupKexOpcodeName(cmd)); + log.debug("next({})[{}] process command={} (expected={})", + this, session, KeyExchange.getGroupKexOpcodeName(cmd), + KeyExchange.getGroupKexOpcodeName(expected)); } - if (cmd == SshConstants.SSH_MSG_KEX_DH_GEX_REQUEST_OLD && expected == SshConstants.SSH_MSG_KEX_DH_GEX_REQUEST) { + if ((cmd == SshConstants.SSH_MSG_KEX_DH_GEX_REQUEST_OLD) + && (expected == SshConstants.SSH_MSG_KEX_DH_GEX_REQUEST)) { oldRequest = true; min = SecurityUtils.MIN_DHGEX_KEY_SIZE; prf = buffer.getInt(); @@ -116,15 +118,17 @@ public class DHGEXServer extends AbstractDHServerKeyExchange { if ((max < min) || (prf < min) || (max < prf)) { throw new SshException(SshConstants.SSH2_DISCONNECT_KEY_EXCHANGE_FAILED, - "Protocol error: bad parameters " + min + " !< " + prf + " !< " + max); + "Protocol error: bad parameters " + min + " !< " + prf + " !< " + max); } + dh = chooseDH(min, prf, max); f = dh.getE(); hash = dh.getHash(); hash.init(); if (debugEnabled) { - log.debug("next({})[{}] send SSH_MSG_KEX_DH_GEX_GROUP", this, session); + log.debug("next({})[{}] send (old request) SSH_MSG_KEX_DH_GEX_GROUP - min={}, prf={}, max={}", + this, session, min, prf, max); } buffer = session.createBuffer(SshConstants.SSH_MSG_KEX_DH_GEX_GROUP); @@ -136,21 +140,25 @@ public class DHGEXServer extends AbstractDHServerKeyExchange { return false; } - if (cmd == SshConstants.SSH_MSG_KEX_DH_GEX_REQUEST && expected == SshConstants.SSH_MSG_KEX_DH_GEX_REQUEST) { + if ((cmd == SshConstants.SSH_MSG_KEX_DH_GEX_REQUEST) + && (expected == SshConstants.SSH_MSG_KEX_DH_GEX_REQUEST)) { min = buffer.getInt(); prf = buffer.getInt(); max = buffer.getInt(); + if ((prf < min) || (max < prf)) { throw new SshException(SshConstants.SSH2_DISCONNECT_KEY_EXCHANGE_FAILED, - "Protocol error: bad parameters " + min + " !< " + prf + " !< " + max); + "Protocol error: bad parameters " + min + " !< " + prf + " !< " + max); } + dh = chooseDH(min, prf, max); f = dh.getE(); hash = dh.getHash(); hash.init(); if (debugEnabled) { - log.debug("next({})[{}] Send SSH_MSG_KEX_DH_GEX_GROUP", this, session); + log.debug("next({})[{}] Send SSH_MSG_KEX_DH_GEX_GROUP - min={}, prf={}, max={}", + this, session, min, prf, max); } buffer = session.createBuffer(SshConstants.SSH_MSG_KEX_DH_GEX_GROUP); buffer.putMPInt(dh.getP()); @@ -163,8 +171,8 @@ public class DHGEXServer extends AbstractDHServerKeyExchange { if (cmd != expected) { throw new SshException(SshConstants.SSH2_DISCONNECT_KEY_EXCHANGE_FAILED, - "Protocol error: expected packet " + KeyExchange.getGroupKexOpcodeName(expected) - + ", got " + KeyExchange.getGroupKexOpcodeName(cmd)); + "Protocol error: expected packet " + KeyExchange.getGroupKexOpcodeName(expected) + + ", got " + KeyExchange.getGroupKexOpcodeName(cmd)); } if (cmd == SshConstants.SSH_MSG_KEX_DH_GEX_INIT) { @@ -190,6 +198,7 @@ public class DHGEXServer extends AbstractDHServerKeyExchange { buffer.putBytes(i_c); buffer.putBytes(i_s); buffer.putBytes(k_s); + if (oldRequest) { buffer.putInt(prf); } else { @@ -197,6 +206,7 @@ public class DHGEXServer extends AbstractDHServerKeyExchange { buffer.putInt(prf); buffer.putInt(max); } + buffer.putMPInt(dh.getP()); buffer.putMPInt(dh.getG()); buffer.putMPInt(e); @@ -220,10 +230,12 @@ public class DHGEXServer extends AbstractDHServerKeyExchange { // Send response if (debugEnabled) { - log.debug("next({})[{}] Send SSH_MSG_KEX_DH_GEX_REPLY", this, session); + log.debug("next({})[{}] Send SSH_MSG_KEX_DH_GEX_REPLY - old={}, min={}, prf={}, max={}", + this, session, oldRequest, min, prf, max); } - buffer = session.prepareBuffer(SshConstants.SSH_MSG_KEX_DH_GEX_REPLY, BufferUtils.clear(buffer)); + buffer = session.prepareBuffer( + SshConstants.SSH_MSG_KEX_DH_GEX_REPLY, BufferUtils.clear(buffer)); buffer.putBytes(k_s); buffer.putBytes(f); buffer.putBytes(sigH); @@ -235,20 +247,23 @@ public class DHGEXServer extends AbstractDHServerKeyExchange { } protected DHG chooseDH(int min, int prf, int max) throws Exception { - List<Moduli.DhGroup> groups = loadModuliGroups(); - + int maxDHGroupExchangeKeySize = SecurityUtils.getMaxDHGroupExchangeKeySize(); min = Math.max(min, SecurityUtils.MIN_DHGEX_KEY_SIZE); prf = Math.max(prf, SecurityUtils.MIN_DHGEX_KEY_SIZE); - prf = Math.min(prf, SecurityUtils.getMaxDHGroupExchangeKeySize()); - max = Math.min(max, SecurityUtils.getMaxDHGroupExchangeKeySize()); - int bestSize = 0; + prf = Math.min(prf, maxDHGroupExchangeKeySize); + max = Math.min(max, maxDHGroupExchangeKeySize); + + List<Moduli.DhGroup> groups = loadModuliGroups(); + Session session = getServerSession(); List<Moduli.DhGroup> selected = new ArrayList<>(); + int bestSize = 0; boolean traceEnabled = log.isTraceEnabled(); for (Moduli.DhGroup group : groups) { int size = group.getSize(); if ((size < min) || (size > max)) { if (traceEnabled) { - log.trace("chooseDH - skip group={} - size not in range [{}-{}]", group, min, max); + log.trace("chooseDH({})[{}] - skip group={} - size not in range [{}-{}]", + this, session, group, min, max); } continue; } @@ -256,22 +271,24 @@ public class DHGEXServer extends AbstractDHServerKeyExchange { if (((size > prf) && (size < bestSize)) || ((size > bestSize) && (bestSize < prf))) { bestSize = size; if (traceEnabled) { - log.trace("chooseDH(prf={}, min={}, max={}) new best size={} from group={}", prf, min, max, bestSize, group); + log.trace("chooseDH({})[{}][prf={}, min={}, max={}] new best size={} from group={}", + this, session, prf, min, max, bestSize, group); } selected.clear(); } if (size == bestSize) { if (traceEnabled) { - log.trace("chooseDH(prf={}, min={}, max={}) selected {}", prf, min, max, group); + log.trace("chooseDH({})[{}][prf={}, min={}, max={}] selected {}", + this, session, prf, min, max, group); } selected.add(group); } } - ServerSession session = getServerSession(); if (selected.isEmpty()) { - log.warn("chooseDH({})[{}] No suitable primes found, defaulting to DHG1", this, session); + log.warn("chooseDH({})[{}][prf={}, min={}, max={}] No suitable primes found, defaulting to DHG1", + this, session, prf, min, max); return getDH(new BigInteger(DHGroupData.getP1()), new BigInteger(DHGroupData.getG())); } @@ -280,42 +297,48 @@ public class DHGEXServer extends AbstractDHServerKeyExchange { Random random = Objects.requireNonNull(factory.create(), "No random generator"); int which = random.random(selected.size()); Moduli.DhGroup group = selected.get(which); + if (traceEnabled) { + log.trace("chooseDH({})[{}][prf={}, min={}, max={}] selected {}", + this, session, prf, min, max, group); + } return getDH(group.getP(), group.getG()); } protected List<Moduli.DhGroup> loadModuliGroups() throws IOException { - ServerSession session = getServerSession(); + Session session = getServerSession(); String moduliStr = session.getString(ServerFactoryManager.MODULI_URL); List<Moduli.DhGroup> groups = null; - URL moduli; if (!GenericUtils.isEmpty(moduliStr)) { try { - moduli = new URL(moduliStr); + URL moduli = new URL(moduliStr); groups = Moduli.parseModuli(moduli); } catch (IOException e) { // OK - use internal moduli - log.warn("Error (" + e.getClass().getSimpleName() + ") loading external moduli from " + moduliStr + ": " + e.getMessage()); + log.warn("loadModuliGroups({})[{}] Error ({}) loading external moduli from {}: {}", + this, session, e.getClass().getSimpleName(), moduliStr, e.getMessage()); } } if (groups == null) { - moduliStr = "/org/apache/sshd/moduli"; + moduliStr = Moduli.INTERNAL_MODULI_RESPATH; try { - moduli = getClass().getResource(moduliStr); + URL moduli = getClass().getResource(moduliStr); if (moduli == null) { throw new FileNotFoundException("Missing internal moduli file"); } moduliStr = moduli.toExternalForm(); - groups = Moduli.parseModuli(moduli); + groups = Moduli.loadInternalModuli(moduli); } catch (IOException e) { - log.warn("Error (" + e.getClass().getSimpleName() + ") loading internal moduli from " + moduliStr + ": " + e.getMessage()); + log.warn("loadModuliGroups({})[{}] Error ({}) loading internal moduli from {}: {}", + this, session, e.getClass().getSimpleName(), moduliStr, e.getMessage()); throw e; // this time we MUST throw the exception } } if (log.isDebugEnabled()) { - log.debug("Loaded moduli groups from {}", moduliStr); + log.debug("loadModuliGroups({})[{}] Loaded moduli groups from {}", + this, session, moduliStr); } return groups; } diff --git a/sshd-core/src/main/java/org/apache/sshd/server/kex/Moduli.java b/sshd-core/src/main/java/org/apache/sshd/server/kex/Moduli.java index adb7098..481b697 100644 --- a/sshd-core/src/main/java/org/apache/sshd/server/kex/Moduli.java +++ b/sshd-core/src/main/java/org/apache/sshd/server/kex/Moduli.java @@ -19,14 +19,21 @@ package org.apache.sshd.server.kex; import java.io.BufferedReader; +import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStreamReader; import java.math.BigInteger; import java.net.URL; import java.nio.charset.StandardCharsets; +import java.util.AbstractMap.SimpleImmutableEntry; import java.util.ArrayList; +import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Objects; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.sshd.common.util.GenericUtils; /** * Helper class to load DH group primes from a file. @@ -34,6 +41,10 @@ import java.util.Objects; * @author <a href="mailto:d...@mina.apache.org">Apache MINA SSHD Project</a> */ public final class Moduli { + /** + * Resource path of internal moduli file + */ + public static final String INTERNAL_MODULI_RESPATH = "/org/apache/sshd/moduli"; public static final int MODULI_TYPE_SAFE = 2; public static final int MODULI_TESTS_COMPOSITE = 0x01; @@ -63,20 +74,53 @@ public final class Moduli { @Override public String toString() { - return "[size=" + getSize() + ",G=" + getG() + ",P=" + getP() + "]"; + return "[size=" + getSize() + ", G=" + getG() + ", P=" + getP() + "]"; } } + private static final AtomicReference<Map.Entry<String, List<DhGroup>>> INTERNAL_MODULI_HOLDER = new AtomicReference<>(); + // Private constructor private Moduli() { throw new UnsupportedOperationException("No instance allowed"); } + public static Map.Entry<String, List<DhGroup>> clearInternalModuliCache() { + return INTERNAL_MODULI_HOLDER.getAndSet(null); + } + + public static List<DhGroup> loadInternalModuli(URL url) throws IOException { + if (url == null) { + throw new FileNotFoundException("No internal moduli resource specified"); + } + + String moduliStr = url.toExternalForm(); + Map.Entry<String, List<DhGroup>> lastModuli = INTERNAL_MODULI_HOLDER.get(); + String lastResource = (lastModuli == null) ? null : lastModuli.getKey(); + if (Objects.equals(lastResource, moduliStr)) { + return lastModuli.getValue(); + } + + List<DhGroup> groups = parseModuli(url); + if (GenericUtils.isEmpty(groups)) { + groups = Collections.emptyList(); + } else { + groups = Collections.unmodifiableList(groups); + } + + INTERNAL_MODULI_HOLDER.set(new SimpleImmutableEntry<>(moduliStr, groups)); + return groups; + } + public static List<DhGroup> parseModuli(URL url) throws IOException { List<DhGroup> groups = new ArrayList<>(); try (BufferedReader r = new BufferedReader(new InputStreamReader(url.openStream(), StandardCharsets.UTF_8))) { for (String line = r.readLine(); line != null; line = r.readLine()) { line = line.trim(); + if (line.isEmpty()) { + continue; + } + if (line.startsWith("#")) { continue; } @@ -105,7 +149,10 @@ public final class Moduli { continue; } - DhGroup group = new DhGroup(Integer.parseInt(parts[4]) + 1, new BigInteger(parts[5], 16), new BigInteger(parts[6], 16)); + DhGroup group = new DhGroup( + Integer.parseInt(parts[4]) + 1, + new BigInteger(parts[5], 16), + new BigInteger(parts[6], 16)); groups.add(group); } diff --git a/sshd-core/src/test/java/org/apache/sshd/server/kex/ModuliTest.java b/sshd-core/src/test/java/org/apache/sshd/server/kex/ModuliTest.java new file mode 100644 index 0000000..ef37662 --- /dev/null +++ b/sshd-core/src/test/java/org/apache/sshd/server/kex/ModuliTest.java @@ -0,0 +1,70 @@ +/* + * 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.server.kex; + +import java.io.IOException; +import java.net.URL; +import java.util.List; + +import org.apache.sshd.common.util.GenericUtils; +import org.apache.sshd.server.kex.Moduli.DhGroup; +import org.apache.sshd.util.test.JUnitTestSupport; +import org.junit.After; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.FixMethodOrder; +import org.junit.Test; +import org.junit.runners.MethodSorters; + +/** + * @author <a href="mailto:d...@mina.apache.org">Apache MINA SSHD Project</a> + */ +@FixMethodOrder(MethodSorters.NAME_ASCENDING) +public class ModuliTest extends JUnitTestSupport { + public ModuliTest() { + super(); + } + + @BeforeClass + @AfterClass + public static void clearInternalModuliCache() { + Moduli.clearInternalModuliCache(); + } + + @Before + @After + public void clearCache() { + clearInternalModuliCache(); + } + + @Test + public void testLoadInternalModuli() throws IOException { + URL moduli = getClass().getResource(Moduli.INTERNAL_MODULI_RESPATH); + assertNotNull("Missing internal moduli resource", moduli); + + List<DhGroup> expected = Moduli.loadInternalModuli(moduli); + assertTrue("No moduli groups parsed", GenericUtils.isNotEmpty(expected)); + + for (int index = 1; index <= Byte.SIZE; index++) { + List<DhGroup> actual = Moduli.loadInternalModuli(moduli); + assertSame("Mismatched cached instance at retry #" + index, expected, actual); + } + } +}