From d1c1e3cbccf88a9180e9a130a81342e0acb87fde Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 15 Dec 2024 14:07:50 +0800 Subject: [PATCH] Fine tune ssh related comments and code (#32846) Add more comments to explain the ssh problem, and rename `sshConn` to `sshSession` --- modules/ssh/ssh.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/modules/ssh/ssh.go b/modules/ssh/ssh.go index 6d0695ee16..7479cfbd95 100644 --- a/modules/ssh/ssh.go +++ b/modules/ssh/ssh.go @@ -13,7 +13,6 @@ import ( "errors" "fmt" "io" - "maps" "net" "os" "os/exec" @@ -49,6 +48,10 @@ import ( // Then sessionHandler should only use the "verified keyID" from the original ssh conn, but not the ctx one. // Otherwise, if a user provides 2 keys A (a correct one) and B (public key matches but no private key), // then only A succeeds to authenticate, sessionHandler will see B's keyID +// +// After x/crypto >= 0.31.0 (fix CVE-2024-45337), the PublicKeyCallback will be called again for the verified key, +// it mitigates the misuse for most cases, it's still good for us to make sure we don't rely on that mitigation +// and do not misuse the PublicKeyCallback: we should only use the verified keyID from the verified ssh conn. const giteaPermissionExtensionKeyID = "gitea-perm-ext-key-id" @@ -100,8 +103,8 @@ func ptr[T any](intf any) *T { func sessionHandler(session ssh.Session) { // here can't use session.Permissions() because it only uses the value from ctx, which might not be the authenticated one. // so we must use the original ssh conn, which always contains the correct (verified) keyID. - sshConn := ptr[sessionPartial](session) - keyID := sshConn.conn.Permissions.Extensions[giteaPermissionExtensionKeyID] + sshSession := ptr[sessionPartial](session) + keyID := sshSession.conn.Permissions.Extensions[giteaPermissionExtensionKeyID] command := session.RawCommand() @@ -210,10 +213,7 @@ func publicKeyHandler(ctx ssh.Context, key ssh.PublicKey) bool { // first, reset the ctx permissions (just like https://github.com/gliderlabs/ssh/pull/243 does) // it shouldn't be reused across different ssh conn (sessions), each pub key should have its own "Permissions" - oldCtxPerm := ctx.Permissions().Permissions ctx.Permissions().Permissions = &gossh.Permissions{} - ctx.Permissions().Permissions.CriticalOptions = maps.Clone(oldCtxPerm.CriticalOptions) - setPermExt := func(keyID int64) { ctx.Permissions().Permissions.Extensions = map[string]string{ giteaPermissionExtensionKeyID: fmt.Sprint(keyID),