From 2f5637512167db7486dd6c00d234fc0942be7a82 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Thu, 1 Apr 2021 18:53:48 +0200 Subject: [PATCH] improve SFTP loop detection --- .golangci.yml | 1 - common/connection.go | 5 ++ common/protocol_test.go | 61 ++++++++++++++ dataprovider/dataprovider.go | 18 ---- dataprovider/user.go | 42 +++++++++- docs/virtual-folders.md | 2 + httpd/httpd_test.go | 41 --------- sftpd/server.go | 7 ++ sftpd/sftpd_test.go | 157 ++++++++++++++++++++++++++++++++++- utils/utils.go | 9 +- vfs/folder.go | 6 +- vfs/sftpfs.go | 17 +++- vfs/vfs.go | 6 ++ webdavd/webdavd_test.go | 56 +++++++++++++ 14 files changed, 353 insertions(+), 75 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 7fca1f1f..350a19ac 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -37,6 +37,5 @@ linters: - maligned - whitespace - dupl - - scopelint - rowserrcheck - dogsled \ No newline at end of file diff --git a/common/connection.go b/common/connection.go index add73d4a..cfb6159a 100644 --- a/common/connection.go +++ b/common/connection.go @@ -1093,6 +1093,11 @@ func (c *BaseConnection) GetFsError(fs vfs.Fs, err error) error { func (c *BaseConnection) GetFsAndResolvedPath(virtualPath string) (vfs.Fs, string, error) { fs, err := c.User.GetFilesystemForPath(virtualPath, c.ID) if err != nil { + if c.protocol == ProtocolWebDAV && strings.Contains(err.Error(), vfs.ErrSFTPLoop.Error()) { + // if there is an SFTP loop we return a permission error, for WebDAV, so the problematic folder + // will not be listed + return nil, "", c.GetPermissionDeniedError() + } return nil, "", err } diff --git a/common/protocol_test.go b/common/protocol_test.go index 79c357a8..c1f7d3a7 100644 --- a/common/protocol_test.go +++ b/common/protocol_test.go @@ -2085,6 +2085,67 @@ func TestRenameSymlink(t *testing.T) { assert.NoError(t, err) } +func TestSFTPLoopError(t *testing.T) { + user1 := getTestUser() + user2 := getTestUser() + user1.Username += "1" + user2.Username += "2" + // user1 is a local account with a virtual SFTP folder to user2 + // user2 has user1 as SFTP fs + user1.VirtualFolders = append(user1.VirtualFolders, vfs.VirtualFolder{ + BaseVirtualFolder: vfs.BaseVirtualFolder{ + Name: "sftp", + FsConfig: vfs.Filesystem{ + Provider: vfs.SFTPFilesystemProvider, + SFTPConfig: vfs.SFTPFsConfig{ + Endpoint: sftpServerAddr, + Username: user2.Username, + Password: kms.NewPlainSecret(defaultPassword), + }, + }, + }, + VirtualPath: "/vdir", + }) + + user2.FsConfig.Provider = vfs.SFTPFilesystemProvider + user2.FsConfig.SFTPConfig = vfs.SFTPFsConfig{ + Endpoint: sftpServerAddr, + Username: user1.Username, + Password: kms.NewPlainSecret(defaultPassword), + } + + user1, resp, err := httpdtest.AddUser(user1, http.StatusCreated) + assert.NoError(t, err, string(resp)) + user2, resp, err = httpdtest.AddUser(user2, http.StatusCreated) + assert.NoError(t, err, string(resp)) + + user1.VirtualFolders[0].FsConfig.SFTPConfig.Password = kms.NewPlainSecret(defaultPassword) + user2.FsConfig.SFTPConfig.Password = kms.NewPlainSecret(defaultPassword) + + conn := common.NewBaseConnection("", common.ProtocolWebDAV, user1) + _, _, err = conn.GetFsAndResolvedPath(user1.VirtualFolders[0].VirtualPath) + assert.ErrorIs(t, err, os.ErrPermission) + + conn = common.NewBaseConnection("", common.ProtocolSFTP, user1) + _, _, err = conn.GetFsAndResolvedPath(user1.VirtualFolders[0].VirtualPath) + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "SFTP loop") + } + conn = common.NewBaseConnection("", common.ProtocolFTP, user1) + _, _, err = conn.GetFsAndResolvedPath(user1.VirtualFolders[0].VirtualPath) + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "SFTP loop") + } + _, err = httpdtest.RemoveUser(user1, http.StatusOK) + assert.NoError(t, err) + err = os.RemoveAll(user1.GetHomeDir()) + assert.NoError(t, err) + _, err = httpdtest.RemoveUser(user2, http.StatusOK) + assert.NoError(t, err) + err = os.RemoveAll(user2.GetHomeDir()) + assert.NoError(t, err) +} + func TestNonLocalCrossRename(t *testing.T) { baseUser, resp, err := httpdtest.AddUser(getTestUser(), http.StatusCreated) assert.NoError(t, err, string(resp)) diff --git a/dataprovider/dataprovider.go b/dataprovider/dataprovider.go index 89558f1a..67f0b0ca 100644 --- a/dataprovider/dataprovider.go +++ b/dataprovider/dataprovider.go @@ -1081,16 +1081,6 @@ func getVirtualFolderIfInvalid(folder *vfs.BaseVirtualFolder) *vfs.BaseVirtualFo return folder } -func hasSFTPLoop(user *User, fs *vfs.Filesystem) bool { - if fs.Provider == vfs.SFTPFilesystemProvider { - // FIXME: this could be inaccurate, it is not easy to check the endpoint too - if fs.SFTPConfig.Username == user.Username { - return true - } - } - return false -} - func validateUserVirtualFolders(user *User) error { if len(user.VirtualFolders) == 0 { user.VirtualFolders = []vfs.VirtualFolder{} @@ -1111,10 +1101,6 @@ func validateUserVirtualFolders(user *User) error { if err := ValidateFolder(folder); err != nil { return err } - if hasSFTPLoop(user, &folder.FsConfig) { - return &ValidationError{err: fmt.Sprintf("SFTP folder %#v could point to the same SFTPGo account, this is not allowed", - folder.Name)} - } cleanedMPath := folder.MappedPath if folder.IsLocalOrLocalCrypted() { if isMappedDirOverlapped(cleanedMPath, user.GetHomeDir(), true) { @@ -1527,10 +1513,6 @@ func ValidateUser(user *User) error { if err := validateFilesystemConfig(&user.FsConfig, user); err != nil { return err } - if hasSFTPLoop(user, &user.FsConfig) { - return &ValidationError{err: fmt.Sprintf("SFTP fs for user %#v could point to the same SFTPGo account, this is not allowed", - user.Username)} - } if err := validateUserVirtualFolders(user); err != nil { return err } diff --git a/dataprovider/user.go b/dataprovider/user.go index 15eb79b6..ac7bd9b8 100644 --- a/dataprovider/user.go +++ b/dataprovider/user.go @@ -225,7 +225,12 @@ func (u *User) getRootFs(connectionID string) (fs vfs.Fs, err error) { case vfs.CryptedFilesystemProvider: return vfs.NewCryptFs(connectionID, u.GetHomeDir(), "", u.FsConfig.CryptConfig) case vfs.SFTPFilesystemProvider: - return vfs.NewSFTPFs(connectionID, "", u.FsConfig.SFTPConfig) + forbiddenSelfUsers, err := u.getForbiddenSFTPSelfUsers(u.FsConfig.SFTPConfig.Username) + if err != nil { + return nil, err + } + forbiddenSelfUsers = append(forbiddenSelfUsers, u.Username) + return vfs.NewSFTPFs(connectionID, "", forbiddenSelfUsers, u.FsConfig.SFTPConfig) default: return vfs.NewOsFs(connectionID, u.GetHomeDir(), ""), nil } @@ -431,6 +436,31 @@ func (u *User) GetPermissionsForPath(p string) []string { return permissions } +func (u *User) getForbiddenSFTPSelfUsers(username string) ([]string, error) { + sftpUser, err := UserExists(username) + if err == nil { + // we don't allow local nested SFTP folders + var forbiddens []string + if sftpUser.FsConfig.Provider == vfs.SFTPFilesystemProvider { + forbiddens = append(forbiddens, sftpUser.Username) + return forbiddens, nil + } + for idx := range sftpUser.VirtualFolders { + v := &sftpUser.VirtualFolders[idx] + if v.FsConfig.Provider == vfs.SFTPFilesystemProvider { + forbiddens = append(forbiddens, sftpUser.Username) + return forbiddens, nil + } + } + return forbiddens, nil + } + if _, ok := err.(*RecordNotFoundError); !ok { + return nil, err + } + + return nil, nil +} + // GetFilesystemForPath returns the filesystem for the given path func (u *User) GetFilesystemForPath(virtualPath, connectionID string) (vfs.Fs, error) { if u.fsCache == nil { @@ -442,7 +472,15 @@ func (u *User) GetFilesystemForPath(virtualPath, connectionID string) (vfs.Fs, e if fs, ok := u.fsCache[folder.VirtualPath]; ok { return fs, nil } - fs, err := folder.GetFilesystem(connectionID) + forbiddenSelfUsers := []string{u.Username} + if folder.FsConfig.Provider == vfs.SFTPFilesystemProvider { + forbiddens, err := u.getForbiddenSFTPSelfUsers(folder.FsConfig.SFTPConfig.Username) + if err != nil { + return nil, err + } + forbiddenSelfUsers = append(forbiddenSelfUsers, forbiddens...) + } + fs, err := folder.GetFilesystem(connectionID, forbiddenSelfUsers) if err == nil { u.fsCache[folder.VirtualPath] = fs } diff --git a/docs/virtual-folders.md b/docs/virtual-folders.md index cf6d9459..2b76def5 100644 --- a/docs/virtual-folders.md +++ b/docs/virtual-folders.md @@ -18,6 +18,8 @@ For each virtual folder, the following properties can be configured: For example if the configure folder has configured `/tmp/mapped` or `C:\mapped` as filesystem path and you set `/vfolder` as virtual path then SFTPGo users can access `/tmp/mapped` or `C:\mapped` via the `/vfolder` virtual path. +Nested SFTP folders using the same SFTPGo instance (identified using the host keys) are not allowed as they could cause infinite SFTP loops. + The same virtual folder can be shared among users, different folder quota limits for each user are supported. Folder quota limits can also be included inside the user quota but in this case the folder is considered "private" and sharing it with other users will break user quota calculation. The calculation of the quota for a given user is obtained as the sum of the files contained in his home directory and those within each defined virtual folder. diff --git a/httpd/httpd_test.go b/httpd/httpd_test.go index 0eef211a..b714ed45 100644 --- a/httpd/httpd_test.go +++ b/httpd/httpd_test.go @@ -738,21 +738,6 @@ func TestUserRedactedPassword(t *testing.T) { assert.NoError(t, err) } -func TestSFTPSelf(t *testing.T) { - u := getTestUser() - u.FsConfig = vfs.Filesystem{ - Provider: vfs.SFTPFilesystemProvider, - SFTPConfig: vfs.SFTPFsConfig{ - Endpoint: "localhost:2022", - Username: defaultUsername, - Password: kms.NewPlainSecret(defaultPassword), - }, - } - _, resp, err := httpdtest.AddUser(u, http.StatusBadRequest) - assert.NoError(t, err, string(resp)) - assert.Contains(t, string(resp), "could point to the same SFTPGo account") -} - func TestAddUserInvalidVirtualFolders(t *testing.T) { u := getTestUser() folderName := "fname" @@ -909,32 +894,6 @@ func TestAddUserInvalidVirtualFolders(t *testing.T) { assert.NoError(t, err) } -func TestSFTPVirtualFolderSelf(t *testing.T) { - // an sftp virtual folder cannot use the same sftp account, it will generate an infinite loop - // at login - u := getTestUser() - mappedPathSFTP := filepath.Join(os.TempDir(), "sftp") - folderNameSFTP := filepath.Base(mappedPathSFTP) - vdirSFTPPath := "/vdir/sftp" - u.VirtualFolders = append(u.VirtualFolders, vfs.VirtualFolder{ - BaseVirtualFolder: vfs.BaseVirtualFolder{ - Name: folderNameSFTP, - FsConfig: vfs.Filesystem{ - Provider: vfs.SFTPFilesystemProvider, - SFTPConfig: vfs.SFTPFsConfig{ - Endpoint: "127.0.0.1:2022", - Username: defaultUsername, - Password: kms.NewPlainSecret(defaultPassword), - }, - }, - }, - VirtualPath: vdirSFTPPath, - }) - _, resp, err := httpdtest.AddUser(u, http.StatusBadRequest) - assert.NoError(t, err, string(resp)) - assert.Contains(t, string(resp), "could point to the same SFTPGo account") -} - func TestUserPublicKey(t *testing.T) { u := getTestUser() u.Password = "" diff --git a/sftpd/server.go b/sftpd/server.go index a188e5b5..f8799dc7 100644 --- a/sftpd/server.go +++ b/sftpd/server.go @@ -21,6 +21,7 @@ import ( "github.com/drakkan/sftpgo/logger" "github.com/drakkan/sftpgo/metrics" "github.com/drakkan/sftpgo/utils" + "github.com/drakkan/sftpgo/vfs" ) const ( @@ -711,6 +712,12 @@ func (c *Configuration) checkAndLoadHostKeys(configDir string, serverConfig *ssh // Add private key to the server configuration. serverConfig.AddHostKey(private) } + var fp []string + for idx := range serviceStatus.HostKeys { + h := &serviceStatus.HostKeys[idx] + fp = append(fp, h.Fingerprint) + } + vfs.SetSFTPFingerprints(fp) return nil } diff --git a/sftpd/sftpd_test.go b/sftpd/sftpd_test.go index c3a7c28d..856ba2d9 100644 --- a/sftpd/sftpd_test.go +++ b/sftpd/sftpd_test.go @@ -3439,9 +3439,160 @@ func TestVirtualFoldersQuotaLimit(t *testing.T) { assert.NoError(t, err) } -func TestNestedVirtualFolders(t *testing.T) { +func TestSFTPLoopSimple(t *testing.T) { usePubKey := false - baseUser, resp, err := httpdtest.AddUser(getTestUser(false), http.StatusCreated) + user1 := getTestSFTPUser(usePubKey) + user2 := getTestSFTPUser(usePubKey) + user1.Username += "1" + user2.Username += "2" + user1.FsConfig.Provider = vfs.SFTPFilesystemProvider + user2.FsConfig.Provider = vfs.SFTPFilesystemProvider + user1.FsConfig.SFTPConfig = vfs.SFTPFsConfig{ + Endpoint: sftpServerAddr, + Username: user2.Username, + Password: kms.NewPlainSecret(defaultPassword), + } + user2.FsConfig.SFTPConfig = vfs.SFTPFsConfig{ + Endpoint: sftpServerAddr, + Username: user1.Username, + Password: kms.NewPlainSecret(defaultPassword), + } + user1, resp, err := httpdtest.AddUser(user1, http.StatusCreated) + assert.NoError(t, err, string(resp)) + user2, resp, err = httpdtest.AddUser(user2, http.StatusCreated) + assert.NoError(t, err, string(resp)) + + _, err = getSftpClient(user1, usePubKey) + assert.Error(t, err) + _, err = getSftpClient(user2, usePubKey) + assert.Error(t, err) + + user1.FsConfig.SFTPConfig.Username = user1.Username + user1.FsConfig.SFTPConfig.Password = kms.NewPlainSecret(defaultPassword) + + _, _, err = httpdtest.UpdateUser(user1, http.StatusOK, "") + assert.NoError(t, err) + _, err = getSftpClient(user1, usePubKey) + assert.Error(t, err) + + _, err = httpdtest.RemoveUser(user1, http.StatusOK) + assert.NoError(t, err) + err = os.RemoveAll(user1.GetHomeDir()) + assert.NoError(t, err) + _, err = httpdtest.RemoveUser(user2, http.StatusOK) + assert.NoError(t, err) + err = os.RemoveAll(user2.GetHomeDir()) + assert.NoError(t, err) +} + +func TestSFTPLoopVirtualFolders(t *testing.T) { + usePubKey := false + user1 := getTestUser(usePubKey) + user2 := getTestSFTPUser(usePubKey) + user3 := getTestSFTPUser(usePubKey) + user1.Username += "1" + user2.Username += "2" + user3.Username += "3" + + // user1 is a local account with a virtual SFTP folder to user2 + // user2 has user1 as SFTP fs + user1.VirtualFolders = append(user1.VirtualFolders, vfs.VirtualFolder{ + BaseVirtualFolder: vfs.BaseVirtualFolder{ + Name: "sftp", + FsConfig: vfs.Filesystem{ + Provider: vfs.SFTPFilesystemProvider, + SFTPConfig: vfs.SFTPFsConfig{ + Endpoint: sftpServerAddr, + Username: user2.Username, + Password: kms.NewPlainSecret(defaultPassword), + }, + }, + }, + VirtualPath: "/vdir", + }) + + user2.FsConfig.Provider = vfs.SFTPFilesystemProvider + user2.FsConfig.SFTPConfig = vfs.SFTPFsConfig{ + Endpoint: sftpServerAddr, + Username: user1.Username, + Password: kms.NewPlainSecret(defaultPassword), + } + user3.FsConfig.Provider = vfs.SFTPFilesystemProvider + user3.FsConfig.SFTPConfig = vfs.SFTPFsConfig{ + Endpoint: sftpServerAddr, + Username: user1.Username, + Password: kms.NewPlainSecret(defaultPassword), + } + + user1, resp, err := httpdtest.AddUser(user1, http.StatusCreated) + assert.NoError(t, err, string(resp)) + user2, resp, err = httpdtest.AddUser(user2, http.StatusCreated) + assert.NoError(t, err, string(resp)) + user3, resp, err = httpdtest.AddUser(user3, http.StatusCreated) + assert.NoError(t, err, string(resp)) + + // login will work but /vdir will not be accessible + client, err := getSftpClient(user1, usePubKey) + if assert.NoError(t, err) { + defer client.Close() + assert.NoError(t, checkBasicSFTP(client)) + _, err = client.ReadDir("/vdir") + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "SFTP loop") + } + } + // now make user2 a local account with an SFTP virtual folder to user1. + // So we have: + // user1 -> local account with the SFTP virtual folder /vdir to user2 + // user2 -> local account with the SFTP virtual folder /vdir2 to user3 + // user3 -> sftp user with user1 as fs + user2.FsConfig.Provider = vfs.LocalFilesystemProvider + user2.FsConfig.SFTPConfig = vfs.SFTPFsConfig{} + user2.VirtualFolders = append(user2.VirtualFolders, vfs.VirtualFolder{ + BaseVirtualFolder: vfs.BaseVirtualFolder{ + Name: "sftp", + FsConfig: vfs.Filesystem{ + Provider: vfs.SFTPFilesystemProvider, + SFTPConfig: vfs.SFTPFsConfig{ + Endpoint: sftpServerAddr, + Username: user3.Username, + Password: kms.NewPlainSecret(defaultPassword), + }, + }, + }, + VirtualPath: "/vdir2", + }) + user2, _, err = httpdtest.UpdateUser(user2, http.StatusOK, "") + assert.NoError(t, err) + + // login will work but /vdir will not be accessible + client, err = getSftpClient(user1, usePubKey) + if assert.NoError(t, err) { + defer client.Close() + assert.NoError(t, checkBasicSFTP(client)) + _, err = client.ReadDir("/vdir") + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "SFTP loop") + } + } + + _, err = httpdtest.RemoveUser(user1, http.StatusOK) + assert.NoError(t, err) + err = os.RemoveAll(user1.GetHomeDir()) + assert.NoError(t, err) + _, err = httpdtest.RemoveUser(user2, http.StatusOK) + assert.NoError(t, err) + err = os.RemoveAll(user2.GetHomeDir()) + assert.NoError(t, err) + _, err = httpdtest.RemoveUser(user3, http.StatusOK) + assert.NoError(t, err) + err = os.RemoveAll(user3.GetHomeDir()) + assert.NoError(t, err) +} + +func TestNestedVirtualFolders(t *testing.T) { + usePubKey := true + baseUser, resp, err := httpdtest.AddUser(getTestUser(usePubKey), http.StatusCreated) assert.NoError(t, err, string(resp)) u := getTestSFTPUser(usePubKey) u.QuotaFiles = 1000 @@ -6049,7 +6200,7 @@ func TestRelativePaths(t *testing.T) { Password: kms.NewPlainSecret(defaultPassword), Prefix: keyPrefix, } - sftpfs, _ := vfs.NewSFTPFs("", "", sftpconfig) + sftpfs, _ := vfs.NewSFTPFs("", "", []string{user.Username}, sftpconfig) if runtime.GOOS != osWindows { filesystems = append(filesystems, s3fs, gcsfs, sftpfs) } diff --git a/utils/utils.go b/utils/utils.go index 29cc2795..57ef2fe5 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -474,9 +474,10 @@ func EncodeTLSCertToPem(tlsCert *x509.Certificate) (string, error) { return string(pem.EncodeToMemory(&publicKeyBlock)), nil } -// CheckTCP4Port quits the app if bind to the given IPv4 port. +// CheckTCP4Port quits the app if bind on the given IPv4 port fails. // This is a ugly hack to avoid to bind on an already used port. -// It is required on Windows only. +// It is required on Windows only. Upstream does not consider this +// behaviour a bug: // https://github.com/golang/go/issues/45150 func CheckTCP4Port(port int) { if runtime.GOOS != osWindows { @@ -484,8 +485,8 @@ func CheckTCP4Port(port int) { } listener, err := net.Listen("tcp4", fmt.Sprintf(":%d", port)) if err != nil { - logger.ErrorToConsole("unable to bind tcp4 address: %v", err) - logger.Error(logSender, "", "unable to bind tcp4 address: %v", err) + logger.ErrorToConsole("unable to bind on tcp4 address: %v", err) + logger.Error(logSender, "", "unable to bind on tcp4 address: %v", err) os.Exit(1) } listener.Close() diff --git a/vfs/folder.go b/vfs/folder.go index 9258ae22..54b6b815 100644 --- a/vfs/folder.go +++ b/vfs/folder.go @@ -170,7 +170,7 @@ type VirtualFolder struct { } // GetFilesystem returns the filesystem for this folder -func (v *VirtualFolder) GetFilesystem(connectionID string) (Fs, error) { +func (v *VirtualFolder) GetFilesystem(connectionID string, forbiddenSelfUsers []string) (Fs, error) { switch v.FsConfig.Provider { case S3FilesystemProvider: return NewS3Fs(connectionID, v.MappedPath, v.VirtualPath, v.FsConfig.S3Config) @@ -183,7 +183,7 @@ func (v *VirtualFolder) GetFilesystem(connectionID string) (Fs, error) { case CryptedFilesystemProvider: return NewCryptFs(connectionID, v.MappedPath, v.VirtualPath, v.FsConfig.CryptConfig) case SFTPFilesystemProvider: - return NewSFTPFs(connectionID, v.VirtualPath, v.FsConfig.SFTPConfig) + return NewSFTPFs(connectionID, v.VirtualPath, forbiddenSelfUsers, v.FsConfig.SFTPConfig) default: return NewOsFs(connectionID, v.MappedPath, v.VirtualPath), nil } @@ -191,7 +191,7 @@ func (v *VirtualFolder) GetFilesystem(connectionID string) (Fs, error) { // ScanQuota scans the folder and returns the number of files and their size func (v *VirtualFolder) ScanQuota() (int, int64, error) { - fs, err := v.GetFilesystem("") + fs, err := v.GetFilesystem("", nil) if err != nil { return 0, 0, err } diff --git a/vfs/sftpfs.go b/vfs/sftpfs.go index c75a8331..e7feb456 100644 --- a/vfs/sftpfs.go +++ b/vfs/sftpfs.go @@ -29,6 +29,8 @@ const ( sftpFsName = "sftpfs" ) +var ErrSFTPLoop = errors.New("SFTP loop or nested local SFTP folders detected") + // SFTPFsConfig defines the configuration for SFTP based filesystem type SFTPFsConfig struct { Endpoint string `json:"endpoint,omitempty"` @@ -41,7 +43,8 @@ type SFTPFsConfig struct { // Concurrent reads are safe to use and disabling them will degrade performance. // Some servers automatically delete files once they are downloaded. // Using concurrent reads is problematic with such servers. - DisableCouncurrentReads bool `json:"disable_concurrent_reads,omitempty"` + DisableCouncurrentReads bool `json:"disable_concurrent_reads,omitempty"` + forbiddenSelfUsernames []string `json:"-"` } func (c *SFTPFsConfig) isEqual(other *SFTPFsConfig) bool { @@ -148,7 +151,7 @@ type SFTPFs struct { } // NewSFTPFs returns an SFTPFa object that allows to interact with an SFTP server -func NewSFTPFs(connectionID, mountPath string, config SFTPFsConfig) (Fs, error) { +func NewSFTPFs(connectionID, mountPath string, forbiddenSelfUsernames []string, config SFTPFsConfig) (Fs, error) { if err := config.Validate(); err != nil { return nil, err } @@ -162,6 +165,7 @@ func NewSFTPFs(connectionID, mountPath string, config SFTPFsConfig) (Fs, error) return nil, err } } + config.forbiddenSelfUsernames = forbiddenSelfUsernames sftpFs := &SFTPFs{ connectionID: connectionID, mountPath: mountPath, @@ -607,8 +611,15 @@ func (fs *SFTPFs) createConnection() error { clientConfig := &ssh.ClientConfig{ User: fs.config.Username, HostKeyCallback: func(hostname string, remote net.Addr, key ssh.PublicKey) error { + fp := ssh.FingerprintSHA256(key) + if utils.IsStringInSlice(fp, sftpFingerprints) { + if utils.IsStringInSlice(fs.config.Username, fs.config.forbiddenSelfUsernames) { + fsLog(fs, logger.LevelWarn, "SFTP loop or nested local SFTP folders detected, mount path %#v, username %#v, forbidden usernames: %+v", + fs.mountPath, fs.config.Username, fs.config.forbiddenSelfUsernames) + return ErrSFTPLoop + } + } if len(fs.config.Fingerprints) > 0 { - fp := ssh.FingerprintSHA256(key) for _, provided := range fs.config.Fingerprints { if provided == fp { return nil diff --git a/vfs/vfs.go b/vfs/vfs.go index 39b56f6e..91e4d035 100644 --- a/vfs/vfs.go +++ b/vfs/vfs.go @@ -30,6 +30,7 @@ var ( // ErrVfsUnsupported defines the error for an unsupported VFS operation ErrVfsUnsupported = errors.New("not supported") credentialsDirPath string + sftpFingerprints []string ) // SetCredentialsDirPath sets the credentials dir path @@ -42,6 +43,11 @@ func GetCredentialsDirPath() string { return credentialsDirPath } +// SetSFTPFingerprints sets the SFTP host key fingerprints +func SetSFTPFingerprints(fp []string) { + sftpFingerprints = fp +} + // Fs defines the interface for filesystem backends type Fs interface { Name() string diff --git a/webdavd/webdavd_test.go b/webdavd/webdavd_test.go index 668eb708..d4adffd6 100644 --- a/webdavd/webdavd_test.go +++ b/webdavd/webdavd_test.go @@ -2071,6 +2071,62 @@ func TestPreLoginHookWithClientCert(t *testing.T) { assert.NoError(t, err) } +func TestSFTPLoopVirtualFolders(t *testing.T) { + user1 := getTestUser() + user2 := getTestUser() + user1.Username += "1" + user2.Username += "2" + // user1 is a local account with a virtual SFTP folder to user2 + // user2 has user1 as SFTP fs + user1.VirtualFolders = append(user1.VirtualFolders, vfs.VirtualFolder{ + BaseVirtualFolder: vfs.BaseVirtualFolder{ + Name: "sftp", + FsConfig: vfs.Filesystem{ + Provider: vfs.SFTPFilesystemProvider, + SFTPConfig: vfs.SFTPFsConfig{ + Endpoint: sftpServerAddr, + Username: user2.Username, + Password: kms.NewPlainSecret(defaultPassword), + }, + }, + }, + VirtualPath: "/vdir", + }) + user2.FsConfig.Provider = vfs.SFTPFilesystemProvider + user2.FsConfig.SFTPConfig = vfs.SFTPFsConfig{ + Endpoint: sftpServerAddr, + Username: user1.Username, + Password: kms.NewPlainSecret(defaultPassword), + } + + user1, resp, err := httpdtest.AddUser(user1, http.StatusCreated) + assert.NoError(t, err, string(resp)) + user2, resp, err = httpdtest.AddUser(user2, http.StatusCreated) + assert.NoError(t, err, string(resp)) + + client := getWebDavClient(user1, true, nil) + + testDir := "tdir" + err = client.Mkdir(testDir, os.ModePerm) + assert.NoError(t, err) + + contents, err := client.ReadDir("/") + assert.NoError(t, err) + if assert.Len(t, contents, 1) { + assert.Equal(t, testDir, contents[0].Name()) + assert.True(t, contents[0].IsDir()) + } + + _, err = httpdtest.RemoveUser(user1, http.StatusOK) + assert.NoError(t, err) + err = os.RemoveAll(user1.GetHomeDir()) + assert.NoError(t, err) + _, err = httpdtest.RemoveUser(user2, http.StatusOK) + assert.NoError(t, err) + err = os.RemoveAll(user2.GetHomeDir()) + assert.NoError(t, err) +} + func TestNestedVirtualFolders(t *testing.T) { u := getTestUser() localUser, _, err := httpdtest.AddUser(u, http.StatusCreated)