Skip to content

Commit 3fa1c30

Browse files
krutonclaude
andcommitted
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. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 750ab35 commit 3fa1c30

3 files changed

Lines changed: 77 additions & 5 deletions

File tree

sshlib/src/main/kotlin/org/connectbot/sshlib/transport/PacketIO.kt

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/*
22
* ConnectBot SSH Library
3-
* Copyright 2025 Kenny Root
3+
* Copyright 2025-2026 Kenny Root
44
*
55
* Licensed under the Apache License, Version 2.0 (the "License");
66
* you may not use this file except in compliance with the License.
@@ -27,6 +27,7 @@ import org.connectbot.sshlib.protocol.UnencryptedPacket
2727
import org.slf4j.LoggerFactory
2828
import java.io.ByteArrayOutputStream
2929
import java.nio.ByteBuffer
30+
import java.security.MessageDigest
3031
import kotlin.random.Random
3132

3233
/**
@@ -321,7 +322,7 @@ internal class PacketIO(private val transport: Transport) {
321322

322323
// Verify MAC (over plaintext for encrypt-and-MAC)
323324
val expectedMac = mac.compute(receiveSequenceNumber, decryptedPacket)
324-
if (!receivedMac.contentEquals(expectedMac)) {
325+
if (!MessageDigest.isEqual(receivedMac, expectedMac)) {
325326
logger.error("MAC verification failed for seq=$receiveSequenceNumber")
326327
logger.error(" Received MAC: ${receivedMac.joinToString("") { "%02x".format(it) }}")
327328
logger.error(" Expected MAC: ${expectedMac.joinToString("") { "%02x".format(it) }}")
@@ -361,7 +362,7 @@ internal class PacketIO(private val transport: Transport) {
361362

362363
// Verify MAC (over sequence_number || length || encrypted_payload)
363364
val expectedMac = mac.computeEtm(receiveSequenceNumber, encryptedLength, encryptedPayload)
364-
if (!receivedMac.contentEquals(expectedMac)) {
365+
if (!MessageDigest.isEqual(receivedMac, expectedMac)) {
365366
logger.error("ETM MAC verification failed for seq=$receiveSequenceNumber")
366367
throw TransportException("ETM MAC verification failed")
367368
}

sshlib/src/test/kotlin/org/connectbot/sshlib/transport/PacketIOEtmTest.kt

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/*
22
* ConnectBot SSH Library
3-
* Copyright 2025 Kenny Root
3+
* Copyright 2025-2026 Kenny Root
44
*
55
* Licensed under the Apache License, Version 2.0 (the "License");
66
* you may not use this file except in compliance with the License.
@@ -24,6 +24,7 @@ import org.connectbot.sshlib.crypto.HmacSha256
2424
import org.connectbot.sshlib.crypto.TripleDesCbcCipher
2525
import org.connectbot.sshlib.protocol.SshEnums
2626
import org.junit.jupiter.api.Assertions.assertEquals
27+
import org.junit.jupiter.api.Assertions.assertThrows
2728
import org.junit.jupiter.api.Test
2829

2930
class PacketIOEtmTest {
@@ -173,6 +174,41 @@ class PacketIOEtmTest {
173174
assertEquals(SshEnums.MessageType.SSH_MSG_NEWKEYS, parsed.messageType())
174175
}
175176

177+
@Test
178+
fun `ETM rejects tampered MAC`() {
179+
val (cipherKey, iv, macKey) = createKeyMaterial()
180+
181+
val writeTransport = ByteArrayTransport()
182+
val writeIO = PacketIO(writeTransport)
183+
writeIO.enableEncryption(
184+
clientToServerCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = true),
185+
clientToServerMac = HmacSha256(macKey.copyOf()),
186+
serverToClientCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = false),
187+
serverToClientMac = HmacSha256(macKey.copyOf()),
188+
clientToServerEtm = true,
189+
serverToClientEtm = true,
190+
)
191+
runBlocking { writeIO.writePacket(SshEnums.MessageType.SSH_MSG_NEWKEYS.id().toInt()) }
192+
193+
val wireData = writeTransport.getWrittenData().clone()
194+
wireData[wireData.size - 1] = (wireData[wireData.size - 1].toInt() xor 0xFF).toByte()
195+
196+
val readTransport = ByteArrayTransport(wireData)
197+
val readIO = PacketIO(readTransport)
198+
readIO.enableEncryption(
199+
clientToServerCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = true),
200+
clientToServerMac = HmacSha256(macKey.copyOf()),
201+
serverToClientCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = false),
202+
serverToClientMac = HmacSha256(macKey.copyOf()),
203+
clientToServerEtm = true,
204+
serverToClientEtm = true,
205+
)
206+
207+
assertThrows(TransportException::class.java) {
208+
runBlocking { readIO.readPacket() }
209+
}
210+
}
211+
176212
@Test
177213
fun `ETM multiple packets round trip with CBC`() = runBlocking {
178214
val (cipherKey, iv, macKey) = createKeyMaterial()

sshlib/src/test/kotlin/org/connectbot/sshlib/transport/PacketIOTest.kt

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/*
22
* ConnectBot SSH Library
3-
* Copyright 2025 Kenny Root
3+
* Copyright 2025-2026 Kenny Root
44
*
55
* Licensed under the Apache License, Version 2.0 (the "License");
66
* you may not use this file except in compliance with the License.
@@ -18,6 +18,8 @@
1818
package org.connectbot.sshlib.transport
1919

2020
import kotlinx.coroutines.runBlocking
21+
import org.connectbot.sshlib.crypto.AesCbcCipher
22+
import org.connectbot.sshlib.crypto.HmacSha256
2123
import org.connectbot.sshlib.protocol.SshEnums
2224
import org.junit.jupiter.api.Assertions.assertEquals
2325
import org.junit.jupiter.api.Assertions.assertThrows
@@ -169,6 +171,39 @@ class PacketIOTest {
169171
assertEquals(0L, io.bytesReceivedOnWire)
170172
}
171173

174+
@Test
175+
fun `encrypt-and-MAC rejects tampered MAC`() {
176+
val cipherKey = ByteArray(16) { it.toByte() }
177+
val iv = ByteArray(16) { (it + 0x10).toByte() }
178+
val macKey = ByteArray(32) { (it + 0x20).toByte() }
179+
180+
val writeTransport = ByteArrayTransport()
181+
val writeIO = PacketIO(writeTransport)
182+
writeIO.enableEncryption(
183+
clientToServerCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = true),
184+
clientToServerMac = HmacSha256(macKey.copyOf()),
185+
serverToClientCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = false),
186+
serverToClientMac = HmacSha256(macKey.copyOf()),
187+
)
188+
runBlocking { writeIO.writePacket(SshEnums.MessageType.SSH_MSG_NEWKEYS.id().toInt()) }
189+
190+
val wireData = writeTransport.getWrittenData().clone()
191+
wireData[wireData.size - 1] = (wireData[wireData.size - 1].toInt() xor 0xFF).toByte()
192+
193+
val readTransport = ByteArrayTransport(wireData)
194+
val readIO = PacketIO(readTransport)
195+
readIO.enableEncryption(
196+
clientToServerCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = true),
197+
clientToServerMac = HmacSha256(macKey.copyOf()),
198+
serverToClientCipher = AesCbcCipher(cipherKey, iv.copyOf(), forEncryption = false),
199+
serverToClientMac = HmacSha256(macKey.copyOf()),
200+
)
201+
202+
assertThrows(TransportException::class.java) {
203+
runBlocking { readIO.readPacket() }
204+
}
205+
}
206+
172207
// calculatePaddingLength: totalLength = 4 + 1 + (1 + payload.size) = 6 + payload.size.
173208
// With blockSize=8: raw = if (totalLength%8==0) 8 else 8-(totalLength%8). Bump by 8 if raw<4.
174209
// Verifying the exact padding_length byte at wire[4] kills MathMutator and ConditionalsBoundaryMutator survivors.

0 commit comments

Comments
 (0)