Detailed changes
@@ -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)
}
@@ -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() {}
@@ -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"] = ""
@@ -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
+}
@@ -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