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/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/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 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()