From 5adb68b3e6894a642e0f86712fcad2cd3e19c4e9 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Sun, 24 May 2026 19:06:18 +0200 Subject: [PATCH] ssh: cap total userauth attempts per server connection serverAuthenticate only bounded real failures via MaxAuthTries. PartialSuccessError responses and the publickey isQuery short-circuit both kept the loop running without incrementing authFailures, so a client could keep the server processing SSH_MSG_USERAUTH_REQUEST messages indefinitely on a single connection. Add an unconditional cap, maxAuthServerAttempts = 128, on the total number of userauth requests handled per connection. The counter is incremented at the top of the loop before method dispatch, so every method and every isQuery / partial-success path counts. When the cap is exceeded the server sends SSH_MSG_DISCONNECT with reason 2 ("too many authentication attempts"), mirroring the MaxAuthTries handling. The bound is well below OpenSSH's hard cap of 1024 but above any realistic multi-step auth flow. Change-Id: I56779fc55cd00ddfd32ec938f8de3a49be0145dc Reviewed-on: https://go-review.googlesource.com/c/crypto/+/781903 Reviewed-by: Junyang Shao Reviewed-by: David Chase LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Filippo Valsorda --- ssh/server.go | 23 +++++++++++++++++++++++ ssh/server_test.go | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/ssh/server.go b/ssh/server.go index 0192a67..9292f0b 100644 --- a/ssh/server.go +++ b/ssh/server.go @@ -607,6 +607,15 @@ func (b *BannerError) Error() string { return b.Err.Error() } +// maxAuthServerAttempts caps the total number of SSH_MSG_USERAUTH_REQUEST +// messages the server will process on a single connection, regardless of +// outcome (failure, partial success, public key query, or none). It is a +// backstop against clients that drive the authentication loop indefinitely +// without ever incurring a real failure — for example by repeatedly +// triggering PartialSuccessError or by spamming public key offer queries — +// neither of which increment the MaxAuthTries failure counter. +const maxAuthServerAttempts = 128 + func (s *connection) serverAuthenticate(config *ServerConfig) (*Permissions, error) { if config.PreAuthConnCallback != nil { config.PreAuthConnCallback(s) @@ -617,6 +626,7 @@ func (s *connection) serverAuthenticate(config *ServerConfig) (*Permissions, err var perms *Permissions authFailures := 0 + authAttempts := 0 noneAuthCount := 0 var authErrs []error var calledBannerCallback bool @@ -645,6 +655,19 @@ userAuthLoop: return nil, &ServerAuthError{Errors: authErrs} } + if authAttempts >= maxAuthServerAttempts { + discMsg := &disconnectMsg{ + Reason: 2, + Message: "too many authentication attempts", + } + if err := s.transport.writePacket(Marshal(discMsg)); err != nil { + return nil, err + } + authErrs = append(authErrs, discMsg) + return nil, &ServerAuthError{Errors: authErrs} + } + authAttempts++ + var userAuthReq userAuthRequestMsg if packet, err := s.transport.readPacket(); err != nil { if err == io.EOF { diff --git a/ssh/server_test.go b/ssh/server_test.go index 502a25b..01e262b 100644 --- a/ssh/server_test.go +++ b/ssh/server_test.go @@ -166,6 +166,52 @@ func TestMaxAuthTriesNoneMethod(t *testing.T) { } } +func TestMaxAuthServerAttempts(t *testing.T) { + c1, c2, err := netPipe() + if err != nil { + t.Fatalf("netPipe: %v", err) + } + defer c1.Close() + defer c2.Close() + + invocations := 0 + clientConf := &ClientConfig{ + User: "user", + HostKeyCallback: InsecureIgnoreHostKey(), + AuthCallback: func(ctx *ClientAuthContext) (AuthMethod, error) { + invocations++ + return PublicKeys(testSigners["rsa"]), nil + }, + } + + go NewServerConn(c1, alwaysPartialPubKeyServer()) + + _, _, _, err = NewClientConn(c2, "", clientConf) + if err == nil { + t.Fatal("expected the server to disconnect after exceeding the attempts cap") + } + // The error must be the server's disconnect, not the client-side + // "too many authentication attempts (N), aborting" bound. The + // disconnect format is set by disconnectMsg.Error(). + if !strings.Contains(err.Error(), "ssh: disconnect") || + !strings.Contains(err.Error(), "too many authentication attempts") { + t.Fatalf("expected server disconnect, got: %v", err) + } + // Server-side request sequence per AuthCallback iteration: + // 1 "none" (initial, before any AuthCallback) + // 2 pubkey query (AuthCallback #k, server returns OK) + // 3 pubkey signed (AuthCallback #k, server returns partial) + // So after k complete callbacks the server has processed 1+2k requests. + // The (k+1)-th callback's query is processed as request number 2(k+1); + // the subsequent signed request is rejected by the cap check + // (authAttempts >= maxAuthServerAttempts), so 2(k+1) = maxAuthServerAttempts + // and invocations = maxAuthServerAttempts / 2. + expected := maxAuthServerAttempts / 2 + if invocations != expected { + t.Errorf("AuthCallback invoked %d times; want %d", invocations, expected) + } +} + func TestMaxAuthTriesFirstNoneAuthErrorIgnored(t *testing.T) { username := "testuser" serverConfig := &ServerConfig{