From 3e1b03838e455d88089b4d6683443e1adfb4752e Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 11 Dec 2024 23:14:27 +0100 Subject: [PATCH] fix: ensure correct ssh public key is used for authentication - The root cause is described in https://github.com/golang/crypto/commit/b4f1988a35dee11ec3e05d6bf3e90b695fbd8909 - Move to a fork of `github.com/gliderlabs/ssh` that exposes the permissions that was chosen by `x/crypto/ssh` after succesfully authenticating, this is the recommended mitigation by the Golang security team. The fork exposes this, since `gliderlabs/ssh` instead relies on context values to do so, which is vulnerable to the same attack, although partially mitigated by the fix in `x/crypto/ssh` it would not be good practice and defense deep to rely on it. - Existing tests covers that the functionality is preserved. - No tests are added to ensure it fixes the described security, the exploit relies on non-standard SSH behavior it would be too hard to craft SSH packets to exploit this. --- go.mod | 2 ++ go.sum | 4 ++-- modules/ssh/ssh.go | 17 +++++++++-------- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/go.mod b/go.mod index 5a3f3cc4dc..b4900e1564 100644 --- a/go.mod +++ b/go.mod @@ -301,3 +301,5 @@ replace github.com/nektos/act => code.forgejo.org/forgejo/act v1.22.0 replace github.com/mholt/archiver/v3 => code.forgejo.org/forgejo/archiver/v3 v3.5.1 replace github.com/goccy/go-json => github.com/grafana/go-json v0.0.0-20241210211703-a119ee5a0a3b + +replace github.com/gliderlabs/ssh => code.forgejo.org/forgejo/ssh v0.0.0-20241211213324-5fc306ca0616 diff --git a/go.sum b/go.sum index 2419878a70..ed15750356 100644 --- a/go.sum +++ b/go.sum @@ -10,6 +10,8 @@ code.forgejo.org/forgejo/archiver/v3 v3.5.1 h1:UmmbA7D5550uf71SQjarmrn6yKwOGxtEj code.forgejo.org/forgejo/archiver/v3 v3.5.1/go.mod h1:e3dqJ7H78uzsRSEACH1joayhuSyhnonssnDhppzS1L4= code.forgejo.org/forgejo/reply v1.0.2 h1:dMhQCHV6/O3L5CLWNTol+dNzDAuyCK88z4J/lCdgFuQ= code.forgejo.org/forgejo/reply v1.0.2/go.mod h1:RyZUfzQLc+fuLIGjTSQWDAJWPiL4WtKXB/FifT5fM7U= +code.forgejo.org/forgejo/ssh v0.0.0-20241211213324-5fc306ca0616 h1:kEZL84+02jY9RxXM4zHBWZ3Fml0B09cmP1LGkDsCfIA= +code.forgejo.org/forgejo/ssh v0.0.0-20241211213324-5fc306ca0616/go.mod h1:zpHEXBstFnQYtGnB8k8kQLol82umzn/2/snG7alWVD8= code.forgejo.org/go-chi/binding v1.0.0 h1:EIDJtk9brK7WsT7rvS/D4cxX8XlnhY3LMy8ex1jeHu0= code.forgejo.org/go-chi/binding v1.0.0/go.mod h1:fWwqaHj0H1/KeCpBqdvKunflq8pYfciEHI5v3UUeE2E= code.forgejo.org/go-chi/cache v1.0.0 h1:akLfGxNlHcacmtutovNtYFSTMsbdcp5MGjAEsP4pxnE= @@ -225,8 +227,6 @@ github.com/fsnotify/fsnotify v1.8.0 h1:dAwr6QBTBZIkG8roQaJjGof0pp0EeF+tNV7YBP3F/ github.com/fsnotify/fsnotify v1.8.0/go.mod h1:8jBTzvmWwFyi3Pb8djgCCO5IBqzKJ/Jwo8TRcHyHii0= github.com/fxamacker/cbor/v2 v2.7.0 h1:iM5WgngdRBanHcxugY4JySA0nk1wZorNOpTgCMedv5E= github.com/fxamacker/cbor/v2 v2.7.0/go.mod h1:pxXPTn3joSm21Gbwsv0w9OSA2y1HFR9qXEeXQVeNoDQ= -github.com/gliderlabs/ssh v0.3.7 h1:iV3Bqi942d9huXnzEF2Mt+CY9gLu8DNM4Obd+8bODRE= -github.com/gliderlabs/ssh v0.3.7/go.mod h1:zpHEXBstFnQYtGnB8k8kQLol82umzn/2/snG7alWVD8= github.com/go-ap/activitypub v0.0.0-20231114162308-e219254dc5c9 h1:j2TrkUG/NATGi/EQS+MvEoF79CxiRUmT16ErFroNcKI= github.com/go-ap/activitypub v0.0.0-20231114162308-e219254dc5c9/go.mod h1:cJ9Ye0ZNSMN7RzZDBRY3E+8M3Bpf/R1JX22Ir9yX6WI= github.com/go-ap/errors v0.0.0-20231003111023-183eef4b31b7 h1:I2nuhyVI/48VXoRCCZR2hYBgnSXa+EuDJf/VyX06TC0= diff --git a/modules/ssh/ssh.go b/modules/ssh/ssh.go index f8e4f569b8..6ee10f718b 100644 --- a/modules/ssh/ssh.go +++ b/modules/ssh/ssh.go @@ -11,7 +11,6 @@ import ( "crypto/x509" "encoding/pem" "errors" - "fmt" "io" "net" "os" @@ -33,10 +32,6 @@ import ( gossh "golang.org/x/crypto/ssh" ) -type contextKey string - -const giteaKeyID = contextKey("gitea-key-id") - func getExitStatusFromError(err error) int { if err == nil { return 0 @@ -62,7 +57,7 @@ func getExitStatusFromError(err error) int { } func sessionHandler(session ssh.Session) { - keyID := fmt.Sprintf("%d", session.Context().Value(giteaKeyID).(int64)) + keyID := session.ConnPermissions().Extensions["forgejo-key-id"] command := session.RawCommand() @@ -238,7 +233,10 @@ 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) + if ctx.Permissions().Extensions == nil { + ctx.Permissions().Extensions = map[string]string{} + } + ctx.Permissions().Extensions["forgejo-key-id"] = strconv.FormatInt(pkey.ID, 10) return true } @@ -266,7 +264,10 @@ 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) + if ctx.Permissions().Extensions == nil { + ctx.Permissions().Extensions = map[string]string{} + } + ctx.Permissions().Extensions["forgejo-key-id"] = strconv.FormatInt(pkey.ID, 10) return true }