From c630b88f3598e5946ceb648cedfdf5254813df37 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 12 Dec 2024 23:34:52 -0800 Subject: [PATCH] Fix misuse of PublicKeyCallback(#32810) (#32824) Backport #32810 --- go.mod | 2 +- go.sum | 4 +-- modules/ssh/ssh.go | 66 ++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 64 insertions(+), 8 deletions(-) diff --git a/go.mod b/go.mod index 8cac5ec507..66e645b09e 100644 --- a/go.mod +++ b/go.mod @@ -34,7 +34,7 @@ require ( github.com/ethantkoenig/rupture v1.0.1 github.com/felixge/fgprof v0.9.4 github.com/fsnotify/fsnotify v1.7.0 - github.com/gliderlabs/ssh v0.3.6 + github.com/gliderlabs/ssh v0.3.8 github.com/go-ap/activitypub v0.0.0-20240316125321-b61fd6a83225 github.com/go-ap/jsonld v0.0.0-20221030091449-f2a191312c73 github.com/go-chi/chi/v5 v5.0.12 diff --git a/go.sum b/go.sum index f7c5d2ad68..c4bd1db398 100644 --- a/go.sum +++ b/go.sum @@ -269,8 +269,8 @@ github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nos github.com/fsnotify/fsnotify v1.7.0/go.mod h1:40Bi/Hjc2AVfZrqy+aj+yEI+/bRxZnMJyTJwOpGvigM= github.com/fxamacker/cbor/v2 v2.6.0 h1:sU6J2usfADwWlYDAFhZBQ6TnLFBHxgesMrQfQgk1tWA= github.com/fxamacker/cbor/v2 v2.6.0/go.mod h1:pxXPTn3joSm21Gbwsv0w9OSA2y1HFR9qXEeXQVeNoDQ= -github.com/gliderlabs/ssh v0.3.6 h1:ZzjlDa05TcFRICb3anf/dSPN3ewz1Zx6CMLPWgkm3b8= -github.com/gliderlabs/ssh v0.3.6/go.mod h1:zpHEXBstFnQYtGnB8k8kQLol82umzn/2/snG7alWVD8= +github.com/gliderlabs/ssh v0.3.8 h1:a4YXD1V7xMF9g5nTkdfnja3Sxy1PVDCj1Zg4Wb8vY6c= +github.com/gliderlabs/ssh v0.3.8/go.mod h1:xYoytBv1sV0aL3CavoDuJIQNURXkkfPA/wxQ1pL1fAU= github.com/glycerine/go-unsnap-stream v0.0.0-20181221182339-f9677308dec2/go.mod h1:/20jfyN9Y5QPEAprSgKAUr+glWDY39ZiUEAYOEv5dsE= github.com/glycerine/goconvey v0.0.0-20190410193231-58a59202ab31/go.mod h1:Ogl1Tioa0aV7gstGFO7KhffUsb9M4ydbEbbxpcEDc24= github.com/go-ap/activitypub v0.0.0-20240316125321-b61fd6a83225 h1:OoM81OclgRX7CUch4M7MmsH0NcmLWpFiSn7rhs6Y5ZU= diff --git a/modules/ssh/ssh.go b/modules/ssh/ssh.go index f8e4f569b8..3b933b4301 100644 --- a/modules/ssh/ssh.go +++ b/modules/ssh/ssh.go @@ -13,10 +13,12 @@ import ( "errors" "fmt" "io" + "maps" "net" "os" "os/exec" "path/filepath" + "reflect" "strconv" "strings" "sync" @@ -33,9 +35,22 @@ import ( gossh "golang.org/x/crypto/ssh" ) -type contextKey string +// The ssh auth overall works like this: +// NewServerConn: +// serverHandshake+serverAuthenticate: +// PublicKeyCallback: +// PublicKeyHandler (our code): +// reset(ctx.Permissions) and set ctx.Permissions.giteaKeyID = keyID +// pubKey.Verify +// return ctx.Permissions // only reaches here, the pub key is really authenticated +// set conn.Permissions from serverAuthenticate +// sessionHandler(conn) +// +// 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 -const giteaKeyID = contextKey("gitea-key-id") +const giteaPermissionExtensionKeyID = "gitea-perm-ext-key-id" func getExitStatusFromError(err error) int { if err == nil { @@ -61,8 +76,32 @@ func getExitStatusFromError(err error) int { return waitStatus.ExitStatus() } +// sessionPartial is the private struct from "gliderlabs/ssh/session.go" +// We need to read the original "conn" field from "ssh.Session interface" which contains the "*session pointer" +// https://github.com/gliderlabs/ssh/blob/d137aad99cd6f2d9495bfd98c755bec4e5dffb8c/session.go#L109-L113 +// If upstream fixes the problem and/or changes the struct, we need to follow. +// If the struct mismatches, the builtin ssh server will fail during integration tests. +type sessionPartial struct { + sync.Mutex + gossh.Channel + conn *gossh.ServerConn +} + +func ptr[T any](intf any) *T { + // https://pkg.go.dev/unsafe#Pointer + // (1) Conversion of a *T1 to Pointer to *T2. + // Provided that T2 is no larger than T1 and that the two share an equivalent memory layout, + // this conversion allows reinterpreting data of one type as data of another type. + v := reflect.ValueOf(intf) + p := v.UnsafePointer() + return (*T)(p) +} + func sessionHandler(session ssh.Session) { - keyID := fmt.Sprintf("%d", session.Context().Value(giteaKeyID).(int64)) + // 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] command := session.RawCommand() @@ -164,6 +203,23 @@ func sessionHandler(session ssh.Session) { } func publicKeyHandler(ctx ssh.Context, key ssh.PublicKey) bool { + // The publicKeyHandler (PublicKeyCallback) only helps to provide the candidate keys to authenticate, + // It does NOT really verify here, so we could only record the related information here. + // After authentication (Verify), the "Permissions" will be assigned to the ssh conn, + // then we can use it in the "session handler" + + // 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), + } + } + if log.IsDebug() { // <- FingerprintSHA256 is kinda expensive so only calculate it if necessary log.Debug("Handle Public Key: Fingerprint: %s from %s", gossh.FingerprintSHA256(key), ctx.RemoteAddr()) } @@ -238,7 +294,7 @@ func publicKeyHandler(ctx ssh.Context, key ssh.PublicKey) bool { if log.IsDebug() { // <- FingerprintSHA256 is kinda expensive so only calculate it if necessary log.Debug("Successfully authenticated: %s Certificate Fingerprint: %s Principal: %s", ctx.RemoteAddr(), gossh.FingerprintSHA256(key), principal) } - ctx.SetValue(giteaKeyID, pkey.ID) + setPermExt(pkey.ID) return true } @@ -266,7 +322,7 @@ func publicKeyHandler(ctx ssh.Context, key ssh.PublicKey) bool { if log.IsDebug() { // <- FingerprintSHA256 is kinda expensive so only calculate it if necessary log.Debug("Successfully authenticated: %s Public Key Fingerprint: %s", ctx.RemoteAddr(), gossh.FingerprintSHA256(key)) } - ctx.SetValue(giteaKeyID, pkey.ID) + setPermExt(pkey.ID) return true }