8539f9ad39918b67d612a35785a2b4326efc8741
- Author
- Ayman Bagabas <ayman.bagabas@gmail.com>
- Committer
- Ayman Bagabas <ayman.bagabas@gmail.com>
- Date
Message
fix: authentication bypass
Diff
This diff is truncated to protect this page.
1diff --git a/pkg/ssh/middleware.go b/pkg/ssh/middleware.go
2index 62f19044b2de02955e2dc366e189eff1edfdd0f4..25a00b1a26754eae77b359bee82a68d178848884 100644
3--- a/pkg/ssh/middleware.go
4+++ b/pkg/ssh/middleware.go
5@@ -2,6 +2,7 @@ package ssh
6
7 import (
8 "fmt"
9+ "strconv"
10 "time"
11
12 "charm.land/log/v2"
13@@ -30,23 +31,43 @@ func AuthenticationMiddleware(sh ssh.Handler) ssh.Handler {
14 // validate the authentication. We need to verify that the _last_ key
15 // that was approved is the one that's being used.
16
17+ ctx := s.Context()
18+ be := backend.FromContext(ctx)
19+
20+ var pkFp string
21+ perms := s.Permissions().Permissions
22 pk := s.PublicKey()
23 if pk != nil {
24 // There is no public key stored in the context, public-key auth
25 // was never requested, skip
26- perms := s.Permissions().Permissions
27 if perms == nil {
28 wish.Fatalln(s, ErrPermissionDenied)
29 return
30 }
31
32- // Check if the key is the same as the one we have in context
33- fp := perms.Extensions["pubkey-fp"]
34- if fp == "" || fp != gossh.FingerprintSHA256(pk) {
35- wish.Fatalln(s, ErrPermissionDenied)
36- return
37- }
38+ pkFp = gossh.FingerprintSHA256(pk)
39+ }
40+
41+ // Check if the key is the same as the one we have in context
42+ fp := perms.Extensions["pubkey-fp"]
43+ if fp != "" && fp != pkFp {
44+ wish.Fatalln(s, ErrPermissionDenied)
45+ return
46+ }
47+
48+ ac := be.AllowKeyless(ctx)
49+ publicKeyCounter.WithLabelValues(strconv.FormatBool(ac || pk != nil)).Inc()
50+ if !ac && pk == nil {
51+ wish.Fatalln(s, ErrPermissionDenied)
52+ return
53+ }
54+
55+ // Set the auth'd user, or anon, in the context
56+ var user proto.User
57+ if pk != nil {
58+ user, _ = be.UserByPublicKey(ctx, pk)
59 }
60+ ctx.SetValue(proto.ContextKeyUser, user)
61
62 sh(s)
63 }
64diff --git a/pkg/ssh/middleware_test.go b/pkg/ssh/middleware_test.go
65new file mode 100644
66index 0000000000000000000000000000000000000000..149fd8d8bc9ca16b8e703f1729753965d519c0c8
67--- /dev/null
68+++ b/pkg/ssh/middleware_test.go
69@@ -0,0 +1,180 @@
70+package ssh
71+
72+import (
73+ "context"
74+ "net"
75+ "testing"
76+
77+ "github.com/charmbracelet/keygen"
78+ "github.com/charmbracelet/soft-serve/pkg/backend"
79+ "github.com/charmbracelet/soft-serve/pkg/config"
80+ "github.com/charmbracelet/soft-serve/pkg/db"
81+ "github.com/charmbracelet/soft-serve/pkg/db/migrate"
82+ "github.com/charmbracelet/soft-serve/pkg/proto"
83+ "github.com/charmbracelet/soft-serve/pkg/store"
84+ "github.com/charmbracelet/soft-serve/pkg/store/database"
85+ "github.com/charmbracelet/ssh"
86+ "github.com/matryer/is"
87+ gossh "golang.org/x/crypto/ssh"
88+ _ "modernc.org/sqlite"
89+)
90+
91+// TestAuthenticationBypass tests for CVE-TBD: Authentication Bypass Vulnerability
92+//
93+// VULNERABILITY:
94+// A critical authentication bypass allows an attacker to impersonate any user
95+// (including Admin) by "offering" the victim's public key during the SSH handshake
96+// before authenticating with their own valid key. This occurs because the user
97+// identity is stored in the session context during the "offer" phase in
98+// PublicKeyHandler and is not properly cleared/validated in AuthenticationMiddleware.
99+//
100+// This test verifies that:
101+// 1. User context is properly set based on the AUTHENTICATED key, not offered keys
102+// 2. User context from failed authentication attempts is not preserved
103+// 3. Non-admin users cannot gain admin privileges through this attack
104+func TestAuthenticationBypass(t *testing.T) {
105+ is := is.New(t)
106+ ctx := context.Background()
107+
108+ // Setup temporary database
109+ dp := t.TempDir()
110+ cfg := config.DefaultConfig()
111+ cfg.DataPath = dp
112+ cfg.DB.Driver = "sqlite"
113+ cfg.DB.DataSource = dp + "/test.db"
114+
115+ ctx = config.WithContext(ctx, cfg)
116+ dbx, err := db.Open(ctx, cfg.DB.Driver, cfg.DB.DataSource)
117+ is.NoErr(err)
118+ defer dbx.Close()
119+
120+ is.NoErr(migrate.Migrate(ctx, dbx))
121+ dbstore := database.New(ctx, dbx)
122+ ctx = store.WithContext(ctx, dbstore)
123+ be := backend.New(ctx, cfg, dbx, dbstore)
124+ ctx = backend.WithContext(ctx, be)
125+
126+ // Generate keys for admin and attacker
127+ adminKeyPath := dp + "/admin_key"
128+ adminPair, err := keygen.New(adminKeyPath, keygen.WithKeyType(keygen.Ed25519), keygen.WithWrite())
129+ is.NoErr(err)
130+
131+ attackerKeyPath := dp + "/attacker_key"
132+ attackerPair, err := keygen.New(attackerKeyPath, keygen.WithKeyType(keygen.Ed25519), keygen.WithWrite())
133+ is.NoErr(err)
134+
135+ // Parse public keys
136+ adminPubKey, _, _, _, err := gossh.ParseAuthorizedKey([]byte(adminPair.AuthorizedKey()))
137+ is.NoErr(err)
138+
139+ attackerPubKey, _, _, _, err := gossh.ParseAuthorizedKey([]byte(attackerPair.AuthorizedKey()))
140+ is.NoErr(err)
141+
142+ // Create admin user
143+ adminUser, err := be.CreateUser(ctx, "testadmin", proto.UserOptions{
144+ Admin: true,
145+ PublicKeys: []gossh.PublicKey{adminPubKey},
146+ })
147+ is.NoErr(err)
148+ is.True(adminUser != nil)
149+
150+ // Create attacker (non-admin) user
151+ attackerUser, err := be.CreateUser(ctx, "testattacker", proto.UserOptions{
152+ Admin: false,
153+ PublicKeys: []gossh.PublicKey{attackerPubKey},
154+ })
155+ is.NoErr(err)
156+ is.True(attackerUser != nil)
157+ is.True(!attackerUser.IsAdmin()) // Verify attacker is NOT admin
158+
159+ // Test: Verify that looking up user by key gives correct user
160+ t.Run("user_lookup_by_key", func(t *testing.T) {
161+ is := is.New(t)
162+
163+ // Looking up admin key should return admin user
164+ user, err := be.UserByPublicKey(ctx, adminPubKey)
165+ is.NoErr(err)
166+ is.Equal(user.Username(), "testadmin")
167+ is.True(user.IsAdmin())
168+
169diff --git a/pkg/ssh/ssh.go b/pkg/ssh/ssh.go
170index 2d56e0a7bf7851257f1b910e3612dae0408804a3..6fc9fcdf82e78b45890e0ce88ef1a0324a0019e7 100644
171--- a/pkg/ssh/ssh.go
172+++ b/pkg/ssh/ssh.go
173@@ -16,7 +16,6 @@ import (
174 "github.com/charmbracelet/soft-serve/pkg/backend"
175 "github.com/charmbracelet/soft-serve/pkg/config"
176 "github.com/charmbracelet/soft-serve/pkg/db"
177- "github.com/charmbracelet/soft-serve/pkg/proto"
178 "github.com/charmbracelet/soft-serve/pkg/store"
179 "github.com/charmbracelet/ssh"
180 "github.com/prometheus/client_golang/prometheus"
181@@ -74,13 +73,14 @@ func NewSSHServer(ctx context.Context) (*SSHServer, error) {
182 CommandMiddleware,
183 // Logging middleware.
184 LoggingMiddleware,
185- // Context middleware.
186- ContextMiddleware(cfg, dbx, datastore, be, logger),
187 // Authentication middleware.
188 // gossh.PublicKeyHandler doesn't guarantee that the public key
189 // is in fact the one used for authentication, so we need to
190 // check it again here.
191 AuthenticationMiddleware,
192+ // Context middleware.
193+ // This must come first to set up the context.
194+ ContextMiddleware(cfg, dbx, datastore, be, logger),
195 ),
196 }
197
198@@ -166,14 +166,6 @@ func (s *SSHServer) PublicKeyHandler(ctx ssh.Context, pk ssh.PublicKey) (allowed
199 }
200
201 allowed = true
202- defer func(allowed *bool) {
203- publicKeyCounter.WithLabelValues(strconv.FormatBool(*allowed)).Inc()
204- }(&allowed)
205-
206- user, _ := s.be.UserByPublicKey(ctx, pk)
207- if user != nil {
208- ctx.SetValue(proto.ContextKeyUser, user)
209- }
210
211 // XXX: store the first "approved" public-key fingerprint in the
212 // permissions block to use for authentication later.
213@@ -194,10 +186,10 @@ func (s *SSHServer) KeyboardInteractiveHandler(ctx ssh.Context, _ gossh.Keyboard
214 keyboardInteractiveCounter.WithLabelValues(strconv.FormatBool(ac)).Inc()
215
216 // If we're allowing keyless access, reset the public key fingerprint
217- if ac {
218- initializePermissions(ctx)
219- perms := ctx.Permissions()
220+ initializePermissions(ctx)
221+ perms := ctx.Permissions()
222
223+ if ac {
224 // XXX: reset the public-key fingerprint. This is used to validate the
225 // public key being used to authenticate.
226 perms.Extensions["pubkey-fp"] = ""
227diff --git a/testscript/script_test.go b/testscript/script_test.go
228index 44b4783632ea2d12ffa217dc49d02323ee3a9199..550a3b3e06f9b386e6bf588b6836847c53ef0187 100644
229--- a/testscript/script_test.go
230+++ b/testscript/script_test.go
231@@ -82,6 +82,10 @@ func TestScript(t *testing.T) {
232 admin1Key, admin1 := mkkey("admin1")
233 _, admin2 := mkkey("admin2")
234 user1Key, user1 := mkkey("user1")
235+ attackerKey, attacker := mkkey("attacker")
236+ attackerSigner := &maliciousSigner{
237+ publicKey: admin1.PublicKey(),
238+ }
239
240 testscript.Run(t, testscript.Params{
241 Dir: "./testdata/",
242@@ -90,8 +94,10 @@ func TestScript(t *testing.T) {
243 Cmds: map[string]func(ts *testscript.TestScript, neg bool, args []string){
244 "soft": cmdSoft("admin", admin1.Signer()),
245 "usoft": cmdSoft("user1", user1.Signer()),
246+ "attacksoft": cmdSoft("attacker", attackerSigner, attacker.Signer()),
247 "git": cmdGit(admin1Key),
248 "ugit": cmdGit(user1Key),
249+ "agit": cmdGit(attackerKey),
250 "curl": cmdCurl,
251 "mkfile": cmdMkfile,
252 "envfile": cmdEnvfile,
253@@ -127,6 +133,7 @@ func TestScript(t *testing.T) {
254 e.Setenv("ADMIN1_AUTHORIZED_KEY", admin1.AuthorizedKey())
255 e.Setenv("ADMIN2_AUTHORIZED_KEY", admin2.AuthorizedKey())
256 e.Setenv("USER1_AUTHORIZED_KEY", user1.AuthorizedKey())
257+ e.Setenv("ATTACKER_AUTHORIZED_KEY", attacker.AuthorizedKey())
258 e.Setenv("SSH_KNOWN_HOSTS_FILE", filepath.Join(t.TempDir(), "known_hosts"))
259 e.Setenv("SSH_KNOWN_CONFIG_FILE", filepath.Join(t.TempDir(), "config"))
260
261@@ -189,14 +196,14 @@ func TestScript(t *testing.T) {
262 })
263 }
264
265-func cmdSoft(user string, key ssh.Signer) func(ts *testscript.TestScript, neg bool, args []string) {
266+func cmdSoft(user string, keys ...ssh.Signer) func(ts *testscript.TestScript, neg bool, args []string) {
267 return func(ts *testscript.TestScript, neg bool, args []string) {
268 cli, err := ssh.Dial(
269 "tcp",
270 net.JoinHostPort("localhost", ts.Getenv("SSH_PORT")),
271 &ssh.ClientConfig{
272 User: user,
273- Auth: []ssh.AuthMethod{ssh.PublicKeys(key)},
274+ Auth: []ssh.AuthMethod{ssh.PublicKeys(keys...)},
275 HostKeyCallback: ssh.InsecureIgnoreHostKey(),
276 },
277 )
278@@ -613,3 +620,20 @@ func setupPostgres(t testscript.T, cfg *config.Config) (func(), error) {
279 }
280 }, nil
281 }
282+
283+type maliciousSigner struct {
284+ publicKey ssh.PublicKey
285+}
286+
287+var _ ssh.Signer = (*maliciousSigner)(nil)
288+
289+// PublicKey implements ssh.Signer.
290+func (m *maliciousSigner) PublicKey() ssh.PublicKey {
291+ return m.publicKey
292+}
293+
294+// Sign implements ssh.Signer.
295+func (m *maliciousSigner) Sign(rand io.Reader, data []byte) (*ssh.Signature, error) {
296+ // The attacker doesn't know how to sign the data without a private key.
297+ return &ssh.Signature{}, nil
298+}
299diff --git a/testscript/testdata/auth-bypass-regression.txtar b/testscript/testdata/auth-bypass-regression.txtar
300new file mode 100644
301index 0000000000000000000000000000000000000000..1d31a5d7d9edd6d17b4cbbf9546c001d81bf31c4
302--- /dev/null
303+++ b/testscript/testdata/auth-bypass-regression.txtar
304@@ -0,0 +1,63 @@
305+# vi: set ft=conf
306+# Regression test for authentication bypass vulnerability
307+#
308+# VULNERABILITY DESCRIPTION:
309+# A critical authentication bypass allows an attacker to impersonate any user
310+# (including Admin) by offering the user's public key but failing to sign with
311+# it, then successfully authenticating with their own key.
312+#
313+# ATTACK SCENARIO:
314+# 1. Attacker obtains Admin's public key (publicly available)
315+# 2. Attacker configures SSH client to offer TWO keys in sequence:
316+# - First: Admin's public key (attacker has this but not the private key)
317+# - Second: Attacker's own valid key pair
318+# 3. During SSH handshake:
319+# - Server sees admin's public key offered
320+# - PublicKeyHandler() is called, looks up admin user, stores in context
321+# - Server requests signature with admin's key
322+# - Attacker can't sign (doesn't have admin's private key), this key fails
323+# - Server tries next key (attacker's key)
324+# - PublicKeyHandler() called again with attacker's key
325+# - Server requests signature with attacker's key
326+# - Attacker signs successfully with their own private key
327+# 4. Admin user is still in context from step 3, even though authentication
328+# succeeded with attacker's key!
329+# 5. Attacker gains full Admin privileges
330+#
331+# THIS TEST VERIFIES:
332+# - Using "attacksoft" command which offers both admin and attacker keys
333+# - Attacker should NOT be able to perform admin user operations
334+# - Attacker should NOT gain admin user privileges
335+
336+[windows] dos2unix notauthorizederr.txt
337+
338+# start soft serve
339+exec soft serve &
340+# wait for SSH server to start
341+ensureserverrunning SSH_PORT
342+
343+# Create a private repo as admin that only admin can access
344+soft repo create admin-only-repo -p
345+
346+# TEST 1: Simulate the attack using attacksoft command
347+! attacksoft repo create attacker-created-repo
348+
349+# TEST 2: Verify attacker cannot access admin's private repo
350+! attacksoft git-upload-pack admin-only-repo
351+cmp stderr notauthorizederr.txt
352+
353+# TEST 3: Verify admin can still create repos (sanity check)
354+soft repo create admin-created-repo
355+
356+# TEST 4: Verify attacker cannot delete admin's repo
357+! attacksoft repo delete admin-only-repo
358+
359+# TEST 5: Verify attacker cannot change settings
360+! attacksoft settings anon-access read-write
361+
362+# stop the server
363+[windows] stopserver
364+[windows] ! stderr .
365+
366+-- notauthorizederr.txt --
367+Error: you are not authorized to do this