Use claude to devise more testing#158
Merged
Merged
Conversation
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.
…ification 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.
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.
…32 overflow 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.
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.
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.
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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens SSH transport/client security and robustness by adding new tests and tightening implementations around integrity checks, compression limits, signature verification, and channel flow-control window handling.
Changes:
- Add negative-path tests for MAC tampering (encrypt-and-MAC and ETM), DH GEX parameter validation, ECDH all-zero secrets, channel window bounds, and zlib decompression limits.
- Harden implementations: constant-time MAC comparison, zlib decompression output cap, stricter DH group validation, and stricter signature verification (including negotiated algorithm enforcement).
- Refactor channel window tracking into a shared
LocalChannelWindowhelper and apply it across session/forwarding/agent channels.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sshlib/src/test/kotlin/org/connectbot/sshlib/transport/PacketIOTest.kt | Adds a regression test ensuring tampered MACs are rejected in encrypt-and-MAC mode. |
| sshlib/src/test/kotlin/org/connectbot/sshlib/transport/PacketIOEtmTest.kt | Adds a regression test ensuring tampered MACs are rejected in ETM mode. |
| sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/ZlibCompressorTest.kt | Adds tests for decompression-bomb rejection and within-limit decompression acceptance. |
| sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/SignatureVerifierTest.kt | New tests asserting negotiated-signature-algorithm enforcement for RSA SHA-2 vs SHA-1. |
| sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/EcdhKeyExchangeTest.kt | Adds test rejecting an all-zero ECDH shared secret. |
| sshlib/src/test/kotlin/org/connectbot/sshlib/crypto/DiffieHellmanGroupExchangeTest.kt | Adds tests for DH GEX group parameter validation (bit sizes, generator bounds, parity). |
| sshlib/src/test/kotlin/org/connectbot/sshlib/client/SessionChannelTest.kt | Adds tests for window-adjust validation and local-window enforcement on incoming data. |
| sshlib/src/test/kotlin/org/connectbot/sshlib/client/LocalChannelWindowTest.kt | New unit tests for LocalChannelWindow window consumption and adjustment rules. |
| sshlib/src/main/kotlin/org/connectbot/sshlib/transport/PacketIO.kt | Switches MAC comparisons to constant-time MessageDigest.isEqual. |
| sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/ZlibCompressor.kt | Adds an uncompressed-output cap to mitigate decompression-bomb DoS. |
| sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/SignatureVerifier.kt | Enforces negotiated signature algorithm during KEX; adds key-type compatibility verifier for self-described signatures. |
| sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/EcdhKeyExchange.kt | Rejects all-zero shared secret via a new helper used by the main compute path. |
| sshlib/src/main/kotlin/org/connectbot/sshlib/crypto/DiffieHellmanGroupExchange.kt | Adds basic DH group sanity checks (min/max size, odd p, generator range). |
| sshlib/src/main/kotlin/org/connectbot/sshlib/client/SshConnection.kt | Plumbs negotiated host key algorithm into SignatureVerifier.verify. |
| sshlib/src/main/kotlin/org/connectbot/sshlib/client/SessionChannel.kt | Replaces ad-hoc window tracking with LocalChannelWindow. |
| sshlib/src/main/kotlin/org/connectbot/sshlib/client/LocalChannelWindow.kt | New helper to centralize channel window accounting and validation. |
| sshlib/src/main/kotlin/org/connectbot/sshlib/client/ForwardingChannel.kt | Replaces ad-hoc window tracking with LocalChannelWindow. |
| sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentProtocolHandler.kt | Uses verifyWithKeyType for session-bind signature verification. |
| sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentChannel.kt | Adds local window enforcement and migrates remote window tracking to LocalChannelWindow. |
Comments suppressed due to low confidence (1)
sshlib/src/main/kotlin/org/connectbot/sshlib/client/ForwardingChannel.kt:92
- Same issue as SessionChannel:
window.remoteRemaining.toInt()can overflow when the remote window exceedsInt.MAX_VALUE(valid for SSH uint32 windows). CalculatechunkSizeusingLongmin bounds (includingmaxPacketSize) and then convert safely toInt.
while (window.remoteRemaining <= 0) {
windowAvailable.receive()
}
val chunkSize = minOf(
data.size - offset,
window.remoteRemaining.toInt(),
maxPacketSize,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Actually came up with some nice bugs.