From e921e2a0a53630f2655e49c3230f892fc0bdbabf Mon Sep 17 00:00:00 2001 From: Kenny Root Date: Wed, 10 Jun 2026 23:03:55 -0700 Subject: [PATCH 1/2] chore: non-Unit tests should be an error Some tests were not running because they were returning values other than Unit. This can be easy to miss in Kotlin because the return types are inferred. Make this something that JUnit halts the build instead of just warning. --- build.gradle.kts | 5 + .../sshlib/client/sftp/SftpClientImplTest.kt | 156 ++++++++++-------- .../sshlib/client/sftp/SftpDispatcherTest.kt | 46 +++--- .../sshlib/client/sftp/SftpPacketIOTest.kt | 18 +- 4 files changed, 127 insertions(+), 98 deletions(-) diff --git a/build.gradle.kts b/build.gradle.kts index 6fb855c..eb4c674 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -15,6 +15,7 @@ */ import net.researchgate.release.ReleaseExtension +import org.gradle.api.tasks.testing.Test plugins { alias(libs.plugins.kotlin.jvm) apply false @@ -34,6 +35,10 @@ allprojects { google() mavenCentral() } + + tasks.withType().configureEach { + systemProperty("junit.platform.discovery.issue.severity.critical", "WARNING") + } } dependencies { diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImplTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImplTest.kt index 0662ec7..b7a0f19 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImplTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImplTest.kt @@ -54,51 +54,58 @@ class SftpClientImplTest { } @Test - fun `create returns protocol errors for malformed version response`() = runBlocking { - val wrongTypeSession = FakeSshSession() - wrongTypeSession.enqueueRead(packet(SSH_FXP_STATUS, statusPayload(SftpStatusCode.OK))) - assertIs(SftpClientImpl.create(wrongTypeSession)) - - val shortPayloadSession = FakeSshSession() - shortPayloadSession.enqueueRead(packet(SSH_FXP_VERSION, byteArrayOf(0, 0, 0))) - assertIs(SftpClientImpl.create(shortPayloadSession)) + fun `create returns protocol errors for malformed version response`() { + runBlocking { + val wrongTypeSession = FakeSshSession() + wrongTypeSession.enqueueRead(packet(SSH_FXP_STATUS, statusPayload(SftpStatusCode.OK))) + assertIs(SftpClientImpl.create(wrongTypeSession)) + + val shortPayloadSession = FakeSshSession() + shortPayloadSession.enqueueRead(packet(SSH_FXP_VERSION, byteArrayOf(0, 0, 0))) + assertIs(SftpClientImpl.create(shortPayloadSession)) + } } @Test - fun `create propagates write and read errors`() = runBlocking { - val writeFailureSession = FakeSshSession(writeFailure = IllegalStateException("cannot write")) - assertIs(SftpClientImpl.create(writeFailureSession)) - - val readFailureSession = FakeSshSession() - assertIs(SftpClientImpl.create(readFailureSession)) + fun `create propagates write and read errors`() { + runBlocking { + val writeFailureSession = FakeSshSession(writeFailure = IllegalStateException("cannot write")) + assertIs(SftpClientImpl.create(writeFailureSession)) + + val readFailureSession = FakeSshSession() + readFailureSession.closeReads() + assertIs(SftpClientImpl.create(readFailureSession)) + } } @Test - fun `create propagates protocol and server handshake results`() = runBlocking { - assertIs( - SftpClientImpl.create( - FakeSshSession(), - FakeHandshakeTransport(writeResult = SftpResult.ProtocolError("init rejected")), - ), - ) - assertIs( - SftpClientImpl.create( - FakeSshSession(), - FakeHandshakeTransport(writeResult = SftpResult.ServerError(SftpStatusCode.FAILURE, "init failed")), - ), - ) - assertIs( - SftpClientImpl.create( - FakeSshSession(), - FakeHandshakeTransport(readResult = SftpResult.ProtocolError("bad version")), - ), - ) - assertIs( - SftpClientImpl.create( - FakeSshSession(), - FakeHandshakeTransport(readResult = SftpResult.ServerError(SftpStatusCode.FAILURE, "version failed")), - ), - ) + fun `create propagates protocol and server handshake results`() { + runBlocking { + assertIs( + SftpClientImpl.create( + FakeSshSession(), + FakeHandshakeTransport(writeResult = SftpResult.ProtocolError("init rejected")), + ), + ) + assertIs( + SftpClientImpl.create( + FakeSshSession(), + FakeHandshakeTransport(writeResult = SftpResult.ServerError(SftpStatusCode.FAILURE, "init failed")), + ), + ) + assertIs( + SftpClientImpl.create( + FakeSshSession(), + FakeHandshakeTransport(readResult = SftpResult.ProtocolError("bad version")), + ), + ) + assertIs( + SftpClientImpl.create( + FakeSshSession(), + FakeHandshakeTransport(readResult = SftpResult.ServerError(SftpStatusCode.FAILURE, "version failed")), + ), + ) + } } @Test @@ -231,41 +238,48 @@ class SftpClientImplTest { } @Test - fun `directory and path operations map eof empty names and protocol errors`() = runBlocking { - val handle = SftpFileHandle(byteArrayOf(1)) - val eofClient = createClient( - FakeSshSession(responseFor = { _, _ -> response(SSH_FXP_STATUS, statusPayload(SftpStatusCode.EOF)) }), - ) - assertEquals(SftpResult.Success(null), eofClient.readdir(handle)) - - val errorClient = createClient( - FakeSshSession(responseFor = { _, _ -> response(SSH_FXP_STATUS, statusPayload(SftpStatusCode.FAILURE, "failed")) }), - ) - assertIs(errorClient.opendir("/dir")) - assertIs(errorClient.readdir(handle)) - assertIs(errorClient.realpath(".")) - assertIs(errorClient.readlink("/link")) - - val emptyNameClient = createClient( - FakeSshSession(responseFor = { _, _ -> response(SSH_FXP_NAME, namePayload(emptyList())) }), - ) - assertIs(emptyNameClient.realpath(".")) - assertIs(emptyNameClient.readlink("/link")) - - val protocolClient = createClient(FakeSshSession(responseFor = { _, _ -> response(SSH_FXP_VERSION, byteArrayOf()) })) - assertIs(protocolClient.opendir("/dir")) - assertIs(protocolClient.readdir(handle)) - assertIs(protocolClient.realpath(".")) - assertIs(protocolClient.readlink("/link")) + fun `directory and path operations map eof empty names and protocol errors`() { + runBlocking { + val handle = SftpFileHandle(byteArrayOf(1)) + val eofClient = createClient( + FakeSshSession(responseFor = { _, _ -> response(SSH_FXP_STATUS, statusPayload(SftpStatusCode.EOF)) }), + ) + assertEquals(SftpResult.Success(null), eofClient.readdir(handle)) + + val errorClient = createClient( + FakeSshSession( + responseFor = { _, _ -> response(SSH_FXP_STATUS, statusPayload(SftpStatusCode.FAILURE, "failed")) }, + ), + ) + assertIs(errorClient.opendir("/dir")) + assertIs(errorClient.readdir(handle)) + assertIs(errorClient.realpath(".")) + assertIs(errorClient.readlink("/link")) + + val emptyNameClient = createClient( + FakeSshSession(responseFor = { _, _ -> response(SSH_FXP_NAME, namePayload(emptyList())) }), + ) + assertIs(emptyNameClient.realpath(".")) + assertIs(emptyNameClient.readlink("/link")) + + val protocolClient = + createClient(FakeSshSession(responseFor = { _, _ -> response(SSH_FXP_VERSION, byteArrayOf()) })) + assertIs(protocolClient.opendir("/dir")) + assertIs(protocolClient.readdir(handle)) + assertIs(protocolClient.realpath(".")) + assertIs(protocolClient.readlink("/link")) + } } @Test - fun `dispatcher propagates request write failures`() = runBlocking { - val client = createClient(FakeSshSession(failAfterHandshake = IllegalStateException("write failed"))) + fun `dispatcher propagates request write failures`() { + runBlocking { + val client = createClient(FakeSshSession(failAfterHandshake = IllegalStateException("write failed"))) - val result = client.remove("/file") + val result = client.remove("/file") - assertIs(result) + assertIs(result) + } } private suspend fun createClient(session: FakeSshSession): SftpClient { @@ -356,6 +370,10 @@ class SftpClientImplTest { reads.trySend(data).getOrThrow() } + fun closeReads() { + reads.close() + } + override suspend fun requestPty( terminalType: String, widthChars: Int, diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcherTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcherTest.kt index 40d5e2d..4c2e363 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcherTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcherTest.kt @@ -57,19 +57,21 @@ class SftpDispatcherTest { } @Test - fun `request returns write protocol server and io failures`() = runBlocking { - val io = IllegalStateException("no channel") - assertIs( - requestWithWriteResult(SftpResult.IoError(io)), - ) - - assertIs( - requestWithWriteResult(SftpResult.ProtocolError("bad packet")), - ) - - assertIs( - requestWithWriteResult(SftpResult.ServerError(SftpStatusCode.FAILURE, "server said no")), - ) + fun `request returns write protocol server and io failures`() { + runBlocking { + val io = IllegalStateException("no channel") + assertIs( + requestWithWriteResult(SftpResult.IoError(io)), + ) + + assertIs( + requestWithWriteResult(SftpResult.ProtocolError("bad packet")), + ) + + assertIs( + requestWithWriteResult(SftpResult.ServerError(SftpStatusCode.FAILURE, "server said no")), + ) + } } @Test @@ -120,16 +122,18 @@ class SftpDispatcherTest { } @Test - fun `read loop server error exits and pending request times out`() = runBlocking { - val packetIO = FakePacketTransport() - val dispatcher = SftpDispatcher(packetIO) - dispatcher.startReadLoop(this) + fun `read loop server error exits and pending request times out`() { + runBlocking { + val packetIO = FakePacketTransport() + val dispatcher = SftpDispatcher(packetIO) + dispatcher.startReadLoop(this) - val response = async { dispatcher.request(14, ByteArray(0), timeoutMs = 50) } - packetIO.awaitWrite() - packetIO.enqueue(SftpResult.ServerError(SftpStatusCode.FAILURE, "framing server error")) + val response = async { dispatcher.request(14, ByteArray(0), timeoutMs = 50) } + packetIO.awaitWrite() + packetIO.enqueue(SftpResult.ServerError(SftpStatusCode.FAILURE, "framing server error")) - assertIs(response.await()) + assertIs(response.await()) + } } @Test diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIOTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIOTest.kt index 5e5ffde..a7382d3 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIOTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIOTest.kt @@ -31,14 +31,16 @@ import kotlin.test.assertIs class SftpPacketIOTest { @Test - fun `readPacket rejects invalid lengths`() = runBlocking { - val tooSmall = FakeSshSession() - tooSmall.enqueue(ByteBuffer.allocate(4).putInt(0).array()) - assertIs(SftpPacketIO(tooSmall).readPacket()) - - val tooLarge = FakeSshSession() - tooLarge.enqueue(ByteBuffer.allocate(4).putInt(256 * 1024 + 1).array()) - assertIs(SftpPacketIO(tooLarge).readPacket()) + fun `readPacket rejects invalid lengths`() { + runBlocking { + val tooSmall = FakeSshSession() + tooSmall.enqueue(ByteBuffer.allocate(4).putInt(0).array()) + assertIs(SftpPacketIO(tooSmall).readPacket()) + + val tooLarge = FakeSshSession() + tooLarge.enqueue(ByteBuffer.allocate(4).putInt(256 * 1024 + 1).array()) + assertIs(SftpPacketIO(tooLarge).readPacket()) + } } @Test From 5b7aa81b3b452c49736df42dad6a70186fcd2654 Mon Sep 17 00:00:00 2001 From: Kenny Root Date: Mon, 8 Jun 2026 21:37:42 -0700 Subject: [PATCH 2/2] fix(kaitai): restrict sizes for Kaitai struct Let Kaitai Struct handle more of the validation logic by supplying the expected sizes where possible. Fixes GHSA-ch3q-cw5r-f4hg --- .../main/resources/kaitai/ascii_string.ksy | 2 +- .../src/main/resources/kaitai/byte_string.ksy | 2 + .../resources/kaitai/ecdsa_signature_blob.ksy | 2 + .../resources/kaitai/encrypted_packet.ksy | 2 + .../src/main/resources/kaitai/etm_mac.ksy | 2 + protocol/src/main/resources/kaitai/mpint.ksy | 2 + .../src/main/resources/kaitai/name_list.ksy | 2 + .../restrict_destination_constraint.ksy | 4 + .../kaitai/ssh_agent_identities_answer.ksy | 2 + .../resources/kaitai/ssh_agent_message.ksy | 2 + .../resources/kaitai/ssh_msg_ext_info.ksy | 2 + .../kaitai/ssh_msg_userauth_info_request.ksy | 2 + .../kaitai/ssh_msg_userauth_info_response.ksy | 2 + .../main/resources/kaitai/ssh_public_key.ksy | 2 + .../main/resources/kaitai/ssh_signature.ksy | 2 + .../resources/kaitai/unencrypted_packet.ksy | 4 + .../userauth_request_gssapi_with_mic.ksy | 2 + .../src/main/resources/kaitai/utf8_string.ksy | 2 + .../org/connectbot/sshlib/KaitaiParsing.kt | 34 ++++ .../sshlib/client/AgentProtocolHandler.kt | 51 ++++-- .../connectbot/sshlib/client/SshConnection.kt | 14 +- .../connectbot/sshlib/transport/PacketIO.kt | 30 ++-- .../connectbot/sshlib/AgentProtocolTest.kt | 113 +++++++++++- .../org/connectbot/sshlib/SshClientTest.kt | 28 +++ .../protocol/KaitaiLengthValidationTest.kt | 161 ++++++++++++++++++ .../sshlib/transport/PacketIOTest.kt | 20 +++ 26 files changed, 462 insertions(+), 29 deletions(-) create mode 100644 sshlib/src/main/kotlin/org/connectbot/sshlib/KaitaiParsing.kt create mode 100644 sshlib/src/test/kotlin/org/connectbot/sshlib/protocol/KaitaiLengthValidationTest.kt diff --git a/protocol/src/main/resources/kaitai/ascii_string.ksy b/protocol/src/main/resources/kaitai/ascii_string.ksy index f56ebaf..fb6b6a0 100644 --- a/protocol/src/main/resources/kaitai/ascii_string.ksy +++ b/protocol/src/main/resources/kaitai/ascii_string.ksy @@ -14,7 +14,7 @@ seq: - id: len type: u4 valid: - expr: _ <= 65535 + expr: _ <= 65535 and _ <= _io.size - _io.pos - id: value type: str size: len diff --git a/protocol/src/main/resources/kaitai/byte_string.ksy b/protocol/src/main/resources/kaitai/byte_string.ksy index 0bd5461..9aeffe7 100644 --- a/protocol/src/main/resources/kaitai/byte_string.ksy +++ b/protocol/src/main/resources/kaitai/byte_string.ksy @@ -12,5 +12,7 @@ doc: > seq: - id: len_data type: u4 + valid: + expr: _ <= _io.size - _io.pos - id: data size: len_data diff --git a/protocol/src/main/resources/kaitai/ecdsa_signature_blob.ksy b/protocol/src/main/resources/kaitai/ecdsa_signature_blob.ksy index e42ce29..1f1ed38 100644 --- a/protocol/src/main/resources/kaitai/ecdsa_signature_blob.ksy +++ b/protocol/src/main/resources/kaitai/ecdsa_signature_blob.ksy @@ -7,6 +7,8 @@ doc-ref: RFC 5656 section 3.1.2 seq: - id: len_blob type: u4 + valid: + expr: _ <= _io.size - _io.pos - id: blob type: ecdsa_signature_inner size: len_blob diff --git a/protocol/src/main/resources/kaitai/encrypted_packet.ksy b/protocol/src/main/resources/kaitai/encrypted_packet.ksy index 2483e19..749b05b 100644 --- a/protocol/src/main/resources/kaitai/encrypted_packet.ksy +++ b/protocol/src/main/resources/kaitai/encrypted_packet.ksy @@ -29,6 +29,8 @@ params: seq: - id: len_encrypted_payload type: u4 + valid: + expr: _ <= _io.size - _io.pos - len_mac - id: encrypted_payload size: len_encrypted_payload - id: mac diff --git a/protocol/src/main/resources/kaitai/etm_mac.ksy b/protocol/src/main/resources/kaitai/etm_mac.ksy index 3fe2bfd..c83a29c 100644 --- a/protocol/src/main/resources/kaitai/etm_mac.ksy +++ b/protocol/src/main/resources/kaitai/etm_mac.ksy @@ -7,5 +7,7 @@ seq: type: u4 - id: len_encrypted_packet type: u4 + valid: + expr: _ == _io.size - _io.pos - id: encrypted_packet size: len_encrypted_packet diff --git a/protocol/src/main/resources/kaitai/mpint.ksy b/protocol/src/main/resources/kaitai/mpint.ksy index 5e718d3..bd33a0e 100644 --- a/protocol/src/main/resources/kaitai/mpint.ksy +++ b/protocol/src/main/resources/kaitai/mpint.ksy @@ -10,6 +10,8 @@ doc-ref: 'https://tools.ietf.org/html/rfc4251#section-5' seq: - id: len_body type: u4 + valid: + expr: _ <= _io.size - _io.pos - id: body size: len_body instances: diff --git a/protocol/src/main/resources/kaitai/name_list.ksy b/protocol/src/main/resources/kaitai/name_list.ksy index 6a4ba6f..79b59fc 100644 --- a/protocol/src/main/resources/kaitai/name_list.ksy +++ b/protocol/src/main/resources/kaitai/name_list.ksy @@ -13,6 +13,8 @@ doc-ref: 'https://tools.ietf.org/html/rfc4251#section-5' seq: - id: len_entries type: u4 + valid: + expr: _ <= _io.size - _io.pos - id: entries type: name_entry size: len_entries diff --git a/protocol/src/main/resources/kaitai/restrict_destination_constraint.ksy b/protocol/src/main/resources/kaitai/restrict_destination_constraint.ksy index ee98526..ce01066 100644 --- a/protocol/src/main/resources/kaitai/restrict_destination_constraint.ksy +++ b/protocol/src/main/resources/kaitai/restrict_destination_constraint.ksy @@ -13,6 +13,8 @@ seq: doc: Hostname of the previous hop (empty for origin) - id: num_from_keyspecs type: u4 + valid: + expr: _ <= (_io.size - _io.pos) / 5 doc: Number of from host key specs - id: from_keyspecs type: keyspec @@ -27,6 +29,8 @@ seq: doc: Destination hostname - id: num_to_hostspecs type: u4 + valid: + expr: _ <= (_io.size - _io.pos) / 5 doc: Number of destination host key specs - id: to_hostspecs type: keyspec diff --git a/protocol/src/main/resources/kaitai/ssh_agent_identities_answer.ksy b/protocol/src/main/resources/kaitai/ssh_agent_identities_answer.ksy index 6a914e0..b9b9083 100644 --- a/protocol/src/main/resources/kaitai/ssh_agent_identities_answer.ksy +++ b/protocol/src/main/resources/kaitai/ssh_agent_identities_answer.ksy @@ -10,6 +10,8 @@ doc: | seq: - id: nkeys type: u4 + valid: + expr: _ <= (_io.size - _io.pos) / 8 doc: Number of keys in the response - id: identities type: identity diff --git a/protocol/src/main/resources/kaitai/ssh_agent_message.ksy b/protocol/src/main/resources/kaitai/ssh_agent_message.ksy index 64cf935..4ec4d66 100644 --- a/protocol/src/main/resources/kaitai/ssh_agent_message.ksy +++ b/protocol/src/main/resources/kaitai/ssh_agent_message.ksy @@ -12,6 +12,8 @@ doc: | seq: - id: length type: u4 + valid: + expr: _ >= 1 and _ == _io.size - _io.pos doc: Length of message_type + payload - id: message_type type: u1 diff --git a/protocol/src/main/resources/kaitai/ssh_msg_ext_info.ksy b/protocol/src/main/resources/kaitai/ssh_msg_ext_info.ksy index 86396fe..b0d2b60 100644 --- a/protocol/src/main/resources/kaitai/ssh_msg_ext_info.ksy +++ b/protocol/src/main/resources/kaitai/ssh_msg_ext_info.ksy @@ -11,6 +11,8 @@ doc: > seq: - id: num_extensions type: u4 + valid: + expr: _ <= (_io.size - _io.pos) / 8 doc: Number of extension name-value pairs - id: extensions type: extension diff --git a/protocol/src/main/resources/kaitai/ssh_msg_userauth_info_request.ksy b/protocol/src/main/resources/kaitai/ssh_msg_userauth_info_request.ksy index 1c67e81..aeb43f6 100644 --- a/protocol/src/main/resources/kaitai/ssh_msg_userauth_info_request.ksy +++ b/protocol/src/main/resources/kaitai/ssh_msg_userauth_info_request.ksy @@ -13,6 +13,8 @@ seq: type: byte_string - id: num_prompts type: u4 + valid: + expr: _ <= (_io.size - _io.pos) / 5 - id: prompts type: prompt repeat: expr diff --git a/protocol/src/main/resources/kaitai/ssh_msg_userauth_info_response.ksy b/protocol/src/main/resources/kaitai/ssh_msg_userauth_info_response.ksy index 28bc846..184f6ac 100644 --- a/protocol/src/main/resources/kaitai/ssh_msg_userauth_info_response.ksy +++ b/protocol/src/main/resources/kaitai/ssh_msg_userauth_info_response.ksy @@ -7,6 +7,8 @@ doc-ref: RFC 4256 section 3.4 seq: - id: num_responses type: u4 + valid: + expr: _ <= (_io.size - _io.pos) / 4 - id: responses type: byte_string repeat: expr diff --git a/protocol/src/main/resources/kaitai/ssh_public_key.ksy b/protocol/src/main/resources/kaitai/ssh_public_key.ksy index 6efa86f..945fd25 100644 --- a/protocol/src/main/resources/kaitai/ssh_public_key.ksy +++ b/protocol/src/main/resources/kaitai/ssh_public_key.ksy @@ -17,6 +17,8 @@ doc: > seq: - id: algorithm_name_len type: u4 + valid: + expr: _ <= _io.size - _io.pos - id: algorithm_name type: str size: algorithm_name_len diff --git a/protocol/src/main/resources/kaitai/ssh_signature.ksy b/protocol/src/main/resources/kaitai/ssh_signature.ksy index 522408b..381ea33 100644 --- a/protocol/src/main/resources/kaitai/ssh_signature.ksy +++ b/protocol/src/main/resources/kaitai/ssh_signature.ksy @@ -17,6 +17,8 @@ doc: > seq: - id: algorithm_name_len type: u4 + valid: + expr: _ <= _io.size - _io.pos - id: algorithm_name type: str size: algorithm_name_len diff --git a/protocol/src/main/resources/kaitai/unencrypted_packet.ksy b/protocol/src/main/resources/kaitai/unencrypted_packet.ksy index 533ae27..06ccc06 100644 --- a/protocol/src/main/resources/kaitai/unencrypted_packet.ksy +++ b/protocol/src/main/resources/kaitai/unencrypted_packet.ksy @@ -16,8 +16,12 @@ meta: seq: - id: len_packet type: u4 + valid: + expr: _ >= 6 and _ == _io.size - _io.pos - id: len_random_padding type: u1 + valid: + expr: _ >= 4 and _ <= len_packet - 2 - id: payload type: unencrypted_payload size: len_packet - len_random_padding - 1 diff --git a/protocol/src/main/resources/kaitai/userauth_request_gssapi_with_mic.ksy b/protocol/src/main/resources/kaitai/userauth_request_gssapi_with_mic.ksy index 8021e4f..05fa267 100644 --- a/protocol/src/main/resources/kaitai/userauth_request_gssapi_with_mic.ksy +++ b/protocol/src/main/resources/kaitai/userauth_request_gssapi_with_mic.ksy @@ -7,6 +7,8 @@ doc-ref: RFC 4462 section 3.2 seq: - id: num_mechanisms type: u4 + valid: + expr: _ <= (_io.size - _io.pos) / 4 doc: The number of mechanism OIDs client supports - id: mechanisms type: byte_string diff --git a/protocol/src/main/resources/kaitai/utf8_string.ksy b/protocol/src/main/resources/kaitai/utf8_string.ksy index e241c92..5787a29 100644 --- a/protocol/src/main/resources/kaitai/utf8_string.ksy +++ b/protocol/src/main/resources/kaitai/utf8_string.ksy @@ -13,6 +13,8 @@ doc-ref: 'https://tools.ietf.org/html/rfc4251#section-5' seq: - id: len type: u4 + valid: + expr: _ <= _io.size - _io.pos - id: value type: str size: len diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/KaitaiParsing.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/KaitaiParsing.kt new file mode 100644 index 0000000..d22a5a5 --- /dev/null +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/KaitaiParsing.kt @@ -0,0 +1,34 @@ +/* + * ConnectBot SSH Library + * Copyright 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 + +import io.kaitai.struct.KaitaiStream +import java.nio.BufferUnderflowException + +internal fun Throwable.kaitaiParseFailureOrNull(): Throwable? { + var current: Throwable? = this + while (current != null) { + if (current is KaitaiStream.KaitaiStructError || + current is BufferUnderflowException + ) { + return current + } + current = current.cause + } + return null +} 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 3179e2a..b36e9b0 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentProtocolHandler.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/AgentProtocolHandler.kt @@ -26,6 +26,7 @@ import org.connectbot.sshlib.AgentProvider import org.connectbot.sshlib.AgentSigningContext import org.connectbot.sshlib.DestinationConstraint import org.connectbot.sshlib.crypto.SignatureVerifier +import org.connectbot.sshlib.kaitaiParseFailureOrNull import org.connectbot.sshlib.protocol.SshAgentIdentitiesAnswer import org.connectbot.sshlib.protocol.SshAgentMessage import org.connectbot.sshlib.protocol.SshAgentSignResponse @@ -157,24 +158,32 @@ internal class AgentProtocolHandler( suspend fun handleRequest(requestBytes: ByteArray): ByteArray { logger.debug("Handling agent request (${requestBytes.size} bytes)") - val stream = ByteBufferKaitaiStream(requestBytes) - val message = SshAgentMessage(stream) - message._read() + val message = try { + parseAgentMessage(requestBytes) + } catch (e: MalformedAgentRequestException) { + logger.warn("Malformed SSH agent request", e.cause) + return createFailureResponse() + } val messageType = message.messageType() logger.debug("Agent message type: $messageType") - return when (messageType) { - SSH_AGENTC_REQUEST_IDENTITIES -> handleRequestIdentities() + return try { + when (messageType) { + SSH_AGENTC_REQUEST_IDENTITIES -> handleRequestIdentities() - SSH_AGENTC_SIGN_REQUEST -> handleSignRequest(message) + SSH_AGENTC_SIGN_REQUEST -> handleSignRequest(message) - SSH_AGENTC_EXTENSION -> handleExtension(message) + SSH_AGENTC_EXTENSION -> handleExtension(message) - else -> { - logger.warn("Unknown agent message type: $messageType") - createFailureResponse() + else -> { + logger.warn("Unknown agent message type: $messageType") + createFailureResponse() + } } + } catch (e: MalformedAgentRequestException) { + logger.warn("Malformed SSH agent request", e.cause) + return createFailureResponse() } } @@ -340,16 +349,34 @@ internal class AgentProtocolHandler( return createSuccessResponse() } - private inline fun parsePayload(message: SshAgentMessage): T { + private fun parseAgentMessage(requestBytes: ByteArray): SshAgentMessage = try { + val stream = ByteBufferKaitaiStream(requestBytes) + val message = SshAgentMessage(stream) + message._read() + message + } catch (e: RuntimeException) { + throwMalformedAgentRequest(e) + } + + private inline fun parsePayload(message: SshAgentMessage): T = try { val stream = ByteBufferKaitaiStream(message._raw_payload()) val payload = T::class.java.getConstructor(KaitaiStream::class.java).newInstance(stream) payload._read() - return payload + payload + } catch (e: Exception) { + throwMalformedAgentRequest(e) } private fun createFailureResponse(): ByteArray = buildAgentMessage(SSH_AGENT_FAILURE, ByteArray(0)) private fun createSuccessResponse(): ByteArray = buildAgentMessage(SSH_AGENT_SUCCESS, ByteArray(0)) + + private fun throwMalformedAgentRequest(e: Exception): Nothing { + val parseFailure = e.kaitaiParseFailureOrNull() ?: throw e + throw MalformedAgentRequestException(parseFailure) + } + + private class MalformedAgentRequestException(cause: Throwable) : Exception(cause) } internal data class AgentSessionInfo( 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 1c73aff..38cc78b 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SshConnection.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/SshConnection.kt @@ -68,6 +68,7 @@ import org.connectbot.sshlib.crypto.SignatureEntry import org.connectbot.sshlib.crypto.SignatureVerifier import org.connectbot.sshlib.crypto.SshPrivateKey import org.connectbot.sshlib.crypto.SshPublicKeyEncoder +import org.connectbot.sshlib.kaitaiParseFailureOrNull import org.connectbot.sshlib.protocol.AsciiString import org.connectbot.sshlib.protocol.ChannelOpenDirectTcpip import org.connectbot.sshlib.protocol.ChannelOpenForwardedTcpip @@ -130,6 +131,7 @@ import org.connectbot.sshlib.protocol.createUtf8String import org.connectbot.sshlib.protocol.toByteArray import org.connectbot.sshlib.transport.PacketIO import org.connectbot.sshlib.transport.Transport +import org.connectbot.sshlib.transport.TransportException import org.slf4j.LoggerFactory import java.math.BigInteger import java.nio.ByteBuffer @@ -2545,15 +2547,21 @@ class SshConnection( } catch (_: CancellationException) { logger.debug("Packet loop cancelled") } catch (e: Exception) { - pendingConnect?.completeExceptionally(e) + val packetFailure = e.kaitaiParseFailureOrNull() + val loopFailure = when { + e is SshException -> e + packetFailure != null -> TransportException("Malformed SSH packet", packetFailure) + else -> e + } + pendingConnect?.completeExceptionally(loopFailure) pendingConnect = null val allClosed = channels.values.all { !it.isOpen } if (allClosed) { logger.debug("Packet loop ended (all channels closed)") } else { - logger.warn("Packet loop ended unexpectedly", e) + logger.warn("Packet loop ended unexpectedly", loopFailure) } - loopException = e + loopException = loopFailure } finally { for (ch in channels.values) { ch.onClose() 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 219bcde..e0538b6 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/transport/PacketIO.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/transport/PacketIO.kt @@ -22,6 +22,7 @@ import org.connectbot.sshlib.crypto.PacketAead import org.connectbot.sshlib.crypto.PacketCipher import org.connectbot.sshlib.crypto.PacketCompressor import org.connectbot.sshlib.crypto.PacketMac +import org.connectbot.sshlib.kaitaiParseFailureOrNull import org.connectbot.sshlib.protocol.IdBanner import org.connectbot.sshlib.protocol.UnencryptedPacket import org.slf4j.LoggerFactory @@ -216,18 +217,23 @@ internal class PacketIO(private val transport: Transport) { * can parse them with proper parent context for body length calculation. */ private fun parsePayloadBytes(payloadBytes: ByteArray): UnencryptedPacket.UnencryptedPayload { - val paddingLength = 4 - val packetLength = 1 + payloadBytes.size + paddingLength - val buffer = ByteArrayOutputStream() - buffer.write(ByteBuffer.allocate(4).putInt(packetLength).array()) - buffer.write(paddingLength) - buffer.write(payloadBytes) - buffer.write(ByteArray(paddingLength)) - val fullPacket = buffer.toByteArray() - val stream = ByteBufferKaitaiStream(fullPacket) - val packet = UnencryptedPacket(stream) - packet._read() - return packet.payload() + try { + val paddingLength = 4 + val packetLength = 1 + payloadBytes.size + paddingLength + val buffer = ByteArrayOutputStream() + buffer.write(ByteBuffer.allocate(4).putInt(packetLength).array()) + buffer.write(paddingLength) + buffer.write(payloadBytes) + buffer.write(ByteArray(paddingLength)) + val fullPacket = buffer.toByteArray() + val stream = ByteBufferKaitaiStream(fullPacket) + val packet = UnencryptedPacket(stream) + packet._read() + return packet.payload() + } catch (e: RuntimeException) { + val parseFailure = e.kaitaiParseFailureOrNull() ?: throw e + throw TransportException("Malformed SSH packet payload", parseFailure) + } } /** diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/AgentProtocolTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/AgentProtocolTest.kt index dfc8ce1..7779c74 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/AgentProtocolTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/AgentProtocolTest.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 import io.kaitai.struct.ByteBufferKaitaiStream +import io.kaitai.struct.KaitaiStream import kotlinx.coroutines.test.runTest import nl.jqno.equalsverifier.EqualsVerifier import org.connectbot.sshlib.client.AgentProtocolHandler @@ -40,9 +41,12 @@ 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.assertNotNull +import org.junit.jupiter.api.Assertions.assertSame import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.Test +import java.nio.BufferUnderflowException import java.nio.ByteBuffer +import kotlin.test.assertFailsWith class AgentProtocolTest { @@ -108,6 +112,113 @@ class AgentProtocolTest { assertEquals(0, payload.flags().toInt()) } + @Test + fun `rejects agent frame length larger than containing channel data`() { + val request = byteArrayOf( + 0x40, + 0x00, + 0x00, + 0x00, + 11, + ) + + assertFailsWith { + SshAgentMessage(ByteBufferKaitaiStream(request))._read() + } + } + + @Test + fun `rejects sign request byte string length larger than agent payload`() { + val request = byteArrayOf( + 0x00, + 0x00, + 0x00, + 0x05, + 13, + 0x40, + 0x00, + 0x00, + 0x00, + ) + + assertFailsWith { + SshAgentMessage(ByteBufferKaitaiStream(request))._read() + } + } + + @Test + fun `handler returns failure for malformed agent frame`() = runTest { + val handler = AgentProtocolHandler( + provider = object : AgentProvider { + override suspend fun getIdentities(): List = emptyList() + + override suspend fun signData(context: AgentSigningContext): ByteArray? = null + }, + sessionInfo = AgentSessionInfo(ByteArray(0), ByteArray(0)), + ) + val request = byteArrayOf( + 0x40, + 0x00, + 0x00, + 0x00, + 11, + ) + + val response = handler.handleRequest(request) + + val (messageType, payload) = parseAgentMessage(response) + assertEquals(5, messageType) + assertEquals(0, payload.size) + } + + @Test + fun `handler returns failure for malformed sign request payload`() = runTest { + val handler = AgentProtocolHandler( + provider = object : AgentProvider { + override suspend fun getIdentities(): List = emptyList() + + override suspend fun signData(context: AgentSigningContext): ByteArray? = null + }, + sessionInfo = AgentSessionInfo(ByteArray(0), ByteArray(0)), + ) + val request = byteArrayOf( + 0x00, + 0x00, + 0x00, + 0x05, + 13, + 0x40, + 0x00, + 0x00, + 0x00, + ) + + val response = handler.handleRequest(request) + + val (messageType, payload) = parseAgentMessage(response) + assertEquals(5, messageType) + assertEquals(0, payload.size) + } + + @Test + fun `handler does not mistake provider exception for malformed input`() = runTest { + val providerFailure = BufferUnderflowException() + val handler = AgentProtocolHandler( + provider = object : AgentProvider { + override suspend fun getIdentities(): List = throw providerFailure + + override suspend fun signData(context: AgentSigningContext): ByteArray? = null + }, + sessionInfo = AgentSessionInfo(ByteArray(0), ByteArray(0)), + ) + + val error = assertFailsWith { + handler.handleRequest(buildAgentMessage(11, ByteArray(0))) + } + + assertSame(providerFailure, error) + } + @Test fun `handler returns identities answer`() = runTest { val testProvider = object : AgentProvider { diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/SshClientTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/SshClientTest.kt index a708a30..03f839f 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/SshClientTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/SshClientTest.kt @@ -23,13 +23,16 @@ import io.mockk.coVerify import io.mockk.every import io.mockk.mockk import io.mockk.verify +import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.runTest import nl.jqno.equalsverifier.EqualsVerifier import org.connectbot.sshlib.client.ForwardingChannel import org.connectbot.sshlib.client.SessionChannel import org.connectbot.sshlib.client.SshConnection import org.connectbot.sshlib.crypto.SshPrivateKey +import org.connectbot.sshlib.transport.ByteArrayTransport import org.connectbot.sshlib.transport.Transport +import org.connectbot.sshlib.transport.TransportException import org.connectbot.sshlib.transport.TransportFactory import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Test @@ -96,6 +99,31 @@ class SshClientTest { assertIs(result) } + @Test + fun `connect returns ProtocolError for malformed server packet`() { + runBlocking { + val malformedIgnore = byteArrayOf( + 0, 0, 0, 14, + 4, + 2, + 0x40, 0, 0, 0, + 0, 0, 0, 0, + 0, 0, 0, 0, + ) + val serverInput = "SSH-2.0-TestServer\r\n".toByteArray() + malformedIgnore + val config = SshClientConfig { + transportFactory = TransportFactory { ByteArrayTransport(serverInput) } + hostKeyVerifier = acceptAllVerifier + } + + val result = SshClient(config).connect() + + val error = assertIs(result) + assertEquals("Malformed SSH packet payload", error.message) + assertIs(error.cause) + } + } + @Test fun `host factory overload accepts host verifier`() { SshClient("example.com", hostKeyVerifier = acceptAllVerifier) diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/protocol/KaitaiLengthValidationTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/protocol/KaitaiLengthValidationTest.kt new file mode 100644 index 0000000..47a55b8 --- /dev/null +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/protocol/KaitaiLengthValidationTest.kt @@ -0,0 +1,161 @@ +/* + * ConnectBot SSH Library + * Copyright 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.protocol + +import io.kaitai.struct.ByteBufferKaitaiStream +import io.kaitai.struct.KaitaiStream +import org.junit.jupiter.api.Test +import kotlin.test.assertContentEquals +import kotlin.test.assertFailsWith + +class KaitaiLengthValidationTest { + private val hugeLength = byteArrayOf(0x40, 0x00, 0x00, 0x00) + private val hugeCount = byteArrayOf(0x7F, 0xFF.toByte(), 0xFF.toByte(), 0xFF.toByte()) + + @Test + fun `ascii string rejects length larger than containing stream`() { + assertValidationFailure { AsciiString(ByteBufferKaitaiStream(hugeLength))._read() } + } + + @Test + fun `utf8 string rejects length larger than containing stream`() { + assertValidationFailure { Utf8String(ByteBufferKaitaiStream(hugeLength))._read() } + } + + @Test + fun `mpint rejects length larger than containing stream`() { + assertValidationFailure { Mpint(ByteBufferKaitaiStream(hugeLength))._read() } + } + + @Test + fun `name list rejects length larger than containing stream`() { + assertValidationFailure { NameList(ByteBufferKaitaiStream(hugeLength))._read() } + } + + @Test + fun `public key rejects algorithm length larger than containing stream`() { + assertValidationFailure { SshPublicKey(ByteBufferKaitaiStream(hugeLength))._read() } + } + + @Test + fun `signature rejects algorithm length larger than containing stream`() { + assertValidationFailure { SshSignature(ByteBufferKaitaiStream(hugeLength))._read() } + } + + @Test + fun `ecdsa signature rejects blob length larger than containing stream`() { + assertValidationFailure { EcdsaSignatureBlob(ByteBufferKaitaiStream(hugeLength))._read() } + } + + @Test + fun `etm mac rejects packet length larger than containing stream`() { + val data = byteArrayOf(0, 0, 0, 0) + hugeLength + assertValidationFailure { EtmMac(ByteBufferKaitaiStream(data))._read() } + } + + @Test + fun `encrypted packet rejects payload length larger than containing stream`() { + assertValidationFailure { EncryptedPacket(ByteBufferKaitaiStream(hugeLength), 32)._read() } + } + + @Test + fun `encrypted packet allows another packet in containing stream`() { + val firstPayload = byteArrayOf(1, 2, 3, 4) + val secondPayload = byteArrayOf(5, 6, 7, 8) + val mac = ByteArray(4) + val data = lengthPrefix(firstPayload.size) + firstPayload + mac + + lengthPrefix(secondPayload.size) + secondPayload + mac + val stream = ByteBufferKaitaiStream(data) + + val first = EncryptedPacket(stream, mac.size.toLong()).also { it._read() } + val second = EncryptedPacket(stream, mac.size.toLong()).also { it._read() } + + assertContentEquals(firstPayload, first.encryptedPayload()) + assertContentEquals(secondPayload, second.encryptedPayload()) + } + + @Test + fun `unencrypted packet rejects packet length larger than containing stream`() { + assertValidationFailure { UnencryptedPacket(ByteBufferKaitaiStream(hugeLength))._read() } + } + + @Test + fun `unencrypted packet rejects padding that leaves no message type`() { + val data = byteArrayOf( + 0, + 0, + 0, + 6, + 5, + 0, + 0, + 0, + 0, + 0, + ) + assertValidationFailure { UnencryptedPacket(ByteBufferKaitaiStream(data))._read() } + } + + @Test + fun `ext info rejects impossible extension count`() { + assertValidationFailure { SshMsgExtInfo(ByteBufferKaitaiStream(hugeCount))._read() } + } + + @Test + fun `keyboard interactive request rejects impossible prompt count`() { + val data = emptyByteString() + emptyByteString() + emptyByteString() + hugeCount + assertValidationFailure { SshMsgUserauthInfoRequest(ByteBufferKaitaiStream(data))._read() } + } + + @Test + fun `keyboard interactive response rejects impossible response count`() { + assertValidationFailure { SshMsgUserauthInfoResponse(ByteBufferKaitaiStream(hugeCount))._read() } + } + + @Test + fun `gssapi request rejects impossible mechanism count`() { + assertValidationFailure { UserauthRequestGssapiWithMic(ByteBufferKaitaiStream(hugeCount))._read() } + } + + @Test + fun `agent identities rejects impossible key count`() { + assertValidationFailure { SshAgentIdentitiesAnswer(ByteBufferKaitaiStream(hugeCount))._read() } + } + + @Test + fun `destination constraint rejects impossible from keyspec count`() { + val data = emptyByteString() + hugeCount + assertValidationFailure { RestrictDestinationConstraint(ByteBufferKaitaiStream(data))._read() } + } + + private fun emptyByteString(): ByteArray = byteArrayOf(0, 0, 0, 0) + + private fun lengthPrefix(length: Int): ByteArray = byteArrayOf( + (length ushr 24).toByte(), + (length ushr 16).toByte(), + (length ushr 8).toByte(), + length.toByte(), + ) + + private fun assertValidationFailure(block: () -> Unit) { + val error = assertFailsWith { block() } + check(error is KaitaiStream.ValidationExprError) { + "Expected ValidationExprError but got ${error::class.qualifiedName}" + } + } +} 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 ff16217..3212672 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/transport/PacketIOTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/transport/PacketIOTest.kt @@ -112,6 +112,26 @@ class PacketIOTest { } } + @Test + fun `readPacket wraps Kaitai validation failure as TransportException`() { + val malformedIgnore = byteArrayOf( + 0, 0, 0, 14, + 4, + SshEnums.MessageType.SSH_MSG_IGNORE.id().toByte(), + 0x40, 0, 0, 0, + 0, 0, 0, 0, + 0, 0, 0, 0, + ) + val io = PacketIO(ByteArrayTransport(malformedIgnore)) + + val error = assertThrows(TransportException::class.java) { + runBlocking { io.readPacket() } + } + + assertEquals("Malformed SSH packet payload", error.message) + assertTrue(error.cause is io.kaitai.struct.KaitaiStream.ValidationExprError) + } + @Test fun `reset sequence numbers works`() = runBlocking { val transport = ByteArrayTransport()