Skip to content

Commit 17402aa

Browse files
nindanaotoclaude
authored andcommitted
Add tests proving host key rotation crash (connectbot/connectbot#2023)
When the server sends hostkeys-00@openssh.com after auth, the code compares stored algorithm names (e.g. "rsa-sha2-512") against key blob algorithm identifiers ("ssh-rsa"). This mismatch causes removeServerHostKey to be called with a null hostKey parameter, which crashes Kotlin callers (like ConnectBot) that declare the parameter as non-nullable ByteArray. These tests demonstrate: 1. removeServerHostKey is called with null due to RSA algo name mismatch 2. The resulting NPE propagates out of handleMessage uncaught 3. Baseline: matching algorithm names cause no removal Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 8bc03ec commit 17402aa

1 file changed

Lines changed: 144 additions & 0 deletions

File tree

src/test/java/com/trilead/ssh2/channel/ChannelManagerTest.java

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,25 @@
11
package com.trilead.ssh2.channel;
22

33
import com.trilead.ssh2.ChannelCondition;
4+
import com.trilead.ssh2.ConnectionInfo;
45
import com.trilead.ssh2.ExtendedServerHostKeyVerifier;
56
import com.trilead.ssh2.packets.PacketGlobalHostkeys;
67
import com.trilead.ssh2.packets.Packets;
78
import com.trilead.ssh2.packets.TypesWriter;
9+
import com.trilead.ssh2.signature.RSASHA1Verify;
10+
import com.trilead.ssh2.signature.RSASHA512Verify;
811
import com.trilead.ssh2.transport.ITransportConnection;
912
import org.junit.jupiter.api.BeforeEach;
1013
import org.junit.jupiter.api.Test;
1114
import org.junit.jupiter.api.extension.ExtendWith;
15+
import org.mockito.ArgumentCaptor;
1216
import org.mockito.Mock;
1317
import org.mockito.junit.jupiter.MockitoExtension;
1418

1519
import java.io.IOException;
1620
import java.util.Arrays;
21+
import java.util.Collections;
22+
import java.util.List;
1723

1824
import static org.junit.jupiter.api.Assertions.assertEquals;
1925
import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -22,9 +28,13 @@
2228
import static org.junit.jupiter.api.Assertions.assertTrue;
2329
import static org.junit.jupiter.api.Assertions.fail;
2430
import static org.mockito.Mockito.any;
31+
import static org.mockito.Mockito.anyInt;
32+
import static org.mockito.Mockito.anyString;
2533
import static org.mockito.Mockito.atLeastOnce;
34+
import static org.mockito.Mockito.doThrow;
2635
import static org.mockito.Mockito.mock;
2736
import static org.mockito.Mockito.never;
37+
import static org.mockito.Mockito.nullable;
2838
import static org.mockito.Mockito.times;
2939
import static org.mockito.Mockito.verify;
3040
import static org.mockito.Mockito.when;
@@ -778,4 +788,138 @@ public void testMsgChannelOpenWithUnknownType() throws IOException {
778788
channelManager.msgChannelOpen(msg, offset);
779789
verify(mockTransportConnection).sendAsynchronousMessage(any(byte[].class));
780790
}
791+
792+
// ---- Host key rotation tests ----
793+
794+
/**
795+
* Build an SSH key blob whose algorithm identifier is the given string.
796+
* The blob is: uint32(len) + algorithm_name + uint32(len) + dummy_data.
797+
* This is enough for extractKeyAlgorithm() which only reads the first string.
798+
*/
799+
private byte[] buildKeyBlob(String algorithm) {
800+
TypesWriter tw = new TypesWriter();
801+
tw.writeString(algorithm);
802+
// Append a dummy "key data" field so it looks like a plausible key blob
803+
tw.writeString(new byte[]{0x00, 0x01, 0x02, 0x03}, 0, 4);
804+
return tw.getBytes();
805+
}
806+
807+
/**
808+
* Build an SSH_MSG_GLOBAL_REQUEST message for hostkeys-00@openssh.com
809+
* containing the given host key blobs.
810+
*/
811+
private byte[] buildHostkeysGlobalRequest(String requestName, boolean wantReply, byte[]... keyBlobs) {
812+
TypesWriter tw = new TypesWriter();
813+
tw.writeByte(Packets.SSH_MSG_GLOBAL_REQUEST);
814+
tw.writeString(requestName);
815+
tw.writeBoolean(wantReply);
816+
for (byte[] blob : keyBlobs) {
817+
tw.writeString(blob, 0, blob.length);
818+
}
819+
return tw.getBytes();
820+
}
821+
822+
/**
823+
* Simulates the scenario from GitHub issue connectbot/connectbot#2023:
824+
*
825+
* 1. ConnectBot stores the host key algorithm as "rsa-sha2-512" (negotiated algo).
826+
* 2. Server sends hostkeys-00@openssh.com advertising its RSA key blob
827+
* which contains "ssh-rsa" as the key format identifier.
828+
* 3. processHostkeysAdvertisement sees "rsa-sha2-512" NOT in the advertised set
829+
* {"ssh-rsa"} and calls removeServerHostKey(hostname, port, "rsa-sha2-512", null).
830+
* 4. A Kotlin implementation (like ConnectBot's) declares hostKey as non-nullable
831+
* ByteArray, so passing null throws NullPointerException, crashing the app.
832+
*
833+
* This test verifies the bug: removeServerHostKey is called with null hostKey.
834+
*/
835+
@Test
836+
public void testHostkeysAdvertisement_rsaAlgoMismatch_callsRemoveWithNull() throws Exception {
837+
// Set up an ExtendedServerHostKeyVerifier that reports "rsa-sha2-512" as known
838+
ExtendedServerHostKeyVerifier mockVerifier = mock(ExtendedServerHostKeyVerifier.class);
839+
when(mockVerifier.getKnownKeyAlgorithmsForHost(anyString(), anyInt()))
840+
.thenReturn(Collections.singletonList(RSASHA512Verify.ID_RSA_SHA_2_512));
841+
842+
// Mock getConnectionInfo so requestHostkeysProve doesn't NPE on ConnectionInfo
843+
ConnectionInfo connInfo = new ConnectionInfo();
844+
connInfo.serverHostKeyAlgorithm = RSASHA512Verify.ID_RSA_SHA_2_512;
845+
when(mockTransportConnection.getConnectionInfo(anyInt())).thenReturn(connInfo);
846+
847+
when(mockTransportConnection.getServerHostKeyVerifier()).thenReturn(mockVerifier);
848+
when(mockTransportConnection.getHostname()).thenReturn("esxi.example.com");
849+
when(mockTransportConnection.getPort()).thenReturn(22);
850+
851+
// Server advertises an RSA key blob (algorithm in blob = "ssh-rsa")
852+
byte[] rsaKeyBlob = buildKeyBlob(RSASHA1Verify.ID_SSH_RSA);
853+
byte[] msg = buildHostkeysGlobalRequest(
854+
"hostkeys-00@openssh.com", false, rsaKeyBlob);
855+
856+
channelManager.handleMessage(msg, msg.length);
857+
858+
// BUG: removeServerHostKey is called with null hostKey because
859+
// "rsa-sha2-512" (known) is not in {"ssh-rsa"} (advertised)
860+
ArgumentCaptor<byte[]> hostKeyCaptor = ArgumentCaptor.forClass(byte[].class);
861+
verify(mockVerifier).removeServerHostKey(
862+
anyString(), anyInt(), anyString(), hostKeyCaptor.capture());
863+
864+
// This proves the bug: the hostKey argument is null
865+
assertNull(hostKeyCaptor.getValue(),
866+
"removeServerHostKey should NOT be called with null hostKey, " +
867+
"but currently it is due to RSA algorithm name mismatch");
868+
}
869+
870+
/**
871+
* Simulates the crash: if removeServerHostKey throws when called with null
872+
* (as Kotlin's non-nullable ByteArray check does), the exception propagates
873+
* uncaught through handleMessage, killing the SSH receiver thread and
874+
* crashing the app.
875+
*/
876+
@Test
877+
public void testHostkeysAdvertisement_rsaAlgoMismatch_crashesReceiverThread() throws Exception {
878+
// Set up an ExtendedServerHostKeyVerifier that throws NPE on null hostKey
879+
// (simulating Kotlin's non-nullable parameter check)
880+
ExtendedServerHostKeyVerifier mockVerifier = mock(ExtendedServerHostKeyVerifier.class);
881+
when(mockVerifier.getKnownKeyAlgorithmsForHost(anyString(), anyInt()))
882+
.thenReturn(Collections.singletonList(RSASHA512Verify.ID_RSA_SHA_2_512));
883+
doThrow(new NullPointerException("Parameter specified as non-null is null: parameter hostKey"))
884+
.when(mockVerifier).removeServerHostKey(anyString(), anyInt(), anyString(), nullable(byte[].class));
885+
886+
when(mockTransportConnection.getServerHostKeyVerifier()).thenReturn(mockVerifier);
887+
when(mockTransportConnection.getHostname()).thenReturn("esxi.example.com");
888+
when(mockTransportConnection.getPort()).thenReturn(22);
889+
890+
byte[] rsaKeyBlob = buildKeyBlob(RSASHA1Verify.ID_SSH_RSA);
891+
byte[] msg = buildHostkeysGlobalRequest(
892+
"hostkeys-00@openssh.com", false, rsaKeyBlob);
893+
894+
// The NPE propagates out of handleMessage — this kills the receiver thread
895+
// and crashes the Android app
896+
assertThrows(NullPointerException.class, () ->
897+
channelManager.handleMessage(msg, msg.length));
898+
}
899+
900+
/**
901+
* Verify that when the stored algorithm matches the advertised key blob
902+
* algorithm (both "ssh-rsa"), removeServerHostKey is NOT called.
903+
* This is the baseline: no mismatch, no problem.
904+
*/
905+
@Test
906+
public void testHostkeysAdvertisement_matchingAlgo_noRemoval() throws Exception {
907+
ExtendedServerHostKeyVerifier mockVerifier = mock(ExtendedServerHostKeyVerifier.class);
908+
when(mockVerifier.getKnownKeyAlgorithmsForHost(anyString(), anyInt()))
909+
.thenReturn(Collections.singletonList(RSASHA1Verify.ID_SSH_RSA));
910+
911+
when(mockTransportConnection.getServerHostKeyVerifier()).thenReturn(mockVerifier);
912+
when(mockTransportConnection.getHostname()).thenReturn("esxi.example.com");
913+
when(mockTransportConnection.getPort()).thenReturn(22);
914+
915+
byte[] rsaKeyBlob = buildKeyBlob(RSASHA1Verify.ID_SSH_RSA);
916+
byte[] msg = buildHostkeysGlobalRequest(
917+
"hostkeys-00@openssh.com", false, rsaKeyBlob);
918+
919+
channelManager.handleMessage(msg, msg.length);
920+
921+
// No algorithm mismatch, so removeServerHostKey should not be called
922+
verify(mockVerifier, never()).removeServerHostKey(
923+
anyString(), anyInt(), anyString(), any());
924+
}
781925
}

0 commit comments

Comments
 (0)