Fix host key rotation crash with RSA algorithm name mismatch

nindanaoto · claude · kruton · commit 2fff66492b9f · 2026-04-09T19:25:27.000-07:00
Fixes connectbot/connectbot#2023. When the server sends hostkeys-00@openssh.com after auth, processHostkeysAdvertisement compared stored algorithm names (e.g. "rsa-sha2-512") against key blob identifiers ("ssh-rsa"). Since these are the same RSA key with different signature algorithms, the mismatch caused removeServerHostKey to be called with a null hostKey, crashing Kotlin callers. Changes: 1. Add normalizeKeyAlgorithm() to treat rsa-sha2-256/512 as ssh-rsa for key identity comparison. This prevents the false mismatch. 2. Catch Exception (not just IOException) around processHostkeysAdvertisement so RuntimeExceptions from verifier callbacks don't kill the receiver thread. 3. Move hostkeys-prove check before globalSuccessCounter increment in msgGlobalSuccess to prevent counter interference with other global requests. 4. Send REQUEST_SUCCESS (not FAILURE) for hostkeys requests we handle. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
diff --git a/src/main/java/com/trilead/ssh2/channel/ChannelManager.java b/src/main/java/com/trilead/ssh2/channel/ChannelManager.java
@@ -1676,24 +1676,12 @@ public void msgChannelOpenFailure(byte[] msg, int msglen) throws IOException
 
 	public void msgGlobalRequest(byte[] msg, int msglen) throws IOException
 	{
-		/* Currently we do not support any kind of global request */
-
 		TypesReader tr = new TypesReader(msg, 0, msglen);
 
 		tr.readByte(); // skip packet type
 		String requestName = tr.readString();
 		boolean wantReply = tr.readBoolean();
 
-		if (wantReply)
-		{
-			byte[] reply_failure = new byte[1];
-			reply_failure[0] = Packets.SSH_MSG_REQUEST_FAILURE;
-
-			tm.sendAsynchronousMessage(reply_failure);
-		}
-
-		/* We do not clean up the requestName String - that is OK for debug */
-
 		if (log.isEnabled())
 			log.log(80, "Got SSH_MSG_GLOBAL_REQUEST (" + requestName + ")");
 
@@ -1702,20 +1690,33 @@ public void msgGlobalRequest(byte[] msg, int msglen) throws IOException
 			try {
 				PacketGlobalHostkeys hostkeys = new PacketGlobalHostkeys(msg, 0, msglen);
 				processHostkeysAdvertisement(hostkeys, requestName);
-			} catch (IOException e) {
+			} catch (Exception e) {
 				if (log.isEnabled())
-					log.log(20, "Failed to parse hostkeys advertisement: " + e.getMessage());
+					log.log(20, "Failed to process hostkeys advertisement: " + e.getMessage());
 			}
+			// hostkeys-00@openssh.com typically has wantReply=false, but if the
+			// server does request a reply, acknowledge it since we processed it.
+			if (wantReply)
+			{
+				byte[] reply_success = new byte[1];
+				reply_success[0] = Packets.SSH_MSG_REQUEST_SUCCESS;
+				tm.sendAsynchronousMessage(reply_success);
+			}
+			return;
 		}
-	}
 
-	public void msgGlobalSuccess(byte[] msg, int msglen) throws IOException {
-		synchronized (channels)
+		if (wantReply)
 		{
-			globalSuccessCounter++;
-			channels.notifyAll();
+			byte[] reply_failure = new byte[1];
+			reply_failure[0] = Packets.SSH_MSG_REQUEST_FAILURE;
+
+			tm.sendAsynchronousMessage(reply_failure);
 		}
+	}
 
+	public void msgGlobalSuccess(byte[] msg, int msglen) throws IOException {
+		// Check for pending hostkeys-prove BEFORE incrementing the global counter,
+		// so the hostkeys-prove response doesn't interfere with other global request tracking.
 		synchronized (hostkeysProveLock) {
 			if (pendingHostkeysProve != null && !pendingHostkeysProve.completed) {
 				try {
@@ -1739,6 +1740,12 @@ public void msgGlobalSuccess(byte[] msg, int msglen) throws IOException {
 			}
 		}
 
+		synchronized (channels)
+		{
+			globalSuccessCounter++;
+			channels.notifyAll();
+		}
+
 		if (log.isEnabled())
 			log.log(80, "Got SSH_MSG_REQUEST_SUCCESS");
 	}
@@ -1882,19 +1889,28 @@ private void processHostkeysAdvertisement(PacketGlobalHostkeys hostkeys, String
 		}
 
 		List<String> knownAlgos = extVerifier.getKnownKeyAlgorithmsForHost(hostname, port);
-		Set<String> knownAlgoSet = (knownAlgos != null) ? new HashSet<>(knownAlgos) : new HashSet<>();
+
+		// Normalize algorithm names so RSA signature variants (rsa-sha2-256,
+		// rsa-sha2-512) are treated as the same key type as ssh-rsa.
+		Set<String> normalizedKnownAlgoSet = new HashSet<>();
+		if (knownAlgos != null) {
+			for (String algo : knownAlgos) {
+				normalizedKnownAlgoSet.add(normalizeKeyAlgorithm(algo));
+			}
+		}
 
 		List<byte[]> newKeys = new ArrayList<>();
-		Set<String> advertisedAlgoSet = new HashSet<>();
+		Set<String> normalizedAdvertisedAlgoSet = new HashSet<>();
 
 		for (byte[] keyBlob : advertisedKeys) {
 			String keyAlgo = extractKeyAlgorithm(keyBlob);
 			if (keyAlgo == null)
 				continue;
 
-			advertisedAlgoSet.add(keyAlgo);
+			String normalizedAlgo = normalizeKeyAlgorithm(keyAlgo);
+			normalizedAdvertisedAlgoSet.add(normalizedAlgo);
 
-			if (!knownAlgoSet.contains(keyAlgo)) {
+			if (!normalizedKnownAlgoSet.contains(normalizedAlgo)) {
 				newKeys.add(keyBlob);
 			}
 		}
@@ -1903,7 +1919,7 @@ private void processHostkeysAdvertisement(PacketGlobalHostkeys hostkeys, String
 			for (String knownAlgo : knownAlgos) {
 				if (knownAlgo == null)
 					continue;
-				if (!advertisedAlgoSet.contains(knownAlgo)) {
+				if (!normalizedAdvertisedAlgoSet.contains(normalizeKeyAlgorithm(knownAlgo))) {
 					extVerifier.removeServerHostKey(hostname, port, knownAlgo, null);
 					if (log.isEnabled())
 						log.log(50, "Removed hostkey algorithm no longer advertised: " + knownAlgo);
@@ -2050,6 +2066,21 @@ private String extractKeyAlgorithm(byte[] keyBlob) throws IOException
 		return tr.readString();
 	}
 
+	/**
+	 * Normalizes RSA algorithm variants to a canonical form for comparison.
+	 * In SSH, rsa-sha2-256 and rsa-sha2-512 use the same RSA key as ssh-rsa
+	 * (the difference is only the signature hash algorithm). Key blobs always
+	 * identify as ssh-rsa regardless of which signature algorithm was negotiated.
+	 */
+	static String normalizeKeyAlgorithm(String algorithm)
+	{
+		if (RSASHA256Verify.ID_RSA_SHA_2_256.equals(algorithm) ||
+			RSASHA512Verify.ID_RSA_SHA_2_512.equals(algorithm)) {
+			return RSASHA1Verify.ID_SSH_RSA;
+		}
+		return algorithm;
+	}
+
 	private SSHSignature getSignatureVerifier(String algorithm)
 	{
 		if (RSASHA1Verify.ID_SSH_RSA.equals(algorithm))
diff --git a/src/test/java/com/trilead/ssh2/channel/ChannelManagerTest.java b/src/test/java/com/trilead/ssh2/channel/ChannelManagerTest.java
@@ -1,12 +1,12 @@
 package com.trilead.ssh2.channel;
 
 import com.trilead.ssh2.ChannelCondition;
-import com.trilead.ssh2.ConnectionInfo;
 import com.trilead.ssh2.ExtendedServerHostKeyVerifier;
 import com.trilead.ssh2.packets.PacketGlobalHostkeys;
 import com.trilead.ssh2.packets.Packets;
 import com.trilead.ssh2.packets.TypesWriter;
 import com.trilead.ssh2.signature.RSASHA1Verify;
+import com.trilead.ssh2.signature.RSASHA256Verify;
 import com.trilead.ssh2.signature.RSASHA512Verify;
 import com.trilead.ssh2.transport.ITransportConnection;
 import org.junit.jupiter.api.BeforeEach;
@@ -19,7 +19,6 @@
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.List;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -820,81 +819,68 @@ private byte[] buildHostkeysGlobalRequest(String requestName, boolean wantReply,
 	}
 
 	/**
-	 * Simulates the scenario from GitHub issue connectbot/connectbot#2023:
+	 * Regression test for GitHub issue connectbot/connectbot#2023:
 	 *
-	 * 1. ConnectBot stores the host key algorithm as "rsa-sha2-512" (negotiated algo).
-	 * 2. Server sends hostkeys-00@openssh.com advertising its RSA key blob
-	 *    which contains "ssh-rsa" as the key format identifier.
-	 * 3. processHostkeysAdvertisement sees "rsa-sha2-512" NOT in the advertised set
-	 *    {"ssh-rsa"} and calls removeServerHostKey(hostname, port, "rsa-sha2-512", null).
-	 * 4. A Kotlin implementation (like ConnectBot's) declares hostKey as non-nullable
-	 *    ByteArray, so passing null throws NullPointerException, crashing the app.
+	 * Before the fix, when the stored algorithm was "rsa-sha2-512" and the
+	 * key blob contained "ssh-rsa", processHostkeysAdvertisement would call
+	 * removeServerHostKey with null hostKey (crashing Kotlin callers).
 	 *
-	 * This test verifies the bug: removeServerHostKey is called with null hostKey.
+	 * After the fix, RSA algorithm variants are normalized so "rsa-sha2-512"
+	 * and "ssh-rsa" are recognized as the same key type. removeServerHostKey
+	 * should NOT be called, since the key is still present.
 	 */
 	@Test
-	public void testHostkeysAdvertisement_rsaAlgoMismatch_callsRemoveWithNull() throws Exception {
-		// Set up an ExtendedServerHostKeyVerifier that reports "rsa-sha2-512" as known
+	public void testHostkeysAdvertisement_rsaAlgoMismatch_noRemovalAfterFix() throws Exception {
 		ExtendedServerHostKeyVerifier mockVerifier = mock(ExtendedServerHostKeyVerifier.class);
 		when(mockVerifier.getKnownKeyAlgorithmsForHost(anyString(), anyInt()))
 			.thenReturn(Collections.singletonList(RSASHA512Verify.ID_RSA_SHA_2_512));
 
-		// Mock getConnectionInfo so requestHostkeysProve doesn't NPE on ConnectionInfo
-		ConnectionInfo connInfo = new ConnectionInfo();
-		connInfo.serverHostKeyAlgorithm = RSASHA512Verify.ID_RSA_SHA_2_512;
-		when(mockTransportConnection.getConnectionInfo(anyInt())).thenReturn(connInfo);
-
 		when(mockTransportConnection.getServerHostKeyVerifier()).thenReturn(mockVerifier);
 		when(mockTransportConnection.getHostname()).thenReturn("esxi.example.com");
 		when(mockTransportConnection.getPort()).thenReturn(22);
 
-		// Server advertises an RSA key blob (algorithm in blob = "ssh-rsa")
 		byte[] rsaKeyBlob = buildKeyBlob(RSASHA1Verify.ID_SSH_RSA);
 		byte[] msg = buildHostkeysGlobalRequest(
 			"hostkeys-00@openssh.com", false, rsaKeyBlob);
 
 		channelManager.handleMessage(msg, msg.length);
 
-		// BUG: removeServerHostKey is called with null hostKey because
-		// "rsa-sha2-512" (known) is not in {"ssh-rsa"} (advertised)
-		ArgumentCaptor<byte[]> hostKeyCaptor = ArgumentCaptor.forClass(byte[].class);
-		verify(mockVerifier).removeServerHostKey(
-			anyString(), anyInt(), anyString(), hostKeyCaptor.capture());
-
-		// This proves the bug: the hostKey argument is null
-		assertNull(hostKeyCaptor.getValue(),
-			"removeServerHostKey should NOT be called with null hostKey, " +
-			"but currently it is due to RSA algorithm name mismatch");
+		// After fix: rsa-sha2-512 is normalized to ssh-rsa, so the algorithm
+		// is recognized as still advertised. removeServerHostKey must NOT be called.
+		verify(mockVerifier, never()).removeServerHostKey(
+			anyString(), anyInt(), anyString(), any());
 	}
 
 	/**
-	 * Simulates the crash: if removeServerHostKey throws when called with null
-	 * (as Kotlin's non-nullable ByteArray check does), the exception propagates
-	 * uncaught through handleMessage, killing the SSH receiver thread and
-	 * crashing the app.
+	 * Regression test: even if removeServerHostKey throws (e.g., Kotlin's
+	 * non-nullable parameter check), it must not propagate out of handleMessage
+	 * and kill the SSH receiver thread.
 	 */
 	@Test
-	public void testHostkeysAdvertisement_rsaAlgoMismatch_crashesReceiverThread() throws Exception {
-		// Set up an ExtendedServerHostKeyVerifier that throws NPE on null hostKey
-		// (simulating Kotlin's non-nullable parameter check)
+	public void testHostkeysAdvertisement_removeThrows_doesNotCrashReceiverThread() throws Exception {
+		// Use a non-RSA algorithm so normalization doesn't prevent the removal call.
+		// Pretend the client knows "ssh-dss" but server no longer advertises it.
 		ExtendedServerHostKeyVerifier mockVerifier = mock(ExtendedServerHostKeyVerifier.class);
 		when(mockVerifier.getKnownKeyAlgorithmsForHost(anyString(), anyInt()))
-			.thenReturn(Collections.singletonList(RSASHA512Verify.ID_RSA_SHA_2_512));
+			.thenReturn(Collections.singletonList("ssh-dss"));
 		doThrow(new NullPointerException("Parameter specified as non-null is null: parameter hostKey"))
 			.when(mockVerifier).removeServerHostKey(anyString(), anyInt(), anyString(), nullable(byte[].class));
 
 		when(mockTransportConnection.getServerHostKeyVerifier()).thenReturn(mockVerifier);
 		when(mockTransportConnection.getHostname()).thenReturn("esxi.example.com");
 		when(mockTransportConnection.getPort()).thenReturn(22);
 
-		byte[] rsaKeyBlob = buildKeyBlob(RSASHA1Verify.ID_SSH_RSA);
+		// Server advertises only an ed25519 key (no DSS)
+		byte[] ed25519KeyBlob = buildKeyBlob("ssh-ed25519");
 		byte[] msg = buildHostkeysGlobalRequest(
-			"hostkeys-00@openssh.com", false, rsaKeyBlob);
+			"hostkeys-00@openssh.com", false, ed25519KeyBlob);
+
+		// Even if removeServerHostKey throws, handleMessage must NOT propagate it
+		channelManager.handleMessage(msg, msg.length);
 
-		// The NPE propagates out of handleMessage — this kills the receiver thread
-		// and crashes the Android app
-		assertThrows(NullPointerException.class, () ->
-			channelManager.handleMessage(msg, msg.length));
+		// The call did happen (and threw), but the exception was caught
+		verify(mockVerifier).removeServerHostKey(
+			anyString(), anyInt(), anyString(), nullable(byte[].class));
 	}
 
 	/**
@@ -922,4 +908,57 @@ public void testHostkeysAdvertisement_matchingAlgo_noRemoval() throws Exception
 		verify(mockVerifier, never()).removeServerHostKey(
 			anyString(), anyInt(), anyString(), any());
 	}
+
+	// ---- normalizeKeyAlgorithm tests ----
+
+	@Test
+	public void testNormalizeKeyAlgorithm_rsaSha2_512() {
+		assertEquals(RSASHA1Verify.ID_SSH_RSA,
+			ChannelManager.normalizeKeyAlgorithm(RSASHA512Verify.ID_RSA_SHA_2_512));
+	}
+
+	@Test
+	public void testNormalizeKeyAlgorithm_rsaSha2_256() {
+		assertEquals(RSASHA1Verify.ID_SSH_RSA,
+			ChannelManager.normalizeKeyAlgorithm(RSASHA256Verify.ID_RSA_SHA_2_256));
+	}
+
+	@Test
+	public void testNormalizeKeyAlgorithm_sshRsa_unchanged() {
+		assertEquals(RSASHA1Verify.ID_SSH_RSA,
+			ChannelManager.normalizeKeyAlgorithm(RSASHA1Verify.ID_SSH_RSA));
+	}
+
+	@Test
+	public void testNormalizeKeyAlgorithm_nonRsa_unchanged() {
+		assertEquals("ssh-ed25519",
+			ChannelManager.normalizeKeyAlgorithm("ssh-ed25519"));
+		assertEquals("ssh-dss",
+			ChannelManager.normalizeKeyAlgorithm("ssh-dss"));
+		assertEquals("ecdsa-sha2-nistp256",
+			ChannelManager.normalizeKeyAlgorithm("ecdsa-sha2-nistp256"));
+	}
+
+	// ---- msgGlobalRequest hostkeys reply tests ----
+
+	@Test
+	public void testMsgGlobalRequest_hostkeys_withReply_sendsSuccess() throws Exception {
+		ExtendedServerHostKeyVerifier mockVerifier = mock(ExtendedServerHostKeyVerifier.class);
+		when(mockVerifier.getKnownKeyAlgorithmsForHost(anyString(), anyInt())).thenReturn(null);
+		when(mockTransportConnection.getServerHostKeyVerifier()).thenReturn(mockVerifier);
+		when(mockTransportConnection.getHostname()).thenReturn("example.com");
+		when(mockTransportConnection.getPort()).thenReturn(22);
+
+		byte[] rsaKeyBlob = buildKeyBlob(RSASHA1Verify.ID_SSH_RSA);
+		byte[] msg = buildHostkeysGlobalRequest(
+			"hostkeys-00@openssh.com", true, rsaKeyBlob);
+
+		channelManager.handleMessage(msg, msg.length);
+
+		// Should send REQUEST_SUCCESS (not FAILURE) for handled hostkeys request
+		ArgumentCaptor<byte[]> replyCaptor = ArgumentCaptor.forClass(byte[].class);
+		verify(mockTransportConnection).sendAsynchronousMessage(replyCaptor.capture());
+		assertEquals(Packets.SSH_MSG_REQUEST_SUCCESS, replyCaptor.getValue()[0],
+			"Hostkeys global request should get SUCCESS reply, not FAILURE");
+	}
 }
