From 8539f9ad39918b67d612a35785a2b4326efc8741 Mon Sep 17 00:00:00 2001 From: Ayman Bagabas Date: Wed, 21 Jan 2026 16:50:37 -0500 Subject: [PATCH] fix: authentication bypass --- pkg/ssh/middleware.go | 35 +++- pkg/ssh/middleware_test.go | 180 ++++++++++++++++++ pkg/ssh/ssh.go | 20 +- testscript/script_test.go | 28 ++- .../testdata/auth-bypass-regression.txtar | 63 ++++++ 5 files changed, 303 insertions(+), 23 deletions(-) create mode 100644 pkg/ssh/middleware_test.go create mode 100644 testscript/testdata/auth-bypass-regression.txtar diff --git a/pkg/ssh/middleware.go b/pkg/ssh/middleware.go index 62f19044b2de02955e2dc366e189eff1edfdd0f4..25a00b1a26754eae77b359bee82a68d178848884 100644 --- a/pkg/ssh/middleware.go +++ b/pkg/ssh/middleware.go @@ -2,6 +2,7 @@ package ssh import ( "fmt" + "strconv" "time" "charm.land/log/v2" @@ -30,23 +31,43 @@ func AuthenticationMiddleware(sh ssh.Handler) ssh.Handler { // validate the authentication. We need to verify that the _last_ key // that was approved is the one that's being used. + ctx := s.Context() + be := backend.FromContext(ctx) + + var pkFp string + perms := s.Permissions().Permissions pk := s.PublicKey() if pk != nil { // There is no public key stored in the context, public-key auth // was never requested, skip - perms := s.Permissions().Permissions if perms == nil { wish.Fatalln(s, ErrPermissionDenied) return } - // Check if the key is the same as the one we have in context - fp := perms.Extensions["pubkey-fp"] - if fp == "" || fp != gossh.FingerprintSHA256(pk) { - wish.Fatalln(s, ErrPermissionDenied) - return - } + pkFp = gossh.FingerprintSHA256(pk) + } + + // Check if the key is the same as the one we have in context + fp := perms.Extensions["pubkey-fp"] + if fp != "" && fp != pkFp { + wish.Fatalln(s, ErrPermissionDenied) + return + } + + ac := be.AllowKeyless(ctx) + publicKeyCounter.WithLabelValues(strconv.FormatBool(ac || pk != nil)).Inc() + if !ac && pk == nil { + wish.Fatalln(s, ErrPermissionDenied) + return + } + + // Set the auth'd user, or anon, in the context + var user proto.User + if pk != nil { + user, _ = be.UserByPublicKey(ctx, pk) } + ctx.SetValue(proto.ContextKeyUser, user) sh(s) } diff --git a/pkg/ssh/middleware_test.go b/pkg/ssh/middleware_test.go new file mode 100644 index 0000000000000000000000000000000000000000..149fd8d8bc9ca16b8e703f1729753965d519c0c8 --- /dev/null +++ b/pkg/ssh/middleware_test.go @@ -0,0 +1,180 @@ +package ssh + +import ( + "context" + "net" + "testing" + + "github.com/charmbracelet/keygen" + "github.com/charmbracelet/soft-serve/pkg/backend" + "github.com/charmbracelet/soft-serve/pkg/config" + "github.com/charmbracelet/soft-serve/pkg/db" + "github.com/charmbracelet/soft-serve/pkg/db/migrate" + "github.com/charmbracelet/soft-serve/pkg/proto" + "github.com/charmbracelet/soft-serve/pkg/store" + "github.com/charmbracelet/soft-serve/pkg/store/database" + "github.com/charmbracelet/ssh" + "github.com/matryer/is" + gossh "golang.org/x/crypto/ssh" + _ "modernc.org/sqlite" +) + +// TestAuthenticationBypass tests for CVE-TBD: Authentication Bypass Vulnerability +// +// VULNERABILITY: +// A critical authentication bypass allows an attacker to impersonate any user +// (including Admin) by "offering" the victim's public key during the SSH handshake +// before authenticating with their own valid key. This occurs because the user +// identity is stored in the session context during the "offer" phase in +// PublicKeyHandler and is not properly cleared/validated in AuthenticationMiddleware. +// +// This test verifies that: +// 1. User context is properly set based on the AUTHENTICATED key, not offered keys +// 2. User context from failed authentication attempts is not preserved +// 3. Non-admin users cannot gain admin privileges through this attack +func TestAuthenticationBypass(t *testing.T) { + is := is.New(t) + ctx := context.Background() + + // Setup temporary database + dp := t.TempDir() + cfg := config.DefaultConfig() + cfg.DataPath = dp + cfg.DB.Driver = "sqlite" + cfg.DB.DataSource = dp + "/test.db" + + ctx = config.WithContext(ctx, cfg) + dbx, err := db.Open(ctx, cfg.DB.Driver, cfg.DB.DataSource) + is.NoErr(err) + defer dbx.Close() + + is.NoErr(migrate.Migrate(ctx, dbx)) + dbstore := database.New(ctx, dbx) + ctx = store.WithContext(ctx, dbstore) + be := backend.New(ctx, cfg, dbx, dbstore) + ctx = backend.WithContext(ctx, be) + + // Generate keys for admin and attacker + adminKeyPath := dp + "/admin_key" + adminPair, err := keygen.New(adminKeyPath, keygen.WithKeyType(keygen.Ed25519), keygen.WithWrite()) + is.NoErr(err) + + attackerKeyPath := dp + "/attacker_key" + attackerPair, err := keygen.New(attackerKeyPath, keygen.WithKeyType(keygen.Ed25519), keygen.WithWrite()) + is.NoErr(err) + + // Parse public keys + adminPubKey, _, _, _, err := gossh.ParseAuthorizedKey([]byte(adminPair.AuthorizedKey())) + is.NoErr(err) + + attackerPubKey, _, _, _, err := gossh.ParseAuthorizedKey([]byte(attackerPair.AuthorizedKey())) + is.NoErr(err) + + // Create admin user + adminUser, err := be.CreateUser(ctx, "testadmin", proto.UserOptions{ + Admin: true, + PublicKeys: []gossh.PublicKey{adminPubKey}, + }) + is.NoErr(err) + is.True(adminUser != nil) + + // Create attacker (non-admin) user + attackerUser, err := be.CreateUser(ctx, "testattacker", proto.UserOptions{ + Admin: false, + PublicKeys: []gossh.PublicKey{attackerPubKey}, + }) + is.NoErr(err) + is.True(attackerUser != nil) + is.True(!attackerUser.IsAdmin()) // Verify attacker is NOT admin + + // Test: Verify that looking up user by key gives correct user + t.Run("user_lookup_by_key", func(t *testing.T) { + is := is.New(t) + + // Looking up admin key should return admin user + user, err := be.UserByPublicKey(ctx, adminPubKey) + is.NoErr(err) + is.Equal(user.Username(), "testadmin") + is.True(user.IsAdmin()) + + // Looking up attacker key should return attacker user + user, err = be.UserByPublicKey(ctx, attackerPubKey) + is.NoErr(err) + is.Equal(user.Username(), "testattacker") + is.True(!user.IsAdmin()) + }) + + // Test: Simulate the authentication bypass vulnerability + // This test documents the EXPECTED behavior to prevent regression + t.Run("authentication_bypass_simulation", func(t *testing.T) { + is := is.New(t) + + // Create a mock context + mockCtx := &mockSSHContext{ + Context: ctx, + values: make(map[any]any), + permissions: &ssh.Permissions{Permissions: &gossh.Permissions{Extensions: make(map[string]string)}}, + } + + // ATTACK SIMULATION: + // Step 1: SSH client offers admin's public key + // PublicKeyHandler is called and sets admin user in context + mockCtx.SetValue(proto.ContextKeyUser, adminUser) + mockCtx.permissions.Extensions["pubkey-fp"] = gossh.FingerprintSHA256(adminPubKey) + + // Step 2: Signature verification FAILS (attacker doesn't have admin's private key) + // SSH protocol continues to next key... + + // Step 3: SSH client offers attacker's key (which SUCCEEDS) + // PublicKeyHandler is called again, fingerprint is updated + mockCtx.permissions.Extensions["pubkey-fp"] = gossh.FingerprintSHA256(attackerPubKey) + // BUG: Admin user is STILL in context from step 1! + + // Step 4: AuthenticationMiddleware should re-lookup user based on authenticated key + // The middleware MUST NOT trust the user already in context + authenticatedUser, err := be.UserByPublicKey(mockCtx, attackerPubKey) + is.NoErr(err) + + // EXPECTED: User should be "attacker", NOT "admin" + is.Equal(authenticatedUser.Username(), "testattacker") + is.True(!authenticatedUser.IsAdmin()) + + // If the vulnerability exists, the context would still have admin user + contextUser := proto.UserFromContext(mockCtx) + if contextUser != nil && contextUser.Username() == "testadmin" { + t.Logf("WARNING: Context still contains admin user! This indicates the vulnerability exists.") + t.Logf("The authenticated key is attacker's, but context has admin user.") + } + }) +} + +// mockSSHContext implements ssh.Context for testing +type mockSSHContext struct { + context.Context + values map[any]any + permissions *ssh.Permissions +} + +func (m *mockSSHContext) SetValue(key, value any) { + m.values[key] = value +} + +func (m *mockSSHContext) Value(key any) any { + if v, ok := m.values[key]; ok { + return v + } + return m.Context.Value(key) +} + +func (m *mockSSHContext) Permissions() *ssh.Permissions { + return m.permissions +} + +func (m *mockSSHContext) User() string { return "" } +func (m *mockSSHContext) RemoteAddr() net.Addr { return &net.TCPAddr{} } +func (m *mockSSHContext) LocalAddr() net.Addr { return &net.TCPAddr{} } +func (m *mockSSHContext) ServerVersion() string { return "" } +func (m *mockSSHContext) ClientVersion() string { return "" } +func (m *mockSSHContext) SessionID() string { return "" } +func (m *mockSSHContext) Lock() {} +func (m *mockSSHContext) Unlock() {} diff --git a/pkg/ssh/ssh.go b/pkg/ssh/ssh.go index 2d56e0a7bf7851257f1b910e3612dae0408804a3..6fc9fcdf82e78b45890e0ce88ef1a0324a0019e7 100644 --- a/pkg/ssh/ssh.go +++ b/pkg/ssh/ssh.go @@ -16,7 +16,6 @@ import ( "github.com/charmbracelet/soft-serve/pkg/backend" "github.com/charmbracelet/soft-serve/pkg/config" "github.com/charmbracelet/soft-serve/pkg/db" - "github.com/charmbracelet/soft-serve/pkg/proto" "github.com/charmbracelet/soft-serve/pkg/store" "github.com/charmbracelet/ssh" "github.com/prometheus/client_golang/prometheus" @@ -74,13 +73,14 @@ func NewSSHServer(ctx context.Context) (*SSHServer, error) { CommandMiddleware, // Logging middleware. LoggingMiddleware, - // Context middleware. - ContextMiddleware(cfg, dbx, datastore, be, logger), // Authentication middleware. // gossh.PublicKeyHandler doesn't guarantee that the public key // is in fact the one used for authentication, so we need to // check it again here. AuthenticationMiddleware, + // Context middleware. + // This must come first to set up the context. + ContextMiddleware(cfg, dbx, datastore, be, logger), ), } @@ -166,14 +166,6 @@ func (s *SSHServer) PublicKeyHandler(ctx ssh.Context, pk ssh.PublicKey) (allowed } allowed = true - defer func(allowed *bool) { - publicKeyCounter.WithLabelValues(strconv.FormatBool(*allowed)).Inc() - }(&allowed) - - user, _ := s.be.UserByPublicKey(ctx, pk) - if user != nil { - ctx.SetValue(proto.ContextKeyUser, user) - } // XXX: store the first "approved" public-key fingerprint in the // permissions block to use for authentication later. @@ -194,10 +186,10 @@ func (s *SSHServer) KeyboardInteractiveHandler(ctx ssh.Context, _ gossh.Keyboard keyboardInteractiveCounter.WithLabelValues(strconv.FormatBool(ac)).Inc() // If we're allowing keyless access, reset the public key fingerprint - if ac { - initializePermissions(ctx) - perms := ctx.Permissions() + initializePermissions(ctx) + perms := ctx.Permissions() + if ac { // XXX: reset the public-key fingerprint. This is used to validate the // public key being used to authenticate. perms.Extensions["pubkey-fp"] = "" diff --git a/testscript/script_test.go b/testscript/script_test.go index 44b4783632ea2d12ffa217dc49d02323ee3a9199..550a3b3e06f9b386e6bf588b6836847c53ef0187 100644 --- a/testscript/script_test.go +++ b/testscript/script_test.go @@ -82,6 +82,10 @@ func TestScript(t *testing.T) { admin1Key, admin1 := mkkey("admin1") _, admin2 := mkkey("admin2") user1Key, user1 := mkkey("user1") + attackerKey, attacker := mkkey("attacker") + attackerSigner := &maliciousSigner{ + publicKey: admin1.PublicKey(), + } testscript.Run(t, testscript.Params{ Dir: "./testdata/", @@ -90,8 +94,10 @@ func TestScript(t *testing.T) { Cmds: map[string]func(ts *testscript.TestScript, neg bool, args []string){ "soft": cmdSoft("admin", admin1.Signer()), "usoft": cmdSoft("user1", user1.Signer()), + "attacksoft": cmdSoft("attacker", attackerSigner, attacker.Signer()), "git": cmdGit(admin1Key), "ugit": cmdGit(user1Key), + "agit": cmdGit(attackerKey), "curl": cmdCurl, "mkfile": cmdMkfile, "envfile": cmdEnvfile, @@ -127,6 +133,7 @@ func TestScript(t *testing.T) { e.Setenv("ADMIN1_AUTHORIZED_KEY", admin1.AuthorizedKey()) e.Setenv("ADMIN2_AUTHORIZED_KEY", admin2.AuthorizedKey()) e.Setenv("USER1_AUTHORIZED_KEY", user1.AuthorizedKey()) + e.Setenv("ATTACKER_AUTHORIZED_KEY", attacker.AuthorizedKey()) e.Setenv("SSH_KNOWN_HOSTS_FILE", filepath.Join(t.TempDir(), "known_hosts")) e.Setenv("SSH_KNOWN_CONFIG_FILE", filepath.Join(t.TempDir(), "config")) @@ -189,14 +196,14 @@ func TestScript(t *testing.T) { }) } -func cmdSoft(user string, key ssh.Signer) func(ts *testscript.TestScript, neg bool, args []string) { +func cmdSoft(user string, keys ...ssh.Signer) func(ts *testscript.TestScript, neg bool, args []string) { return func(ts *testscript.TestScript, neg bool, args []string) { cli, err := ssh.Dial( "tcp", net.JoinHostPort("localhost", ts.Getenv("SSH_PORT")), &ssh.ClientConfig{ User: user, - Auth: []ssh.AuthMethod{ssh.PublicKeys(key)}, + Auth: []ssh.AuthMethod{ssh.PublicKeys(keys...)}, HostKeyCallback: ssh.InsecureIgnoreHostKey(), }, ) @@ -613,3 +620,20 @@ func setupPostgres(t testscript.T, cfg *config.Config) (func(), error) { } }, nil } + +type maliciousSigner struct { + publicKey ssh.PublicKey +} + +var _ ssh.Signer = (*maliciousSigner)(nil) + +// PublicKey implements ssh.Signer. +func (m *maliciousSigner) PublicKey() ssh.PublicKey { + return m.publicKey +} + +// Sign implements ssh.Signer. +func (m *maliciousSigner) Sign(rand io.Reader, data []byte) (*ssh.Signature, error) { + // The attacker doesn't know how to sign the data without a private key. + return &ssh.Signature{}, nil +} diff --git a/testscript/testdata/auth-bypass-regression.txtar b/testscript/testdata/auth-bypass-regression.txtar new file mode 100644 index 0000000000000000000000000000000000000000..1d31a5d7d9edd6d17b4cbbf9546c001d81bf31c4 --- /dev/null +++ b/testscript/testdata/auth-bypass-regression.txtar @@ -0,0 +1,63 @@ +# vi: set ft=conf +# Regression test for authentication bypass vulnerability +# +# VULNERABILITY DESCRIPTION: +# A critical authentication bypass allows an attacker to impersonate any user +# (including Admin) by offering the user's public key but failing to sign with +# it, then successfully authenticating with their own key. +# +# ATTACK SCENARIO: +# 1. Attacker obtains Admin's public key (publicly available) +# 2. Attacker configures SSH client to offer TWO keys in sequence: +# - First: Admin's public key (attacker has this but not the private key) +# - Second: Attacker's own valid key pair +# 3. During SSH handshake: +# - Server sees admin's public key offered +# - PublicKeyHandler() is called, looks up admin user, stores in context +# - Server requests signature with admin's key +# - Attacker can't sign (doesn't have admin's private key), this key fails +# - Server tries next key (attacker's key) +# - PublicKeyHandler() called again with attacker's key +# - Server requests signature with attacker's key +# - Attacker signs successfully with their own private key +# 4. Admin user is still in context from step 3, even though authentication +# succeeded with attacker's key! +# 5. Attacker gains full Admin privileges +# +# THIS TEST VERIFIES: +# - Using "attacksoft" command which offers both admin and attacker keys +# - Attacker should NOT be able to perform admin user operations +# - Attacker should NOT gain admin user privileges + +[windows] dos2unix notauthorizederr.txt + +# start soft serve +exec soft serve & +# wait for SSH server to start +ensureserverrunning SSH_PORT + +# Create a private repo as admin that only admin can access +soft repo create admin-only-repo -p + +# TEST 1: Simulate the attack using attacksoft command +! attacksoft repo create attacker-created-repo + +# TEST 2: Verify attacker cannot access admin's private repo +! attacksoft git-upload-pack admin-only-repo +cmp stderr notauthorizederr.txt + +# TEST 3: Verify admin can still create repos (sanity check) +soft repo create admin-created-repo + +# TEST 4: Verify attacker cannot delete admin's repo +! attacksoft repo delete admin-only-repo + +# TEST 5: Verify attacker cannot change settings +! attacksoft settings anon-access read-write + +# stop the server +[windows] stopserver +[windows] ! stderr . + +-- notauthorizederr.txt -- +Error: you are not authorized to do this