fix: Prevent premature authentication on failed ssh key challege

Many thanks to András Veres-Szentkirályi for the report and the support
in understanding and finding the issue.

Author: flaix

releases.moxie

@@ -6,6 +6,8 @@ r34: {
     id: ${project.version}
     date: ${project.buildDate}
     note: ''
+          This release fixes a vulnerability allowing an attacker to circumvent authentication on the SSH transport. Users are urged to update to this version.
+
           Should you have disabled the Flash-based copy-to-clipboard function because it wasn't working anymore (`web.allowFlashCopyToClipboard = false`), you may want to rethink this and enable it again. The configuration property has the same name, but the mechanism was exchanged. Flash is gone, and a modern JavaScript solution is now used to copy text directly to the clipboard (via clipboard.js).
 
           The setting `server.requireClientCertificates` now has three values: `required`, `optional` and `none`. While `required` is synonymous to the old `true` value, and `optional` is synonymous to the old `false` value, the new `none` value results in the server never asking the client to present any client certificate at all. The old values `true` and `false` can still be used and keep their meaning.
@@ -18,6 +20,7 @@ r34: {
           Highlights:
 
           * Support for ECDSA and Ed25519 SSH keys
+          * Fix vulnerability that allowed SSH authentication to be circumvented
           * Explicitly disable requesting optional client TLS certificates
           * Copy-to-clipboard button is back and working
           * Minimal required Java version is Java 8
@@ -30,6 +33,7 @@ r34: {
           ''
     security:
       - Fix path traversal vulnerability which allowed access to "/resources//../WEB-INF/". (CVE-2022-31268) This was fixed by updating Jetty. (issue-1409)
+      - Fix exploit circumventing SSH authentication. Many thanks to András Veres-Szentkirályi (silentsignal.eu) for the report. (CVE-2024-28080)
     fixes:
       - Fix crash in Gitblit Authority when users were deleted from Gitblit but still had entries (certificates) in the Authority. (issue-1359, pr-1435)
       - Fix tab-to-space conversion to work like tabs. (pr-1065 by @QuentinC)
@@ -92,6 +96,7 @@ r34: {
       - Tino Desjardins
       - @xxl-cc
       - Egor Shchegolkov
+      - András Veres-Szentkirályi
 }
 
 #

src/main/java/com/gitblit/transport/ssh/SshKrbAuthenticator.java

@@ -54,10 +54,7 @@ public SshKrbAuthenticator(IStoredSettings settings, IAuthenticationManager auth
 	public boolean validateIdentity(ServerSession session, String identity) {
 		log.info("identify with kerberos {}", identity);
 		SshDaemonClient client = session.getAttribute(SshDaemonClient.KEY);
-		if (client.getUser() != null) {
-			log.info("{} has already authenticated!", identity);
-			return true;
-		}
+
 		String username = identity.toLowerCase(Locale.US);
 		if (stripDomain) {
 			int p = username.indexOf('@');
@@ -67,6 +64,7 @@ public boolean validateIdentity(ServerSession session, String identity) {
 		}
 		UserModel user = authManager.authenticate(username);
 		if (user != null) {
+// TODO: Check if the user was set in the client and if it is the same as this user. Do not allow changing the user during the SSH auth process.
 			client.setUser(user);
 			return true;
 		}

src/main/java/com/gitblit/transport/ssh/UsernamePasswordAuthenticator.java

@@ -45,14 +45,11 @@ public UsernamePasswordAuthenticator(IAuthenticationManager authManager) {
 	@Override
 	public boolean authenticate(String username, String password, ServerSession session) {
 		SshDaemonClient client = session.getAttribute(SshDaemonClient.KEY);
-		if (client.getUser() != null) {
-			log.info("{} has already authenticated!", username);
-			return true;
-		}
 
 		username = username.toLowerCase(Locale.US);
 		UserModel user = authManager.authenticate(username, password.toCharArray(), null);
 		if (user != null) {
+// TODO: Check if the user was set in the client and if it is the same as this user. Do not allow changing the user during the SSH auth process.
 			client.setUser(user);
 			return true;
 		}

src/test/java/com/gitblit/tests/SshDaemonTest.java

@@ -16,10 +16,12 @@
 package com.gitblit.tests;
 
 import java.io.File;
+import java.security.KeyPair;
 import java.text.MessageFormat;
 import java.util.List;
 
 import org.apache.sshd.client.SshClient;
+import org.apache.sshd.client.future.AuthFuture;
 import org.apache.sshd.client.session.ClientSession;
 import org.eclipse.jgit.api.CloneCommand;
 import org.eclipse.jgit.api.Git;
@@ -41,12 +43,73 @@ public class SshDaemonTest extends SshUnitTest {
 
 	String url = GitBlitSuite.sshDaemonUrl;
 
+	@Test
+	public void testPasswordAuthentication() throws Exception {
+		SshClient client = getClient();
+		ClientSession session = client.connect(username, "localhost", GitBlitSuite.sshPort).verify().getSession();
+
+		session.addPasswordIdentity(password);
+		AuthFuture authFuture = session.auth();
+		assertTrue(authFuture.await());
+		assertTrue(authFuture.isSuccess());
+	}
+
 	@Test
 	public void testPublicKeyAuthentication() throws Exception {
 		SshClient client = getClient();
 		ClientSession session = client.connect(username, "localhost", GitBlitSuite.sshPort).verify().getSession();
+
 		session.addPublicKeyIdentity(rwKeyPair);
-		assertTrue(session.auth().await());
+		AuthFuture authFuture = session.auth();
+		assertTrue(authFuture.await());
+		assertTrue(authFuture.isSuccess());
+	}
+
+	@Test
+	public void testWrongPublicKeyAuthentication() throws Exception {
+		SshClient client = getClient();
+		ClientSession session = client.connect(username, "localhost", GitBlitSuite.sshPort).verify().getSession();
+		KeyPair attackKeyPair = generator.generateKeyPair();
+
+		session.addPublicKeyIdentity(attackKeyPair);
+		AuthFuture authFuture = session.auth();
+		assertTrue(authFuture.await());
+		assertFalse(authFuture.isSuccess());
+	}
+
+	@Test
+	public void testWrongPublicKeyThenPasswordAuthentication() throws Exception {
+		SshClient client = getClient();
+		ClientSession session = client.connect(username, "localhost", GitBlitSuite.sshPort).verify().getSession();
+		KeyPair otherKeyPair = generator.generateKeyPair();
+
+		session.addPublicKeyIdentity(otherKeyPair);
+		AuthFuture authFuture = session.auth();
+		assertTrue(authFuture.await());
+		assertFalse(authFuture.isSuccess());
+
+		session.addPasswordIdentity(password);
+		authFuture = session.auth();
+		assertTrue(authFuture.await());
+		assertTrue(authFuture.isSuccess());
+	}
+
+	@Test
+	public void testWrongPublicKeyThenWrongPasswordAuthentication() throws Exception {
+		SshClient client = getClient();
+		ClientSession session = client.connect(username, "localhost", GitBlitSuite.sshPort).verify().getSession();
+		KeyPair otherKeyPair = generator.generateKeyPair();
+		KeyPair attackKeyPair = new KeyPair(rwKeyPair.getPublic(), otherKeyPair.getPrivate());
+
+		session.addPublicKeyIdentity(attackKeyPair);
+		AuthFuture authFuture = session.auth();
+		assertTrue(authFuture.await());
+		assertFalse(authFuture.isSuccess());
+
+		session.addPasswordIdentity("nothing");
+		authFuture = session.auth();
+		assertTrue(authFuture.await());
+		assertFalse(authFuture.isSuccess());
 	}
 
 	@Test