From 92911bda2bb1bec1b91eaf0620e427c42acfc633 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Sun, 25 Feb 2024 11:12:57 +0100 Subject: [PATCH] require at least 2048 bits for RSA certificates/keys Signed-off-by: Nicola Murino --- internal/dataprovider/dataprovider.go | 55 +++++++++++++++++++++------ internal/httpd/httpd_test.go | 37 ++++++++++++++++-- internal/util/i18n.go | 2 + internal/vfs/sftpfs.go | 46 ++++++++++++++++++---- static/locales/en/translation.json | 2 + static/locales/it/translation.json | 2 + 6 files changed, 121 insertions(+), 23 deletions(-) diff --git a/internal/dataprovider/dataprovider.go b/internal/dataprovider/dataprovider.go index 62a4d95b..61bf7089 100644 --- a/internal/dataprovider/dataprovider.go +++ b/internal/dataprovider/dataprovider.go @@ -21,6 +21,7 @@ import ( "bytes" "context" "crypto/md5" + "crypto/rsa" "crypto/sha1" "crypto/sha256" "crypto/sha512" @@ -2877,15 +2878,32 @@ func validatePublicKeys(user *User) error { user.PublicKeys = []string{} } var validatedKeys []string - for i, k := range user.PublicKeys { - if k == "" { + for idx, key := range user.PublicKeys { + if key == "" { continue } - _, _, _, _, err := ssh.ParseAuthorizedKey([]byte(k)) + out, _, _, _, err := ssh.ParseAuthorizedKey([]byte(key)) if err != nil { - return util.NewValidationError(fmt.Sprintf("could not parse key nr. %d: %s", i+1, err)) + return util.NewI18nError( + util.NewValidationError(fmt.Sprintf("error parsing public key at position %d: %v", idx, err)), + util.I18nErrorPubKeyInvalid, + ) } - validatedKeys = append(validatedKeys, k) + if k, ok := out.(ssh.CryptoPublicKey); ok { + cryptoKey := k.CryptoPublicKey() + if rsaKey, ok := cryptoKey.(*rsa.PublicKey); ok { + if size := rsaKey.N.BitLen(); size < 2048 { + providerLog(logger.LevelError, "rsa key with size %d not accepted, minimum 2048", size) + return util.NewI18nError( + util.NewValidationError(fmt.Sprintf("invalid size %d for rsa key at position %d, minimum 2048", + size, idx)), + util.I18nErrorKeySizeInvalid, + ) + } + } + } + + validatedKeys = append(validatedKeys, key) } user.PublicKeys = util.RemoveDuplicates(validatedKeys, false) return nil @@ -3047,12 +3065,25 @@ func validateTLSCerts(certs []string) error { util.I18nErrorInvalidTLSCert, ) } - if _, err := x509.ParseCertificate(derBlock.Bytes); err != nil { + cert, err := x509.ParseCertificate(derBlock.Bytes) + if err != nil { return util.NewI18nError( util.NewValidationError(fmt.Sprintf("error parsing TLS certificate %d", idx)), util.I18nErrorInvalidTLSCert, ) } + if cert.PublicKeyAlgorithm == x509.RSA { + if rsaCert, ok := cert.PublicKey.(*rsa.PublicKey); ok { + if size := rsaCert.N.BitLen(); size < 2048 { + providerLog(logger.LevelError, "rsa cert with size %d not accepted, minimum 2048", size) + return util.NewI18nError( + util.NewValidationError(fmt.Sprintf("invalid size %d for rsa cert at position %d, minimum 2048", + size, idx)), + util.I18nErrorKeySizeInvalid, + ) + } + } + } } return nil } @@ -3275,7 +3306,7 @@ func ValidateUser(user *User) error { return err } if err := validatePublicKeys(user); err != nil { - return util.NewI18nError(err, util.I18nErrorPubKeyInvalid) + return err } if err := validateBaseFilters(&user.Filters.BaseUserFilters); err != nil { return err @@ -3477,14 +3508,14 @@ func checkUserAndPubKey(user *User, pubKey []byte, isSSHCert bool) (User, string if len(user.PublicKeys) == 0 { return *user, "", ErrInvalidCredentials } - for i, k := range user.PublicKeys { - storedPubKey, comment, _, _, err := ssh.ParseAuthorizedKey([]byte(k)) + for idx, key := range user.PublicKeys { + storedKey, comment, _, _, err := ssh.ParseAuthorizedKey([]byte(key)) if err != nil { - providerLog(logger.LevelError, "error parsing stored public key %d for user %s: %v", i, user.Username, err) + providerLog(logger.LevelError, "error parsing stored public key %d for user %s: %v", idx, user.Username, err) return *user, "", err } - if bytes.Equal(storedPubKey.Marshal(), pubKey) { - return *user, fmt.Sprintf("%s:%s", ssh.FingerprintSHA256(storedPubKey), comment), nil + if bytes.Equal(storedKey.Marshal(), pubKey) { + return *user, fmt.Sprintf("%s:%s", ssh.FingerprintSHA256(storedKey), comment), nil } } return *user, "", ErrInvalidCredentials diff --git a/internal/httpd/httpd_test.go b/internal/httpd/httpd_test.go index 7c693292..3bebde00 100644 --- a/internal/httpd/httpd_test.go +++ b/internal/httpd/httpd_test.go @@ -241,6 +241,23 @@ qwlk5iw/jQekxThg== ` testPubKeyPwd = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAILqltfCL7IPuIQ2q+8w23flfgskjIlKViEwMfjJR4mrb" privateKeyPwd = "password" + rsa1024PrivKey = `-----BEGIN OPENSSH PRIVATE KEY----- +b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAlwAAAAdzc2gtcn +NhAAAAAwEAAQAAAIEAxgrZ84gJyU7Qz8JbYuYh0fgTN29h4qVkqDkEE0lWZe7L4QRcQHrB +vycJO5vjfitY5JTojV3nbDNHN6XGVX8QNurwXmxv0EmEbqPoNO/rTf1t7qqwMBBAfSJJ5H +TXsO37vqcWSOt1Ki5yjRm232UfPo3AYXaZdOKDWKpzI12FfqkAAAIAondFqKJ3RagAAAAH +c3NoLXJzYQAAAIEAxgrZ84gJyU7Qz8JbYuYh0fgTN29h4qVkqDkEE0lWZe7L4QRcQHrBvy +cJO5vjfitY5JTojV3nbDNHN6XGVX8QNurwXmxv0EmEbqPoNO/rTf1t7qqwMBBAfSJJ5HTX +sO37vqcWSOt1Ki5yjRm232UfPo3AYXaZdOKDWKpzI12FfqkAAAADAQABAAAAgC7V5COG+a +GFJTbtJQWnnTn17D2A9upN6RcrnL4e6vLiXY8So+qP3YAicDmLrWpqP/SXDsRX/+ID4oTT +jKstiJy5jTvXAozwBbFCvNDk1qifs8p/HKzel3t0172j6gLOa2h9+clJ4BYyCk6ue4f8fV +yKTIc9chdJSpeINNY60CJxAAAAQQDhYpGXljD2Xy/CzqRXyoF+iMtOImLlbgQYswTXegk3 +7JoCNvwqg8xP+JxGpvUGpX23VWh0nBhzcAKHGlssiYQuAAAAQQDwB6s7s1WIRZ2Jsz8f6l +7/ebpPrAMyKmWkXc7KyvR53zuMkMIdvujM5NkOWh1ON8jtNumArey2dWuGVh+pXbdVAAAA +QQDTOAaMcyTfXMH/oSMsp+5obvT/RuewaRLHdBiCy0y1Jw0ykOcOCkswr/btDL26hImaHF +SheorO+2We7dnFuUIFAAAACW5pY29sYUBwMQE= +-----END OPENSSH PRIVATE KEY-----` + rsa1024PubKey = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAAAgQDGCtnziAnJTtDPwlti5iHR+BM3b2HipWSoOQQTSVZl7svhBFxAesG/Jwk7m+N+K1jklOiNXedsM0c3pcZVfxA26vBebG/QSYRuo+g07+tN/W3uqrAwEEB9IknkdNew7fu+pxZI63UqLnKNGbbfZR8+jcBhdpl04oNYqnMjXYV+qQ==" redactedSecret = "[**redacted**]" osWindows = "windows" oidcMockAddr = "127.0.0.1:11111" @@ -823,6 +840,20 @@ func TestRoleRelations(t *testing.T) { assert.NoError(t, err) } +func TestRSAKeyInvalidSize(t *testing.T) { + u := getTestUser() + u.PublicKeys = append(u.PublicKeys, rsa1024PubKey) + _, resp, err := httpdtest.AddUser(u, http.StatusBadRequest) + assert.NoError(t, err, string(resp)) + assert.Contains(t, string(resp), "invalid size") + u = getTestSFTPUser() + u.FsConfig.SFTPConfig.Password = nil + u.FsConfig.SFTPConfig.PrivateKey = kms.NewPlainSecret(rsa1024PrivKey) + _, resp, err = httpdtest.AddUser(u, http.StatusBadRequest) + assert.NoError(t, err, string(resp)) + assert.Contains(t, string(resp), "rsa key with size 1024 not accepted") +} + func TestTLSCert(t *testing.T) { u := getTestUser() u.Filters.TLSCerts = []string{"not a cert"} @@ -11065,7 +11096,7 @@ func TestWebAPIChangeUserProfileMock(t *testing.T) { setBearerForReq(req, token) rr = executeRequest(req) checkResponseCode(t, http.StatusBadRequest, rr) - assert.Contains(t, rr.Body.String(), "Validation error: could not parse key") + assert.Contains(t, rr.Body.String(), "Validation error: error parsing public key") user, _, err = httpdtest.GetUserByUsername(user.Username, http.StatusOK) assert.NoError(t, err) @@ -22409,8 +22440,8 @@ func TestWebUserSFTPFsMock(t *testing.T) { user.FsConfig.SFTPConfig.Endpoint = "127.0.0.1:22" user.FsConfig.SFTPConfig.Username = "sftpuser" user.FsConfig.SFTPConfig.Password = kms.NewPlainSecret("pwd") - user.FsConfig.SFTPConfig.PrivateKey = kms.NewPlainSecret(sftpPrivateKey) - user.FsConfig.SFTPConfig.KeyPassphrase = kms.NewPlainSecret(testPrivateKeyPwd) + user.FsConfig.SFTPConfig.PrivateKey = kms.NewPlainSecret(testPrivateKeyPwd) + user.FsConfig.SFTPConfig.KeyPassphrase = kms.NewPlainSecret(privateKeyPwd) user.FsConfig.SFTPConfig.Fingerprints = []string{sftpPkeyFingerprint} user.FsConfig.SFTPConfig.Prefix = "/home/sftpuser" user.FsConfig.SFTPConfig.DisableCouncurrentReads = true diff --git a/internal/util/i18n.go b/internal/util/i18n.go index 57b74fc1..94fb90f4 100644 --- a/internal/util/i18n.go +++ b/internal/util/i18n.go @@ -119,6 +119,8 @@ const ( I18nErrorHomeRequired = "user.home_required" I18nErrorHomeInvalid = "user.home_invalid" I18nErrorPubKeyInvalid = "user.pub_key_invalid" + I18nErrorPrivKeyInvalid = "user.priv_key_invalid" + I18nErrorKeySizeInvalid = "user.key_invalid_size" I18nErrorPrimaryGroup = "user.err_primary_group" I18nErrorDuplicateGroup = "user.err_duplicate_group" I18nErrorNoPermission = "user.no_permissions" diff --git a/internal/vfs/sftpfs.go b/internal/vfs/sftpfs.go index 55d9e677..b4085708 100644 --- a/internal/vfs/sftpfs.go +++ b/internal/vfs/sftpfs.go @@ -17,6 +17,7 @@ package vfs import ( "bufio" "bytes" + "crypto/rsa" "errors" "fmt" "hash/fnv" @@ -179,6 +180,34 @@ func (c *SFTPFsConfig) validate() error { } else { c.Prefix = "/" } + return c.validatePrivateKey() +} + +func (c *SFTPFsConfig) validatePrivateKey() error { + if c.PrivateKey.IsPlain() { + var signer ssh.Signer + var err error + if c.KeyPassphrase.IsPlain() { + signer, err = ssh.ParsePrivateKeyWithPassphrase([]byte(c.PrivateKey.GetPayload()), + []byte(c.KeyPassphrase.GetPayload())) + } else { + signer, err = ssh.ParsePrivateKey([]byte(c.PrivateKey.GetPayload())) + } + if err != nil { + return util.NewI18nError(fmt.Errorf("invalid private key: %w", err), util.I18nErrorPrivKeyInvalid) + } + if key, ok := signer.PublicKey().(ssh.CryptoPublicKey); ok { + cryptoKey := key.CryptoPublicKey() + if rsaKey, ok := cryptoKey.(*rsa.PublicKey); ok { + if size := rsaKey.N.BitLen(); size < 2048 { + return util.NewI18nError( + fmt.Errorf("rsa key with size %d not accepted, minimum 2048", size), + util.I18nErrorKeySizeInvalid, + ) + } + } + } + } return nil } @@ -902,6 +931,14 @@ func (c *sftpConnection) OpenConnection() error { return c.openConnNoLock() } +func (c *sftpConnection) getSigner() (ssh.Signer, error) { + if c.config.KeyPassphrase.GetPayload() != "" { + return ssh.ParsePrivateKeyWithPassphrase([]byte(c.config.PrivateKey.GetPayload()), + []byte(c.config.KeyPassphrase.GetPayload())) + } + return ssh.ParsePrivateKey([]byte(c.config.PrivateKey.GetPayload())) +} + func (c *sftpConnection) openConnNoLock() error { if c.isConnected { logger.Debug(c.logSender, "", "reusing connection") @@ -940,14 +977,7 @@ func (c *sftpConnection) openConnNoLock() error { ClientVersion: fmt.Sprintf("SSH-2.0-SFTPGo_%v", version.Get().Version), } if c.config.PrivateKey.GetPayload() != "" { - var signer ssh.Signer - var err error - if c.config.KeyPassphrase.GetPayload() != "" { - signer, err = ssh.ParsePrivateKeyWithPassphrase([]byte(c.config.PrivateKey.GetPayload()), - []byte(c.config.KeyPassphrase.GetPayload())) - } else { - signer, err = ssh.ParsePrivateKey([]byte(c.config.PrivateKey.GetPayload())) - } + signer, err := c.getSigner() if err != nil { return fmt.Errorf("sftpfs: unable to parse the private key: %w", err) } diff --git a/static/locales/en/translation.json b/static/locales/en/translation.json index 9293fd1a..e6758e7b 100644 --- a/static/locales/en/translation.json +++ b/static/locales/en/translation.json @@ -489,6 +489,8 @@ "home_required": "The home directory is mandatory", "home_invalid": "The home directory must be an absolute path", "pub_key_invalid": "Invalid public key", + "priv_key_invalid": "Invalid private key", + "key_invalid_size": "Invalid RSA key: the minimum supported size is 2048", "err_primary_group": "Only one primary group is allowed", "err_duplicate_group": "Duplicate groups detected", "no_permissions": "Directories permissions are mandatory", diff --git a/static/locales/it/translation.json b/static/locales/it/translation.json index 1b6ef580..6463bf80 100644 --- a/static/locales/it/translation.json +++ b/static/locales/it/translation.json @@ -489,6 +489,8 @@ "home_required": "La directory principale è obbligatoria", "home_invalid": "La directory principale deve essere un path assoluto", "pub_key_invalid": "Chiave pubblica non valida", + "priv_key_invalid": "Chiave privata non valida", + "key_invalid_size": "Chiave RSA non valida: la dimensione minima supportata è 2048", "err_primary_group": "È consentito un solo gruppo primario", "err_duplicate_group": "Rilevati gruppi duplicati", "no_permissions": "I permessi per le directory sono obbligatori",