From b25012b37bb33a8d0a59388aad6b32e43ce87225 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Sun, 1 Feb 2026 17:55:09 +0100 Subject: [PATCH] ssh: enforce nil Permissions when returning PartialSuccessError In serverAuthenticate, the permissions variable is reset to nil at the beginning of the authentication loop. If an authentication callback returns a PartialSuccessError along with non-nil Permissions, those permissions are currently silently discarded before the next authentication step. This change returns an error if a callback returns both a PartialSuccessError and non-nil Permissions, preventing API misuse where the user might erroneously expect those permissions to be preserved or merged into the final session permissions. This issue was found during a security audit by NCC Group Cryptography Services, sponsored by Teleport. Fixes golang/go#79562 Fixes CVE-2026-39828 Change-Id: I632c9e46e2b5e8804ef88081063a3612a2462f9f Reviewed-on: https://go-review.googlesource.com/c/crypto/+/781621 Reviewed-by: Neal Patel LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Auto-Submit: Gopher Robot Reviewed-by: Roland Shoemaker --- ssh/keys_test.go | 24 ++++++++++----------- ssh/server.go | 13 +++++++++-- ssh/server_test.go | 54 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 14 deletions(-) diff --git a/ssh/keys_test.go b/ssh/keys_test.go index a1165ec..e1412bb 100644 --- a/ssh/keys_test.go +++ b/ssh/keys_test.go @@ -272,18 +272,18 @@ func TestParseEncryptedPrivateKeysWithPassphrase(t *testing.T) { } func TestParseEncryptedPrivateKeysWithUnsupportedCiphers(t *testing.T) { - for _, tt := range testdata.UnsupportedCipherData { - t.Run(tt.Name, func(t *testing.T){ - _, err := ParsePrivateKeyWithPassphrase(tt.PEMBytes, []byte(tt.EncryptionKey)) - if err == nil { - t.Fatalf("expected 'unknown cipher' error for %q, got nil", tt.Name) - // If this cipher is now supported, remove it from testdata.UnsupportedCipherData - } - if !strings.Contains(err.Error(), "unknown cipher") { - t.Errorf("wanted 'unknown cipher' error, got %v", err.Error()) - } - }) - } + for _, tt := range testdata.UnsupportedCipherData { + t.Run(tt.Name, func(t *testing.T) { + _, err := ParsePrivateKeyWithPassphrase(tt.PEMBytes, []byte(tt.EncryptionKey)) + if err == nil { + t.Fatalf("expected 'unknown cipher' error for %q, got nil", tt.Name) + // If this cipher is now supported, remove it from testdata.UnsupportedCipherData + } + if !strings.Contains(err.Error(), "unknown cipher") { + t.Errorf("wanted 'unknown cipher' error, got %v", err.Error()) + } + }) + } } func TestParseEncryptedPrivateKeysWithIncorrectPassphrase(t *testing.T) { diff --git a/ssh/server.go b/ssh/server.go index 064dcba..7f7ecf9 100644 --- a/ssh/server.go +++ b/ssh/server.go @@ -242,8 +242,10 @@ func (c *pubKeyCache) add(candidate cachedPubKey) { type ServerConn struct { Conn - // If the succeeding authentication callback returned a - // non-nil Permissions pointer, it is stored here. + // If the succeeding authentication callback returned a non-nil Permissions + // pointer, it is stored here. These are the permissions from the final, + // successful authentication method. Permissions returned by callbacks that + // return PartialSuccessError are not preserved and must be nil. Permissions *Permissions } @@ -824,6 +826,13 @@ userAuthLoop: var failureMsg userAuthFailureMsg if partialSuccess, ok := authErr.(*PartialSuccessError); ok { + // Permissions are not preserved between authentication steps. To + // avoid confusion about the final state of the connection, we + // disallow returning non-nil Permissions combined with + // PartialSuccessError. + if perms != nil { + return nil, errors.New("ssh: permissions must be nil when returning PartialSuccessError") + } // After a partial success error we don't allow changing the user // name and execute the NoClientAuthCallback. partialSuccessReturned = true diff --git a/ssh/server_test.go b/ssh/server_test.go index e186193..9b514ad 100644 --- a/ssh/server_test.go +++ b/ssh/server_test.go @@ -808,6 +808,60 @@ func TestVerifiedPublicKeyCallbackOnly(t *testing.T) { <-done } +func TestPartialSuccessWithNonNilPerms(t *testing.T) { + c1, c2, err := netPipe() + if err != nil { + t.Fatalf("netPipe: %v", err) + } + defer c1.Close() + defer c2.Close() + + serverConf := &ServerConfig{ + PublicKeyCallback: func(conn ConnMetadata, key PublicKey) (*Permissions, error) { + if bytes.Equal(key.Marshal(), testPublicKeys["rsa"].Marshal()) { + return nil, nil + } + return nil, errors.New("invalid credentials") + }, + VerifiedPublicKeyCallback: func(conn ConnMetadata, key PublicKey, permissions *Permissions, signatureAlgorithm string) (*Permissions, error) { + if bytes.Equal(key.Marshal(), testPublicKeys["rsa"].Marshal()) { + // Intentionally return non-nil Permissions along with a + // PartialSuccessError. Since permissions are reset between + // authentication steps, this constitutes invalid library usage + // and the server is expected to reject the connection. + return &Permissions{Extensions: map[string]string{"permit-port-forwarding": ""}}, &PartialSuccessError{ + Next: ServerAuthCallbacks{ + PasswordCallback: func(conn ConnMetadata, password []byte) (*Permissions, error) { + if string(password) == clientPassword { + return nil, nil + } + return nil, nil + }, + }, + } + } + return nil, errors.New("invalid credentials") + }, + } + serverConf.AddHostKey(testSigners["rsa"]) + + clientConf := ClientConfig{ + User: "user", + Auth: []AuthMethod{ + PublicKeys(testSigners["rsa"]), + Password(clientPassword), + }, + HostKeyCallback: InsecureIgnoreHostKey(), + } + + go NewServerConn(c1, serverConf) + + _, _, _, err = NewClientConn(c2, "", &clientConf) + if err == nil { + t.Fatal("authentication succeeded unexpectedly; server should have rejected non-nil Permissions combined with PartialSuccessError") + } +} + type markerConn struct { closed uint32 used uint32