From 167fab09a42cdf778bd635ffdf06849181fe89e2 Mon Sep 17 00:00:00 2001 From: Kenny Root Date: Thu, 14 May 2026 18:45:06 -0700 Subject: [PATCH 1/8] fix(security): validate DH-GEX group parameters from server A malicious server could send SSH_MSG_KEX_DH_GEX_GROUP with a weak or degenerate prime (e.g., 16-bit p, g=1, or an even p) to trivially break the key exchange. Enforce that p falls within the client-requested [min, max] bit range, that p is odd, and that g satisfies 1 < g < p-1. --- .../crypto/DiffieHellmanGroupExchange.kt | 15 ++++ .../crypto/DiffieHellmanGroupExchangeTest.kt | 76 ++++++++++++++++++- 2 files changed, 90 insertions(+), 1 deletion(-) diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/DiffieHellmanGroupExchange.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/DiffieHellmanGroupExchange.kt index 10053e2..b48d2fb 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/DiffieHellmanGroupExchange.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/DiffieHellmanGroupExchange.kt @@ -44,6 +44,21 @@ internal class DiffieHellmanGroupExchange(override val hashAlgorithm: String) : private var privateKey: BigInteger? = null fun setGroup(p: BigInteger, g: BigInteger) { + val pBits = p.bitLength() + if (pBits < min) { + throw SshException("DH group prime too small: $pBits bits < $min bits minimum") + } + if (pBits > max) { + throw SshException("DH group prime too large: $pBits bits > $max bits maximum") + } + // p must be odd (a basic primality sanity check: all primes > 2 are odd) + if (!p.testBit(0)) { + throw SshException("DH group prime is even") + } + // g must satisfy 1 < g < p-1 to be a valid generator candidate + if (g <= BigInteger.ONE || g >= p - BigInteger.ONE) { + throw SshException("DH group generator g is out of range: must be 1 < g < p-1") + } this.p = p this.g = g } diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/DiffieHellmanGroupExchangeTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/DiffieHellmanGroupExchangeTest.kt index 2fb58c2..f84e8c9 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/DiffieHellmanGroupExchangeTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/DiffieHellmanGroupExchangeTest.kt @@ -1,6 +1,6 @@ /* * ConnectBot SSH Library - * Copyright 2025 Kenny Root + * Copyright 2025-2026 Kenny Root * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -34,6 +34,80 @@ import kotlin.test.assertTrue class DiffieHellmanGroupExchangeTest { + // A tiny 16-bit prime — far below the min=2048 requested by the client + private val tinyPrime = BigInteger("65537") + + // A 2048-bit composite (GROUP14_P with the last bit flipped) — not a safe prime + private val compositeLikePrime = DhGroups.GROUP14_P.subtract(BigInteger.ONE) + + @Test + fun `setGroup rejects p smaller than min bits`() { + val gex = DiffieHellmanGroupExchange("SHA-256") + assertFailsWith { + gex.setGroup(tinyPrime, DhGroups.GENERATOR) + } + } + + @Test + fun `setGroup rejects p larger than max bits`() { + // Construct a number just over 8192 bits + val tooBig = BigInteger.ONE.shiftLeft(8193) + val gex = DiffieHellmanGroupExchange("SHA-256") + assertFailsWith { + gex.setGroup(tooBig, DhGroups.GENERATOR) + } + } + + @Test + fun `setGroup rejects g equal to 1`() { + val gex = DiffieHellmanGroupExchange("SHA-256") + assertFailsWith { + gex.setGroup(DhGroups.GROUP14_P, BigInteger.ONE) + } + } + + @Test + fun `setGroup rejects g equal to 0`() { + val gex = DiffieHellmanGroupExchange("SHA-256") + assertFailsWith { + gex.setGroup(DhGroups.GROUP14_P, BigInteger.ZERO) + } + } + + @Test + fun `setGroup rejects g greater than or equal to p minus 1`() { + val gex = DiffieHellmanGroupExchange("SHA-256") + assertFailsWith { + gex.setGroup(DhGroups.GROUP14_P, DhGroups.GROUP14_P - BigInteger.ONE) + } + } + + @Test + fun `setGroup rejects even p`() { + val evenP = DhGroups.GROUP14_P.subtract(BigInteger.ONE) // make it even + val gex = DiffieHellmanGroupExchange("SHA-256") + assertFailsWith { + gex.setGroup(evenP, DhGroups.GENERATOR) + } + } + + @Test + fun `setGroup accepts valid group 14 parameters`() { + val gex = DiffieHellmanGroupExchange("SHA-256") + gex.setGroup(DhGroups.GROUP14_P, DhGroups.GENERATOR) + // Should not throw; verify we can generate keys + val e = gex.generateClientKeys() + assertTrue(e.isNotEmpty()) + } + + @Test + fun `setGroup accepts valid group 16 parameters`() { + val gex = DiffieHellmanGroupExchange("SHA-256") + gex.setGroup(DhGroups.GROUP16_P, DhGroups.GENERATOR) + val e = gex.generateClientKeys() + assertTrue(e.isNotEmpty()) + } + @Test fun `generateClientKeys throws if group not set`() { val gex = DiffieHellmanGroupExchange("SHA-256") From 25deb8663f9b00b68f1f155a92c3cd46927d3bb7 Mon Sep 17 00:00:00 2001 From: Kenny Root Date: Thu, 14 May 2026 18:51:30 -0700 Subject: [PATCH 2/8] fix(security): enforce negotiated algorithm in host key signature verification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A server could send a signature blob claiming algorithm "ssh-rsa" (SHA-1) even when the client negotiated "rsa-sha2-256" or "rsa-sha2-512", causing the client to verify with the weaker SHA-1 hash. Per RFC 8332 §3.2, the algorithm identifier in the signature blob must match what was negotiated. SignatureVerifier.verify() now requires the expected algorithm and rejects mismatches. Agent session binding uses a separate verifyWithKeyType() path that validates the sig algorithm is compatible with the host key type, guarding against cross-type forgery in that context. --- .../sshlib/client/AgentProtocolHandler.kt | 4 +- .../connectbot/sshlib/client/SshConnection.kt | 4 +- .../sshlib/crypto/SignatureVerifier.kt | 45 +++++- .../sshlib/crypto/SignatureVerifierTest.kt | 135 ++++++++++++++++++ 4 files changed, 183 insertions(+), 5 deletions(-) create mode 100644 sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/SignatureVerifierTest.kt diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentProtocolHandler.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentProtocolHandler.kt index ee676d2..3179e2a 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentProtocolHandler.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentProtocolHandler.kt @@ -1,6 +1,6 @@ /* * ConnectBot SSH Library - * Copyright 2025 Kenny Root + * Copyright 2025-2026 Kenny Root * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -135,7 +135,7 @@ internal class AgentProtocolHandler( private val provider: AgentProvider, private val sessionInfo: AgentSessionInfo, private val bindVerifier: SessionBindVerifier = SessionBindVerifier { hk, sig, data -> - SignatureVerifier.verify(hk, sig, data) + SignatureVerifier.verifyWithKeyType(hk, sig, data) }, ) { companion object { diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SshConnection.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SshConnection.kt index 43dcdeb..1c73aff 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SshConnection.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SshConnection.kt @@ -1329,7 +1329,9 @@ class SshConnection( } serverHostKeyBlob = serverHostKey - if (!SignatureVerifier.verify(serverHostKey, signature, hash)) { + val negotiatedAlg = negotiatedHostKeyAlgorithm + ?: throw SshException("No host key algorithm negotiated") + if (!SignatureVerifier.verify(serverHostKey, signature, hash, negotiatedAlg)) { logger.error("Server signature verification failed") throw SshException("Server signature verification failed") } diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/SignatureVerifier.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/SignatureVerifier.kt index 6d3d0c1..aaf7308 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/SignatureVerifier.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/SignatureVerifier.kt @@ -1,6 +1,6 @@ /* * ConnectBot SSH Library - * Copyright 2025 Kenny Root + * Copyright 2025-2026 Kenny Root * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -23,10 +23,22 @@ import org.connectbot.sshlib.protocol.SshSignature internal object SignatureVerifier { - fun verify(serverHostKey: ByteArray, signatureData: ByteArray, exchangeHash: ByteArray): Boolean { + /** + * Verifies the server's host key signature during KEX. + * + * Enforces that the algorithm name in the signature blob matches [expectedAlgorithm] + * (the negotiated host key algorithm), per RFC 8332 §3.2. + */ + fun verify(serverHostKey: ByteArray, signatureData: ByteArray, exchangeHash: ByteArray, expectedAlgorithm: String): Boolean { val sig = SshSignature(ByteBufferKaitaiStream(signatureData)) sig._read() + // RFC 8332 §3.2: reject if the algorithm in the signature blob doesn't match + // what was negotiated. Prevents a server from downgrading e.g. rsa-sha2-256 → ssh-rsa. + if (sig.algorithmName() != expectedAlgorithm) { + return false + } + val pubKey = SshPublicKey(ByteBufferKaitaiStream(serverHostKey)) pubKey._read() @@ -35,4 +47,33 @@ internal object SignatureVerifier { return algorithm.verify(pubKey, sig, exchangeHash) } + + /** + * Verifies a signature where the algorithm is self-described in the signature blob + * (e.g., agent session binding). The algorithm must be compatible with the key type + * of [hostKey]. + */ + fun verifyWithKeyType(hostKey: ByteArray, signatureData: ByteArray, data: ByteArray): Boolean { + val sig = SshSignature(ByteBufferKaitaiStream(signatureData)) + sig._read() + + val pubKey = SshPublicKey(ByteBufferKaitaiStream(hostKey)) + pubKey._read() + + val sigAlgorithm = sig.algorithmName() + val keyType = pubKey.algorithmName() + + // Ensure the sig algorithm is compatible with the key type to prevent cross-type forgery. + if (!isAlgorithmCompatibleWithKeyType(sigAlgorithm, keyType)) { + return false + } + + val algorithm = SignatureEntry.fromSshName(sigAlgorithm)?.algorithm ?: return false + return algorithm.verify(pubKey, sig, data) + } + + private fun isAlgorithmCompatibleWithKeyType(sigAlgorithm: String, keyType: String): Boolean = when (keyType) { + "ssh-rsa" -> sigAlgorithm in setOf("ssh-rsa", "rsa-sha2-256", "rsa-sha2-512") + else -> sigAlgorithm == keyType + } } diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/SignatureVerifierTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/SignatureVerifierTest.kt new file mode 100644 index 0000000..8364b7e --- /dev/null +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/SignatureVerifierTest.kt @@ -0,0 +1,135 @@ +/* + * ConnectBot SSH Library + * Copyright 2025-2026 Kenny Root + * + * 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 org.connectbot.sshlib.crypto + +import org.junit.jupiter.api.Test +import java.io.ByteArrayOutputStream +import java.io.DataOutputStream +import java.math.BigInteger +import java.security.KeyPairGenerator +import java.security.Signature +import java.security.interfaces.RSAPublicKey +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +class SignatureVerifierTest { + + private fun encodeString(out: DataOutputStream, value: ByteArray) { + out.writeInt(value.size) + out.write(value) + } + + private fun encodeString(out: DataOutputStream, value: String) = encodeString(out, value.toByteArray(Charsets.US_ASCII)) + + private fun encodeMpint(out: DataOutputStream, value: BigInteger) { + val bytes = value.toByteArray() + out.writeInt(bytes.size) + out.write(bytes) + } + + private fun buildRsaHostKey(pub: RSAPublicKey): ByteArray { + val buf = ByteArrayOutputStream() + val out = DataOutputStream(buf) + encodeString(out, "ssh-rsa") + encodeMpint(out, pub.publicExponent) + encodeMpint(out, pub.modulus) + return buf.toByteArray() + } + + private fun buildSignatureBlob(algorithmName: String, sigBytes: ByteArray): ByteArray { + val buf = ByteArrayOutputStream() + val out = DataOutputStream(buf) + encodeString(out, algorithmName) + encodeString(out, sigBytes) + return buf.toByteArray() + } + + private fun signData(data: ByteArray, jcaAlgorithm: String, kp: java.security.KeyPair): ByteArray { + val sig = Signature.getInstance(jcaAlgorithm) + sig.initSign(kp.private) + sig.update(data) + return sig.sign() + } + + @Test + fun `accepts rsa-sha2-256 signature when rsa-sha2-256 was negotiated`() { + val kp = KeyPairGenerator.getInstance("RSA").apply { initialize(2048) }.generateKeyPair() + val data = "exchange hash".toByteArray() + val sigBytes = signData(data, "SHA256withRSA", kp) + val hostKey = buildRsaHostKey(kp.public as RSAPublicKey) + val sigBlob = buildSignatureBlob("rsa-sha2-256", sigBytes) + + assertTrue(SignatureVerifier.verify(hostKey, sigBlob, data, "rsa-sha2-256")) + } + + @Test + fun `accepts rsa-sha2-512 signature when rsa-sha2-512 was negotiated`() { + val kp = KeyPairGenerator.getInstance("RSA").apply { initialize(2048) }.generateKeyPair() + val data = "exchange hash".toByteArray() + val sigBytes = signData(data, "SHA512withRSA", kp) + val hostKey = buildRsaHostKey(kp.public as RSAPublicKey) + val sigBlob = buildSignatureBlob("rsa-sha2-512", sigBytes) + + assertTrue(SignatureVerifier.verify(hostKey, sigBlob, data, "rsa-sha2-512")) + } + + @Test + fun `rejects ssh-rsa signature blob when rsa-sha2-256 was negotiated`() { + val kp = KeyPairGenerator.getInstance("RSA").apply { initialize(2048) }.generateKeyPair() + val data = "exchange hash".toByteArray() + // Server signs correctly with SHA-1, but client negotiated SHA-256 + val sigBytes = signData(data, "SHA1withRSA", kp) + val hostKey = buildRsaHostKey(kp.public as RSAPublicKey) + val sigBlob = buildSignatureBlob("ssh-rsa", sigBytes) + + assertFalse(SignatureVerifier.verify(hostKey, sigBlob, data, "rsa-sha2-256")) + } + + @Test + fun `rejects rsa-sha2-256 signature blob when rsa-sha2-512 was negotiated`() { + val kp = KeyPairGenerator.getInstance("RSA").apply { initialize(2048) }.generateKeyPair() + val data = "exchange hash".toByteArray() + val sigBytes = signData(data, "SHA256withRSA", kp) + val hostKey = buildRsaHostKey(kp.public as RSAPublicKey) + val sigBlob = buildSignatureBlob("rsa-sha2-256", sigBytes) + + assertFalse(SignatureVerifier.verify(hostKey, sigBlob, data, "rsa-sha2-512")) + } + + @Test + fun `rejects rsa-sha2-512 signature blob when rsa-sha2-256 was negotiated`() { + val kp = KeyPairGenerator.getInstance("RSA").apply { initialize(2048) }.generateKeyPair() + val data = "exchange hash".toByteArray() + val sigBytes = signData(data, "SHA512withRSA", kp) + val hostKey = buildRsaHostKey(kp.public as RSAPublicKey) + val sigBlob = buildSignatureBlob("rsa-sha2-512", sigBytes) + + assertFalse(SignatureVerifier.verify(hostKey, sigBlob, data, "rsa-sha2-256")) + } + + @Test + fun `rejects completely wrong algorithm name in signature blob`() { + val kp = KeyPairGenerator.getInstance("RSA").apply { initialize(2048) }.generateKeyPair() + val data = "exchange hash".toByteArray() + val sigBytes = signData(data, "SHA256withRSA", kp) + val hostKey = buildRsaHostKey(kp.public as RSAPublicKey) + val sigBlob = buildSignatureBlob("unknown-algo", sigBytes) + + assertFalse(SignatureVerifier.verify(hostKey, sigBlob, data, "rsa-sha2-256")) + } +} From 45a2c4d924adaa36141abf19d607fbefac1228fa Mon Sep 17 00:00:00 2001 From: Kenny Root Date: Thu, 14 May 2026 18:55:31 -0700 Subject: [PATCH 3/8] fix(security): limit zlib decompression output to prevent DoS ZlibCompressor.uncompress() had no bound on output size. A malicious server could send a small compressed packet (~1 KB) that expands to hundreds of MB, exhausting client memory. This is exploitable post-KEX on any connection that negotiates zlib or zlib@openssh.com compression. Cap decompressed output at MAX_UNCOMPRESSED_SIZE (512 KB) per packet and throw SshException if exceeded, causing the connection to be torn down. --- .../sshlib/crypto/ZlibCompressor.kt | 14 ++++++-- .../sshlib/crypto/ZlibCompressorTest.kt | 32 ++++++++++++++++++- 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/ZlibCompressor.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/ZlibCompressor.kt index f3d235e..13c393c 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/ZlibCompressor.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/ZlibCompressor.kt @@ -1,6 +1,6 @@ /* * ConnectBot SSH Library - * Copyright 2025 Kenny Root + * Copyright 2025-2026 Kenny Root * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,10 +17,16 @@ package org.connectbot.sshlib.crypto +import org.connectbot.sshlib.SshException import java.util.zip.Deflater import java.util.zip.Inflater internal class ZlibCompressor : PacketCompressor { + companion object { + // Max decompressed size per packet. Prevents decompression-bomb DoS from a malicious server. + internal const val MAX_UNCOMPRESSED_SIZE = 512 * 1024 + } + private val deflater = Deflater(5) private val inflater = Inflater() @@ -54,8 +60,12 @@ internal class ZlibCompressor : PacketCompressor { val count = inflater.inflate(output, totalOut, output.size - totalOut) totalOut += count if (count == 0) break + if (totalOut > MAX_UNCOMPRESSED_SIZE) { + throw SshException("Decompressed packet exceeds maximum allowed size ($MAX_UNCOMPRESSED_SIZE bytes)") + } if (totalOut == output.size) { - output = output.copyOf(output.size * 2) + val newSize = minOf(output.size * 2, MAX_UNCOMPRESSED_SIZE + 1) + output = output.copyOf(newSize) } } diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/ZlibCompressorTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/ZlibCompressorTest.kt index db61ada..7f8e95f 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/ZlibCompressorTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/ZlibCompressorTest.kt @@ -1,6 +1,6 @@ /* * ConnectBot SSH Library - * Copyright 2025 Kenny Root + * Copyright 2025-2026 Kenny Root * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,10 +17,12 @@ package org.connectbot.sshlib.crypto +import org.connectbot.sshlib.SshException import org.junit.jupiter.api.Assertions.assertArrayEquals import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Test import kotlin.random.Random +import kotlin.test.assertFailsWith class ZlibCompressorTest { @@ -111,6 +113,34 @@ class ZlibCompressorTest { assertEquals(data.size, decompressed.size) } + @Test + fun `rejects decompression bomb exceeding limit`() { + val compressor = ZlibCompressor() + val decompressor = ZlibCompressor() + + // Highly compressible data: 1 MB of zeros compresses to ~1 KB + val bomb = ByteArray(1_024 * 1_024) + val compressed = compressor.compress(bomb) + + // The compressed form is small, but decompressing it must be rejected + // when the output would exceed MAX_UNCOMPRESSED_SIZE + assertFailsWith { + decompressor.uncompress(compressed) + } + } + + @Test + fun `accepts decompressed output within limit`() { + val compressor = ZlibCompressor() + val decompressor = ZlibCompressor() + + // Half of the limit — should succeed + val data = ByteArray(ZlibCompressor.MAX_UNCOMPRESSED_SIZE / 2) { it.toByte() } + val compressed = compressor.compress(data) + val result = decompressor.uncompress(compressed) + assertArrayEquals(data, result) + } + @Test fun smallInputFitsInInitialBuffer() { val compressor = ZlibCompressor() From 57ae9b04424efa563c9c2fbd47a56fcf0b1f2d1f Mon Sep 17 00:00:00 2001 From: Kenny Root Date: Thu, 14 May 2026 19:48:09 -0700 Subject: [PATCH 4/8] fix(security): validate SSH_MSG_CHANNEL_WINDOW_ADJUST to prevent uint32 overflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A malicious server could send a window adjust that overflows the uint32 maximum (0xFFFFFFFF per RFC 4254 §5.2), causing remoteWindowSize to wrap negative and deadlock the send loop, or allowing unbounded window growth that breaks flow control. Reject adjustments ≤ 0 or that would push the window past the protocol maximum. --- .../connectbot/sshlib/client/AgentChannel.kt | 10 ++++- .../sshlib/client/ForwardingChannel.kt | 10 ++++- .../sshlib/client/SessionChannel.kt | 10 ++++- .../sshlib/client/SessionChannelTest.kt | 45 ++++++++++++++++++- 4 files changed, 71 insertions(+), 4 deletions(-) diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentChannel.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentChannel.kt index c8d3519..8cc3c5e 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentChannel.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentChannel.kt @@ -1,6 +1,6 @@ /* * ConnectBot SSH Library - * Copyright 2025 Kenny Root + * Copyright 2025-2026 Kenny Root * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -18,6 +18,7 @@ package org.connectbot.sshlib.client import kotlinx.coroutines.channels.Channel +import org.connectbot.sshlib.SshException import org.slf4j.LoggerFactory internal class AgentChannel( @@ -30,6 +31,7 @@ internal class AgentChannel( ) { companion object { private val logger = LoggerFactory.getLogger(AgentChannel::class.java) + private const val MAX_WINDOW_SIZE = 0xFFFFFFFFL } private var _isOpen = true @@ -55,6 +57,12 @@ internal class AgentChannel( } fun onWindowAdjust(bytesToAdd: Long) { + if (bytesToAdd <= 0) { + throw SshException("Invalid window adjust: bytesToAdd must be positive, got $bytesToAdd") + } + if (remoteWindowSize + bytesToAdd > MAX_WINDOW_SIZE) { + throw SshException("Channel window overflow: current=$remoteWindowSize, adding=$bytesToAdd exceeds max $MAX_WINDOW_SIZE") + } remoteWindowSize += bytesToAdd logger.debug("Agent channel window adjust +$bytesToAdd, remote window now $remoteWindowSize") if (remoteWindowSize > 0) { diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/ForwardingChannel.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/ForwardingChannel.kt index 2e80434..1bfa996 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/ForwardingChannel.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/ForwardingChannel.kt @@ -1,6 +1,6 @@ /* * ConnectBot SSH Library - * Copyright 2025 Kenny Root + * Copyright 2025-2026 Kenny Root * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -19,6 +19,7 @@ package org.connectbot.sshlib.client import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.channels.ReceiveChannel +import org.connectbot.sshlib.SshException import org.slf4j.LoggerFactory internal class ForwardingChannel( @@ -32,6 +33,7 @@ internal class ForwardingChannel( companion object { private val logger = LoggerFactory.getLogger(ForwardingChannel::class.java) private const val WINDOW_ADJUST_THRESHOLD = 64 * 1024 + private const val MAX_WINDOW_SIZE = 0xFFFFFFFFL } private var _isOpen = true @@ -57,6 +59,12 @@ internal class ForwardingChannel( } internal fun onWindowAdjust(bytesToAdd: Long) { + if (bytesToAdd <= 0) { + throw SshException("Invalid window adjust: bytesToAdd must be positive, got $bytesToAdd") + } + if (remoteWindowSize + bytesToAdd > MAX_WINDOW_SIZE) { + throw SshException("Channel window overflow: current=$remoteWindowSize, adding=$bytesToAdd exceeds max $MAX_WINDOW_SIZE") + } remoteWindowSize += bytesToAdd logger.debug("Forwarding channel window adjust +$bytesToAdd, remote window now $remoteWindowSize") if (remoteWindowSize > 0) { diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt index d4ef7e7..92910a3 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt @@ -1,6 +1,6 @@ /* * ConnectBot SSH Library - * Copyright 2025 Kenny Root + * Copyright 2025-2026 Kenny Root * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -25,6 +25,7 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock +import org.connectbot.sshlib.SshException import org.connectbot.sshlib.SshSession import org.connectbot.sshlib.protocol.ByteString import org.connectbot.sshlib.protocol.ChannelRequestExec @@ -52,6 +53,7 @@ class SessionChannel internal constructor( companion object { private val logger = LoggerFactory.getLogger(SessionChannel::class.java) private const val WINDOW_ADJUST_THRESHOLD = 16 * 1024 + private const val MAX_WINDOW_SIZE = 0xFFFFFFFFL } private var _isOpen = true @@ -109,6 +111,12 @@ class SessionChannel internal constructor( } internal fun onWindowAdjust(bytesToAdd: Long) { + if (bytesToAdd <= 0) { + throw SshException("Invalid window adjust: bytesToAdd must be positive, got $bytesToAdd") + } + if (remoteWindowSize + bytesToAdd > MAX_WINDOW_SIZE) { + throw SshException("Channel window overflow: current=$remoteWindowSize, adding=$bytesToAdd exceeds max $MAX_WINDOW_SIZE") + } remoteWindowSize += bytesToAdd logger.debug("Window adjust +$bytesToAdd, remote window now $remoteWindowSize") if (remoteWindowSize > 0) { diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/SessionChannelTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/SessionChannelTest.kt index 5f98257..863a2a8 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/SessionChannelTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/SessionChannelTest.kt @@ -1,6 +1,6 @@ /* * ConnectBot SSH Library - * Copyright 2025 Kenny Root + * Copyright 2025-2026 Kenny Root * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -25,11 +25,13 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.UnconfinedTestDispatcher import kotlinx.coroutines.test.runTest +import org.connectbot.sshlib.SshException import org.junit.jupiter.api.Assertions.assertArrayEquals import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Assertions.assertFalse import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.Test +import kotlin.test.assertFailsWith @OptIn(ExperimentalCoroutinesApi::class) class SessionChannelTest { @@ -157,4 +159,45 @@ class SessionChannelTest { coVerify(exactly = 1) { conn.sendChannelClose(1) } } + + @Test + fun `onWindowAdjust throws when remote window would exceed max uint32`() = runTest { + // Start with a large initial window and try to push it over 2^32-1 + val (channel, _) = createChannel( + connection = mockk(relaxed = true), + initialWindowSize = 64 * 1024, + ) + // channel starts with remoteWindowSizeInitial = 64 * 1024; add enough to overflow uint32 + val overflow = (0xFFFFFFFFL - 64 * 1024L) + 1L + assertFailsWith { + channel.onWindowAdjust(overflow) + } + } + + @Test + fun `onWindowAdjust accepts legitimate adjustment within uint32 range`() = runTest { + val (channel, _) = createChannel( + connection = mockk(relaxed = true), + initialWindowSize = 64 * 1024, + ) + // Valid: bring window up to exactly 2^32-1 + val maxAdd = 0xFFFFFFFFL - 64 * 1024L + channel.onWindowAdjust(maxAdd) // should not throw + } + + @Test + fun `onWindowAdjust rejects zero adjustment`() = runTest { + val (channel, _) = createChannel(connection = mockk(relaxed = true)) + assertFailsWith { + channel.onWindowAdjust(0L) + } + } + + @Test + fun `onWindowAdjust rejects negative adjustment`() = runTest { + val (channel, _) = createChannel(connection = mockk(relaxed = true)) + assertFailsWith { + channel.onWindowAdjust(-1L) + } + } } From f1e9a51c7c48f1acd188d0950801cbfbf31124bf Mon Sep 17 00:00:00 2001 From: Kenny Root Date: Thu, 14 May 2026 20:02:18 -0700 Subject: [PATCH 5/8] fix(security): enforce local receive window on incoming channel data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A malicious server could send SSH_MSG_CHANNEL_DATA beyond the advertised local window (RFC 4254 §5.2), causing unbounded memory growth in the Channel.UNLIMITED queues (DoS) and integer overflow in the window-adjust computation when localWindowSize went negative. Extract LocalChannelWindow to centralize both local consume-and-refill and remote adjust-with-overflow-check logic, eliminating the duplication across SessionChannel, ForwardingChannel, and AgentChannel. MAX_WINDOW_SIZE now lives in LocalChannelWindow. --- .../connectbot/sshlib/client/AgentChannel.kt | 27 +++--- .../sshlib/client/ForwardingChannel.kt | 31 ++----- .../sshlib/client/LocalChannelWindow.kt | 84 ++++++++++++++++++ .../sshlib/client/SessionChannel.kt | 37 +++----- .../sshlib/client/LocalChannelWindowTest.kt | 87 +++++++++++++++++++ .../sshlib/client/SessionChannelTest.kt | 22 +++++ 6 files changed, 225 insertions(+), 63 deletions(-) create mode 100644 sshlib/src/main/kotlin/org/connectbot/sshlib/client/LocalChannelWindow.kt create mode 100644 sshlib/src/test/kotlin/org/connectbot/sshlib/client/LocalChannelWindowTest.kt diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentChannel.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentChannel.kt index 8cc3c5e..0626c62 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentChannel.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentChannel.kt @@ -18,7 +18,6 @@ package org.connectbot.sshlib.client import kotlinx.coroutines.channels.Channel -import org.connectbot.sshlib.SshException import org.slf4j.LoggerFactory internal class AgentChannel( @@ -28,16 +27,16 @@ internal class AgentChannel( private var remoteChannelNumber: Int, private val maxPacketSize: Int, remoteWindowSizeInitial: Long, + initialWindowSize: Int = 64 * 1024, ) { companion object { private val logger = LoggerFactory.getLogger(AgentChannel::class.java) - private const val MAX_WINDOW_SIZE = 0xFFFFFFFFL } private var _isOpen = true private var closeSent = false - @Volatile private var remoteWindowSize: Long = remoteWindowSizeInitial + private val window = LocalChannelWindow(initialWindowSize, remoteInitial = remoteWindowSizeInitial) private val windowAvailable = Channel(Channel.CONFLATED) val isOpen: Boolean get() = _isOpen @@ -47,8 +46,12 @@ internal class AgentChannel( logger.warn("Received data on closed agent channel") return } + val adjust = window.consumeLocal(data.size) logger.debug("Agent channel received ${data.size} bytes") + if (adjust > 0) { + connection.sendWindowAdjust(remoteChannelNumber, adjust) + } val response = handler.handleRequest(data) @@ -57,15 +60,9 @@ internal class AgentChannel( } fun onWindowAdjust(bytesToAdd: Long) { - if (bytesToAdd <= 0) { - throw SshException("Invalid window adjust: bytesToAdd must be positive, got $bytesToAdd") - } - if (remoteWindowSize + bytesToAdd > MAX_WINDOW_SIZE) { - throw SshException("Channel window overflow: current=$remoteWindowSize, adding=$bytesToAdd exceeds max $MAX_WINDOW_SIZE") - } - remoteWindowSize += bytesToAdd - logger.debug("Agent channel window adjust +$bytesToAdd, remote window now $remoteWindowSize") - if (remoteWindowSize > 0) { + window.adjustRemote(bytesToAdd) + logger.debug("Agent channel window adjust +$bytesToAdd, remote window now ${window.remoteRemaining}") + if (window.remoteRemaining > 0) { windowAvailable.trySend(Unit) } } @@ -91,17 +88,17 @@ internal class AgentChannel( private suspend fun sendData(data: ByteArray) { var offset = 0 while (offset < data.size) { - while (remoteWindowSize <= 0) { + while (window.remoteRemaining <= 0) { windowAvailable.receive() } val chunkSize = minOf( data.size - offset, - remoteWindowSize.toInt(), + window.remoteRemaining.toInt(), maxPacketSize, ) val chunk = data.copyOfRange(offset, offset + chunkSize) connection.sendChannelData(remoteChannelNumber, chunk) - remoteWindowSize -= chunkSize + window.consumeRemote(chunkSize) offset += chunkSize } } diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/ForwardingChannel.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/ForwardingChannel.kt index 1bfa996..9ff57ad 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/ForwardingChannel.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/ForwardingChannel.kt @@ -19,7 +19,6 @@ package org.connectbot.sshlib.client import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.channels.ReceiveChannel -import org.connectbot.sshlib.SshException import org.slf4j.LoggerFactory internal class ForwardingChannel( @@ -32,15 +31,11 @@ internal class ForwardingChannel( ) { companion object { private val logger = LoggerFactory.getLogger(ForwardingChannel::class.java) - private const val WINDOW_ADJUST_THRESHOLD = 64 * 1024 - private const val MAX_WINDOW_SIZE = 0xFFFFFFFFL } private var _isOpen = true private var closeSent = false - private var localWindowSize: Long = initialWindowSize.toLong() - - @Volatile private var remoteWindowSize: Long = remoteWindowSizeInitial + private val window = LocalChannelWindow(initialWindowSize, remoteInitial = remoteWindowSizeInitial) private val windowAvailable = Channel(Channel.CONFLATED) private val _incomingData = Channel(Channel.UNLIMITED) @@ -49,25 +44,17 @@ internal class ForwardingChannel( val isOpen: Boolean get() = _isOpen internal suspend fun onData(data: ByteArray) { + val adjust = window.consumeLocal(data.size) _incomingData.trySend(data) - localWindowSize -= data.size - if (localWindowSize < WINDOW_ADJUST_THRESHOLD) { - val adjust = initialWindowSize - localWindowSize.toInt() - localWindowSize += adjust + if (adjust > 0) { connection.sendWindowAdjust(remoteChannelNumber, adjust) } } internal fun onWindowAdjust(bytesToAdd: Long) { - if (bytesToAdd <= 0) { - throw SshException("Invalid window adjust: bytesToAdd must be positive, got $bytesToAdd") - } - if (remoteWindowSize + bytesToAdd > MAX_WINDOW_SIZE) { - throw SshException("Channel window overflow: current=$remoteWindowSize, adding=$bytesToAdd exceeds max $MAX_WINDOW_SIZE") - } - remoteWindowSize += bytesToAdd - logger.debug("Forwarding channel window adjust +$bytesToAdd, remote window now $remoteWindowSize") - if (remoteWindowSize > 0) { + window.adjustRemote(bytesToAdd) + logger.debug("Forwarding channel window adjust +$bytesToAdd, remote window now ${window.remoteRemaining}") + if (window.remoteRemaining > 0) { windowAvailable.trySend(Unit) } } @@ -95,17 +82,17 @@ internal class ForwardingChannel( suspend fun sendData(data: ByteArray) { var offset = 0 while (offset < data.size) { - while (remoteWindowSize <= 0) { + while (window.remoteRemaining <= 0) { windowAvailable.receive() } val chunkSize = minOf( data.size - offset, - remoteWindowSize.toInt(), + window.remoteRemaining.toInt(), maxPacketSize, ) val chunk = data.copyOfRange(offset, offset + chunkSize) connection.sendChannelData(remoteChannelNumber, chunk) - remoteWindowSize -= chunkSize + window.consumeRemote(chunkSize) offset += chunkSize } } diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/LocalChannelWindow.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/LocalChannelWindow.kt new file mode 100644 index 0000000..85e3e19 --- /dev/null +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/LocalChannelWindow.kt @@ -0,0 +1,84 @@ +/* + * ConnectBot SSH Library + * Copyright 2025-2026 Kenny Root + * + * 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 org.connectbot.sshlib.client + +import org.connectbot.sshlib.SshException + +/** + * Tracks SSH channel flow control windows per RFC 4254 §5.2. + * + * Local window: bytes we are willing to receive. Enforces that the server never + * exceeds it; auto-refills when below [adjustThreshold]. + * + * Remote window: bytes the server is willing to receive. Enforces uint32 max + * and positive-only adjustments per the protocol. + */ +internal class LocalChannelWindow( + private val initialSize: Int, + private val adjustThreshold: Int = 16 * 1024, + remoteInitial: Long = 0, +) { + companion object { + const val MAX_WINDOW_SIZE = 0xFFFFFFFFL + } + + private var localRemaining: Long = initialSize.toLong() + + @Volatile var remoteRemaining: Long = remoteInitial + private set + + /** + * Consume [size] bytes from the local window, rejecting excess data. + * Returns the window-adjust amount to send back (0 if no adjust needed). + */ + fun consumeLocal(size: Int): Int { + if (size > localRemaining) { + throw SshException("Server sent $size bytes exceeding local window ($localRemaining)") + } + localRemaining -= size + return if (localRemaining < adjustThreshold) { + val adjust = initialSize - localRemaining.toInt() + localRemaining += adjust + adjust + } else { + 0 + } + } + + /** + * Apply a window adjustment from the server, increasing the remote window. + * Validates [bytesToAdd] is positive and won't overflow the uint32 max. + */ + fun adjustRemote(bytesToAdd: Long) { + if (bytesToAdd <= 0) { + throw SshException("Invalid window adjust: bytesToAdd must be positive, got $bytesToAdd") + } + if (remoteRemaining + bytesToAdd > MAX_WINDOW_SIZE) { + throw SshException("Channel window overflow: current=$remoteRemaining, adding=$bytesToAdd exceeds max $MAX_WINDOW_SIZE") + } + remoteRemaining += bytesToAdd + } + + /** + * Consume [size] bytes from the remote window for sending. + * Caller must ensure [remoteRemaining] > 0 before calling. + */ + fun consumeRemote(size: Int) { + remoteRemaining -= size + } +} diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt index 92910a3..06c4a57 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt @@ -25,7 +25,6 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock -import org.connectbot.sshlib.SshException import org.connectbot.sshlib.SshSession import org.connectbot.sshlib.protocol.ByteString import org.connectbot.sshlib.protocol.ChannelRequestExec @@ -52,15 +51,11 @@ class SessionChannel internal constructor( ) : SshSession { companion object { private val logger = LoggerFactory.getLogger(SessionChannel::class.java) - private const val WINDOW_ADJUST_THRESHOLD = 16 * 1024 - private const val MAX_WINDOW_SIZE = 0xFFFFFFFFL } private var _isOpen = true private var closeSent = false - private var localWindowSize: Long = initialWindowSize.toLong() - - @Volatile private var remoteWindowSize: Long = remoteWindowSizeInitial + private val window = LocalChannelWindow(initialWindowSize, remoteInitial = remoteWindowSizeInitial) private val windowAvailable = Channel(Channel.CONFLATED) private val _stdout = Channel(Channel.UNLIMITED) @@ -88,38 +83,28 @@ class SessionChannel internal constructor( get() = ptyGranted && canSendChaff && obscureKeystrokeTimingIntervalMs > 0 internal suspend fun onData(data: ByteArray) { + val adjust = window.consumeLocal(data.size) _stdout.trySend(data) - localWindowSize -= data.size - if (localWindowSize < WINDOW_ADJUST_THRESHOLD) { - val adjust = initialWindowSize - localWindowSize.toInt() - localWindowSize += adjust + if (adjust > 0) { connection.sendWindowAdjust(_remoteChannelNumber, adjust) } } internal suspend fun onExtendedData(dataType: Int, data: ByteArray) { + val adjust = window.consumeLocal(data.size) _extendedData.trySend(dataType to data) if (dataType == 1) { _stderr.trySend(data) } - localWindowSize -= data.size - if (localWindowSize < WINDOW_ADJUST_THRESHOLD) { - val adjust = initialWindowSize - localWindowSize.toInt() - localWindowSize += adjust + if (adjust > 0) { connection.sendWindowAdjust(_remoteChannelNumber, adjust) } } internal fun onWindowAdjust(bytesToAdd: Long) { - if (bytesToAdd <= 0) { - throw SshException("Invalid window adjust: bytesToAdd must be positive, got $bytesToAdd") - } - if (remoteWindowSize + bytesToAdd > MAX_WINDOW_SIZE) { - throw SshException("Channel window overflow: current=$remoteWindowSize, adding=$bytesToAdd exceeds max $MAX_WINDOW_SIZE") - } - remoteWindowSize += bytesToAdd - logger.debug("Window adjust +$bytesToAdd, remote window now $remoteWindowSize") - if (remoteWindowSize > 0) { + window.adjustRemote(bytesToAdd) + logger.debug("Window adjust +$bytesToAdd, remote window now ${window.remoteRemaining}") + if (window.remoteRemaining > 0) { windowAvailable.trySend(Unit) } } @@ -163,17 +148,17 @@ class SessionChannel internal constructor( private suspend fun writeDirect(data: ByteArray) { var offset = 0 while (offset < data.size) { - while (remoteWindowSize <= 0) { + while (window.remoteRemaining <= 0) { windowAvailable.receive() } val chunkSize = minOf( data.size - offset, - remoteWindowSize.toInt(), + window.remoteRemaining.toInt(), maxPacketSize, ) val chunk = data.copyOfRange(offset, offset + chunkSize) connection.sendChannelData(_remoteChannelNumber, chunk) - remoteWindowSize -= chunkSize + window.consumeRemote(chunkSize) offset += chunkSize } } diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/LocalChannelWindowTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/LocalChannelWindowTest.kt new file mode 100644 index 0000000..9dc1026 --- /dev/null +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/LocalChannelWindowTest.kt @@ -0,0 +1,87 @@ +/* + * ConnectBot SSH Library + * Copyright 2025-2026 Kenny Root + * + * 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 org.connectbot.sshlib.client + +import org.connectbot.sshlib.SshException +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test +import kotlin.test.assertFailsWith + +class LocalChannelWindowTest { + + @Test + fun `consumeLocal rejects data exceeding window`() { + val w = LocalChannelWindow(initialSize = 1024, adjustThreshold = 0) + assertFailsWith { w.consumeLocal(1025) } + } + + @Test + fun `consumeLocal accepts data exactly filling window`() { + val w = LocalChannelWindow(initialSize = 1024, adjustThreshold = 0) + val adjust = w.consumeLocal(1024) + assertEquals(0, adjust) + } + + @Test + fun `consumeLocal returns adjust when below threshold`() { + val w = LocalChannelWindow(initialSize = 1024, adjustThreshold = 512) + val adjust = w.consumeLocal(600) + assertEquals(1024 - (1024 - 600), adjust) + } + + @Test + fun `consumeLocal returns 0 when above threshold`() { + val w = LocalChannelWindow(initialSize = 1024, adjustThreshold = 64) + val adjust = w.consumeLocal(100) + assertEquals(0, adjust) + } + + @Test + fun `adjustRemote rejects zero`() { + val w = LocalChannelWindow(initialSize = 1024) + assertFailsWith { w.adjustRemote(0) } + } + + @Test + fun `adjustRemote rejects negative`() { + val w = LocalChannelWindow(initialSize = 1024) + assertFailsWith { w.adjustRemote(-1) } + } + + @Test + fun `adjustRemote rejects overflow past uint32 max`() { + val w = LocalChannelWindow(initialSize = 1024, remoteInitial = 64 * 1024L) + val overflow = LocalChannelWindow.MAX_WINDOW_SIZE - 64 * 1024L + 1L + assertFailsWith { w.adjustRemote(overflow) } + } + + @Test + fun `adjustRemote accepts value reaching exactly uint32 max`() { + val w = LocalChannelWindow(initialSize = 1024, remoteInitial = 64 * 1024L) + val maxAdd = LocalChannelWindow.MAX_WINDOW_SIZE - 64 * 1024L + w.adjustRemote(maxAdd) + assertEquals(LocalChannelWindow.MAX_WINDOW_SIZE, w.remoteRemaining) + } + + @Test + fun `consumeRemote decrements remote window`() { + val w = LocalChannelWindow(initialSize = 1024, remoteInitial = 1000L) + w.consumeRemote(300) + assertEquals(700L, w.remoteRemaining) + } +} diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/SessionChannelTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/SessionChannelTest.kt index 863a2a8..c4df211 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/SessionChannelTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/SessionChannelTest.kt @@ -200,4 +200,26 @@ class SessionChannelTest { channel.onWindowAdjust(-1L) } } + + @Test + fun `onData rejects data exceeding local window size`() = runTest { + val (channel, _) = createChannel(initialWindowSize = 1024) + assertFailsWith { + channel.onData(ByteArray(1025)) + } + } + + @Test + fun `onData accepts data exactly filling local window`() = runTest { + val (channel, _) = createChannel(initialWindowSize = 1024) + channel.onData(ByteArray(1024)) + } + + @Test + fun `onExtendedData rejects data exceeding local window size`() = runTest { + val (channel, _) = createChannel(initialWindowSize = 512) + assertFailsWith { + channel.onExtendedData(1, ByteArray(513)) + } + } } From d92df8bac7f97ce67ef227383e8acd5f7d3d3ab6 Mon Sep 17 00:00:00 2001 From: Kenny Root Date: Thu, 14 May 2026 20:25:57 -0700 Subject: [PATCH 6/8] fix(security): reject all-zero ECDH shared secret A server could send a degenerate EC point that causes the ECDH agreement to compute a zero shared secret, yielding completely predictable session keys. This check mirrors the existing guard in Curve25519KeyExchange and MlKemHybridKeyExchange. --- .../org/connectbot/sshlib/crypto/EcdhKeyExchange.kt | 11 +++++++++-- .../connectbot/sshlib/crypto/EcdhKeyExchangeTest.kt | 11 ++++++++++- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/EcdhKeyExchange.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/EcdhKeyExchange.kt index 4f6f1ad..bdb9a12 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/EcdhKeyExchange.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/EcdhKeyExchange.kt @@ -1,6 +1,6 @@ /* * ConnectBot SSH Library - * Copyright 2025 Kenny Root + * Copyright 2025-2026 Kenny Root * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -96,7 +96,7 @@ internal class EcdhKeyExchange(private val curveName: String) : KexAlgorithm { agreement.doPhase(serverPubKey, true) val rawSecret = agreement.generateSecret() - return encodeMpint(BigInteger(1, rawSecret).toByteArray()) + return computeSharedSecretFromRaw(rawSecret) } catch (e: SshException) { throw e } catch (e: Exception) { @@ -104,6 +104,13 @@ internal class EcdhKeyExchange(private val curveName: String) : KexAlgorithm { } } + internal fun computeSharedSecretFromRaw(rawSecret: ByteArray): ByteArray { + if (rawSecret.all { it == 0.toByte() }) { + throw SshException("Invalid ECDH shared secret: all zeroes") + } + return encodeMpint(BigInteger(1, rawSecret).toByteArray()) + } + private fun encodeEcPoint(point: ECPoint): ByteArray { val x = point.affineX.toByteArray() val y = point.affineY.toByteArray() diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/EcdhKeyExchangeTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/EcdhKeyExchangeTest.kt index 9d68e87..2ccbf08 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/EcdhKeyExchangeTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/EcdhKeyExchangeTest.kt @@ -1,6 +1,6 @@ /* * ConnectBot SSH Library - * Copyright 2025 Kenny Root + * Copyright 2025-2026 Kenny Root * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -125,6 +125,15 @@ class EcdhKeyExchangeTest { } } + @Test + fun `rejects all-zero shared secret`() { + val ecdh = EcdhKeyExchange("nistp256") + ecdh.generateClientKeys() + assertFailsWith { + ecdh.computeSharedSecretFromRaw(ByteArray(32)) + } + } + @Test fun `rejects unknown curve`() { assertFailsWith { From f1f84733571dfdfa1a095ec05d8c554bd3d1aeb2 Mon Sep 17 00:00:00 2001 From: Kenny Root Date: Thu, 14 May 2026 20:32:26 -0700 Subject: [PATCH 7/8] fix(security): use constant-time MAC comparison to prevent timing oracle Replaced ByteArray.contentEquals() with MessageDigest.isEqual() in both the encrypt-and-MAC and ETM packet receive paths. The non-constant-time comparison could allow a network attacker to measure differences in comparison time to forge valid MAC tags byte-by-byte. --- .../connectbot/sshlib/transport/PacketIO.kt | 7 ++-- .../sshlib/transport/PacketIOEtmTest.kt | 38 ++++++++++++++++++- .../sshlib/transport/PacketIOTest.kt | 37 +++++++++++++++++- 3 files changed, 77 insertions(+), 5 deletions(-) diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/transport/PacketIO.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/transport/PacketIO.kt index e4af4d6..219bcde 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/transport/PacketIO.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/transport/PacketIO.kt @@ -1,6 +1,6 @@ /* * ConnectBot SSH Library - * Copyright 2025 Kenny Root + * Copyright 2025-2026 Kenny Root * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -27,6 +27,7 @@ import org.connectbot.sshlib.protocol.UnencryptedPacket import org.slf4j.LoggerFactory import java.io.ByteArrayOutputStream import java.nio.ByteBuffer +import java.security.MessageDigest import kotlin.random.Random /** @@ -321,7 +322,7 @@ internal class PacketIO(private val transport: Transport) { // Verify MAC (over plaintext for encrypt-and-MAC) val expectedMac = mac.compute(receiveSequenceNumber, decryptedPacket) - if (!receivedMac.contentEquals(expectedMac)) { + if (!MessageDigest.isEqual(receivedMac, expectedMac)) { logger.error("MAC verification failed for seq=$receiveSequenceNumber") logger.error(" Received MAC: ${receivedMac.joinToString("") { "%02x".format(it) }}") logger.error(" Expected MAC: ${expectedMac.joinToString("") { "%02x".format(it) }}") @@ -361,7 +362,7 @@ internal class PacketIO(private val transport: Transport) { // Verify MAC (over sequence_number || length || encrypted_payload) val expectedMac = mac.computeEtm(receiveSequenceNumber, encryptedLength, encryptedPayload) - if (!receivedMac.contentEquals(expectedMac)) { + if (!MessageDigest.isEqual(receivedMac, expectedMac)) { logger.error("ETM MAC verification failed for seq=$receiveSequenceNumber") throw TransportException("ETM MAC verification failed") } diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/transport/PacketIOEtmTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/transport/PacketIOEtmTest.kt index 4b3f1c8..b7104f8 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/transport/PacketIOEtmTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/transport/PacketIOEtmTest.kt @@ -1,6 +1,6 @@ /* * ConnectBot SSH Library - * Copyright 2025 Kenny Root + * Copyright 2025-2026 Kenny Root * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -24,6 +24,7 @@ import org.connectbot.sshlib.crypto.HmacSha256 import org.connectbot.sshlib.crypto.TripleDesCbcCipher import org.connectbot.sshlib.protocol.SshEnums import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertThrows import org.junit.jupiter.api.Test class PacketIOEtmTest { @@ -173,6 +174,41 @@ class PacketIOEtmTest { assertEquals(SshEnums.MessageType.SSH_MSG_NEWKEYS, parsed.messageType()) } + @Test + fun `ETM rejects tampered MAC`() { + val (cipherKey, iv, macKey) = createKeyMaterial() + + val writeTransport = ByteArrayTransport() + val writeIO = PacketIO(writeTransport) + writeIO.enableEncryption( + clientToServerCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = true), + clientToServerMac = HmacSha256(macKey.copyOf()), + serverToClientCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = false), + serverToClientMac = HmacSha256(macKey.copyOf()), + clientToServerEtm = true, + serverToClientEtm = true, + ) + runBlocking { writeIO.writePacket(SshEnums.MessageType.SSH_MSG_NEWKEYS.id().toInt()) } + + val wireData = writeTransport.getWrittenData().clone() + wireData[wireData.size - 1] = (wireData[wireData.size - 1].toInt() xor 0xFF).toByte() + + val readTransport = ByteArrayTransport(wireData) + val readIO = PacketIO(readTransport) + readIO.enableEncryption( + clientToServerCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = true), + clientToServerMac = HmacSha256(macKey.copyOf()), + serverToClientCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = false), + serverToClientMac = HmacSha256(macKey.copyOf()), + clientToServerEtm = true, + serverToClientEtm = true, + ) + + assertThrows(TransportException::class.java) { + runBlocking { readIO.readPacket() } + } + } + @Test fun `ETM multiple packets round trip with CBC`() = runBlocking { val (cipherKey, iv, macKey) = createKeyMaterial() diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/transport/PacketIOTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/transport/PacketIOTest.kt index e4a6267..ff16217 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/transport/PacketIOTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/transport/PacketIOTest.kt @@ -1,6 +1,6 @@ /* * ConnectBot SSH Library - * Copyright 2025 Kenny Root + * Copyright 2025-2026 Kenny Root * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -18,6 +18,8 @@ package org.connectbot.sshlib.transport import kotlinx.coroutines.runBlocking +import org.connectbot.sshlib.crypto.AesCbcCipher +import org.connectbot.sshlib.crypto.HmacSha256 import org.connectbot.sshlib.protocol.SshEnums import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Assertions.assertThrows @@ -169,6 +171,39 @@ class PacketIOTest { assertEquals(0L, io.bytesReceivedOnWire) } + @Test + fun `encrypt-and-MAC rejects tampered MAC`() { + val cipherKey = ByteArray(16) { it.toByte() } + val iv = ByteArray(16) { (it + 0x10).toByte() } + val macKey = ByteArray(32) { (it + 0x20).toByte() } + + val writeTransport = ByteArrayTransport() + val writeIO = PacketIO(writeTransport) + writeIO.enableEncryption( + clientToServerCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = true), + clientToServerMac = HmacSha256(macKey.copyOf()), + serverToClientCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = false), + serverToClientMac = HmacSha256(macKey.copyOf()), + ) + runBlocking { writeIO.writePacket(SshEnums.MessageType.SSH_MSG_NEWKEYS.id().toInt()) } + + val wireData = writeTransport.getWrittenData().clone() + wireData[wireData.size - 1] = (wireData[wireData.size - 1].toInt() xor 0xFF).toByte() + + val readTransport = ByteArrayTransport(wireData) + val readIO = PacketIO(readTransport) + readIO.enableEncryption( + clientToServerCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = true), + clientToServerMac = HmacSha256(macKey.copyOf()), + serverToClientCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = false), + serverToClientMac = HmacSha256(macKey.copyOf()), + ) + + assertThrows(TransportException::class.java) { + runBlocking { readIO.readPacket() } + } + } + // calculatePaddingLength: totalLength = 4 + 1 + (1 + payload.size) = 6 + payload.size. // With blockSize=8: raw = if (totalLength%8==0) 8 else 8-(totalLength%8). Bump by 8 if raw<4. // Verifying the exact padding_length byte at wire[4] kills MathMutator and ConditionalsBoundaryMutator survivors. From 7ab98eeab4c57f6ab0d5ac8a88f5d9fca725f8e0 Mon Sep 17 00:00:00 2001 From: Kenny Root Date: Thu, 21 May 2026 19:42:17 -0700 Subject: [PATCH 8/8] chore: follow up for review comments --- .../org/connectbot/sshlib/client/AgentChannel.kt | 6 +----- .../connectbot/sshlib/client/ForwardingChannel.kt | 13 +++++++------ .../connectbot/sshlib/client/LocalChannelWindow.kt | 10 ++++++++++ .../org/connectbot/sshlib/client/SessionChannel.kt | 6 +----- .../sshlib/client/LocalChannelWindowTest.kt | 7 +++++++ .../sshlib/crypto/DiffieHellmanGroupExchangeTest.kt | 3 --- 6 files changed, 26 insertions(+), 19 deletions(-) diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentChannel.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentChannel.kt index 0626c62..3b07b13 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentChannel.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentChannel.kt @@ -91,11 +91,7 @@ internal class AgentChannel( while (window.remoteRemaining <= 0) { windowAvailable.receive() } - val chunkSize = minOf( - data.size - offset, - window.remoteRemaining.toInt(), - maxPacketSize, - ) + val chunkSize = window.sendChunkSize(data.size - offset, maxPacketSize) val chunk = data.copyOfRange(offset, offset + chunkSize) connection.sendChannelData(remoteChannelNumber, chunk) window.consumeRemote(chunkSize) diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/ForwardingChannel.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/ForwardingChannel.kt index 9ff57ad..4899de3 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/ForwardingChannel.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/ForwardingChannel.kt @@ -31,11 +31,16 @@ internal class ForwardingChannel( ) { companion object { private val logger = LoggerFactory.getLogger(ForwardingChannel::class.java) + private const val WINDOW_ADJUST_THRESHOLD = 64 * 1024 } private var _isOpen = true private var closeSent = false - private val window = LocalChannelWindow(initialWindowSize, remoteInitial = remoteWindowSizeInitial) + private val window = LocalChannelWindow( + initialWindowSize, + adjustThreshold = WINDOW_ADJUST_THRESHOLD, + remoteInitial = remoteWindowSizeInitial, + ) private val windowAvailable = Channel(Channel.CONFLATED) private val _incomingData = Channel(Channel.UNLIMITED) @@ -85,11 +90,7 @@ internal class ForwardingChannel( while (window.remoteRemaining <= 0) { windowAvailable.receive() } - val chunkSize = minOf( - data.size - offset, - window.remoteRemaining.toInt(), - maxPacketSize, - ) + val chunkSize = window.sendChunkSize(data.size - offset, maxPacketSize) val chunk = data.copyOfRange(offset, offset + chunkSize) connection.sendChannelData(remoteChannelNumber, chunk) window.consumeRemote(chunkSize) diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/LocalChannelWindow.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/LocalChannelWindow.kt index 85e3e19..2b680ad 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/LocalChannelWindow.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/LocalChannelWindow.kt @@ -81,4 +81,14 @@ internal class LocalChannelWindow( fun consumeRemote(size: Int) { remoteRemaining -= size } + + /** + * Return the next send chunk size without narrowing the uint32 remote + * window until it is bounded by Int-sized data and packet limits. + */ + fun sendChunkSize(remainingData: Int, maxPacketSize: Int): Int = minOf( + remainingData.toLong(), + remoteRemaining, + maxPacketSize.toLong(), + ).toInt() } diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt index 06c4a57..65e377f 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt @@ -151,11 +151,7 @@ class SessionChannel internal constructor( while (window.remoteRemaining <= 0) { windowAvailable.receive() } - val chunkSize = minOf( - data.size - offset, - window.remoteRemaining.toInt(), - maxPacketSize, - ) + val chunkSize = window.sendChunkSize(data.size - offset, maxPacketSize) val chunk = data.copyOfRange(offset, offset + chunkSize) connection.sendChannelData(_remoteChannelNumber, chunk) window.consumeRemote(chunkSize) diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/LocalChannelWindowTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/LocalChannelWindowTest.kt index 9dc1026..7452f16 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/LocalChannelWindowTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/LocalChannelWindowTest.kt @@ -84,4 +84,11 @@ class LocalChannelWindowTest { w.consumeRemote(300) assertEquals(700L, w.remoteRemaining) } + + @Test + fun `sendChunkSize handles uint32 remote window without overflow`() { + val w = LocalChannelWindow(initialSize = 1024, remoteInitial = LocalChannelWindow.MAX_WINDOW_SIZE) + + assertEquals(32768, w.sendChunkSize(64 * 1024, 32768)) + } } diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/DiffieHellmanGroupExchangeTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/DiffieHellmanGroupExchangeTest.kt index f84e8c9..6d7457c 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/DiffieHellmanGroupExchangeTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/DiffieHellmanGroupExchangeTest.kt @@ -37,9 +37,6 @@ class DiffieHellmanGroupExchangeTest { // A tiny 16-bit prime — far below the min=2048 requested by the client private val tinyPrime = BigInteger("65537") - // A 2048-bit composite (GROUP14_P with the last bit flipped) — not a safe prime - private val compositeLikePrime = DhGroups.GROUP14_P.subtract(BigInteger.ONE) - @Test fun `setGroup rejects p smaller than min bits`() { val gex = DiffieHellmanGroupExchange("SHA-256")