From cf541d62ea532cca0e021db5883b8eb5cba00eda Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Fri, 26 Jun 2020 23:38:29 +0200 Subject: [PATCH] recursive permissions check before renaming/copying directories --- dataprovider/user.go | 17 ++++- docs/account.md | 2 +- docs/build-from-source.md | 4 +- docs/full-configuration.md | 2 +- docs/portable-mode.md | 2 +- docs/ssh-commands.md | 2 +- httpd/httpd_test.go | 1 + service/service_portable.go | 5 +- sftpd/handler.go | 110 +++++++++++++++++++++-------- sftpd/internal_test.go | 107 ++++++++++++++++++++++++---- sftpd/sftpd_test.go | 136 +++++++++++++++++++++++++++++++----- sftpd/ssh_cmd.go | 77 +++++++++++++++++--- utils/utils.go | 2 +- vfs/gcsfs.go | 9 ++- vfs/osfs.go | 6 ++ vfs/s3fs.go | 9 ++- vfs/vfs.go | 4 ++ 17 files changed, 414 insertions(+), 81 deletions(-) diff --git a/dataprovider/user.go b/dataprovider/user.go index 1b9dc848..31eee546 100644 --- a/dataprovider/user.go +++ b/dataprovider/user.go @@ -259,7 +259,7 @@ func (u *User) IsVirtualFolder(sftpPath string) bool { return false } -// HasVirtualFoldersInside return true if there are virtual folders inside the +// HasVirtualFoldersInside returns true if there are virtual folders inside the // specified SFTP path. We assume that path are cleaned func (u *User) HasVirtualFoldersInside(sftpPath string) bool { if sftpPath == "/" && len(u.VirtualFolders) > 0 { @@ -275,6 +275,21 @@ func (u *User) HasVirtualFoldersInside(sftpPath string) bool { return false } +// HasPermissionsInside returns true if the specified sftpPath has no permissions itself and +// no subdirs with defined permissions +func (u *User) HasPermissionsInside(sftpPath string) bool { + for dir := range u.Permissions { + if dir == sftpPath { + return true + } else if len(dir) > len(sftpPath) { + if strings.HasPrefix(dir, sftpPath+"/") { + return true + } + } + } + return false +} + // HasOverlappedMappedPaths returns true if this user has virtual folders with overlapped mapped paths func (u *User) HasOverlappedMappedPaths() bool { if len(u.VirtualFolders) <= 1 { diff --git a/docs/account.md b/docs/account.md index 453060d5..f9c566cb 100644 --- a/docs/account.md +++ b/docs/account.md @@ -20,7 +20,7 @@ For each account, the following properties can be configured: - `upload` upload files is allowed - `overwrite` overwrite an existing file, while uploading, is allowed. `upload` permission is required to allow file overwrite - `delete` delete files or directories is allowed - - `rename` rename a file or a directory is allowed if this permission is granted on target path. You can enable rename in a more controlled way granting `delete` permission on source directory and `upload`/`create_dirs` permissions on target directory. Please be aware that no subdir permission is checked for the directories rename case + - `rename` rename a file or a directory is allowed if this permission is granted on source and target path. You can enable rename in a more controlled way granting `delete` permission on source directory and `upload`/`create_dirs`/`create_symlinks` permissions on target directory - `create_dirs` create directories is allowed - `create_symlinks` create symbolic links is allowed - `chmod` changing file or directory permissions is allowed. On Windows, only the 0200 bit (owner writable) of mode is used; it controls whether the file's read-only attribute is set or cleared. The other bits are currently unused. Use mode 0400 for a read-only file and 0600 for a readable+writable file. diff --git a/docs/build-from-source.md b/docs/build-from-source.md index c7b906cc..292a9748 100644 --- a/docs/build-from-source.md +++ b/docs/build-from-source.md @@ -1,11 +1,13 @@ # Build SFTPGo from source -Install the package to your [\$GOPATH](https://github.com/golang/go/wiki/GOPATH "GOPATH") with the [go tool](https://golang.org/cmd/go/ "go command") from shell: +You can install the package to your [\$GOPATH](https://github.com/golang/go/wiki/GOPATH "GOPATH") with the [go tool](https://golang.org/cmd/go/ "go command") from shell: ```bash go get -u github.com/drakkan/sftpgo ``` +Or you can download the sources and use `go build`. + Make sure [Git](https://git-scm.com/downloads) is installed on your machine and in your system's `PATH`. The following build tags are available: diff --git a/docs/full-configuration.md b/docs/full-configuration.md index d875d7c6..fb71c429 100644 --- a/docs/full-configuration.md +++ b/docs/full-configuration.md @@ -32,7 +32,7 @@ The `serve` command supports the following flags: - `--log-max-size` int. Maximum size in megabytes of the log file before it gets rotated. Default 10 or the value of `SFTPGO_LOG_MAX_SIZE` environment variable. It is unused if `log-file-path` is empty. - `--log-verbose` boolean. Enable verbose logs. Default `true` or the value of `SFTPGO_LOG_VERBOSE` environment variable (1 or `true`, 0 or `false`). -Log file can be rotated on demand sending a `SIGUSR1` signal on Unix based systems and using `sftpgo service rotatelogs` on Windows. +Log file can be rotated on demand sending a `SIGUSR1` signal on Unix based systems and using the command `sftpgo service rotatelogs` on Windows. If you don't configure any private host key, the daemon will use `id_rsa` and `id_ecdsa` in the configuration directory. If these files don't exist, the daemon will attempt to autogenerate them (if the user that executes SFTPGo has write access to the `config-dir`). The server supports any private key format supported by [`crypto/ssh`](https://github.com/golang/crypto/blob/master/ssh/keys.go#L33). diff --git a/docs/portable-mode.md b/docs/portable-mode.md index 8953d53b..c9b49ae0 100644 --- a/docs/portable-mode.md +++ b/docs/portable-mode.md @@ -15,7 +15,7 @@ Usage: Flags: -C, --advertise-credentials If the SFTP service is advertised via multicast DNS, this flag allows to put username/password inside the advertised TXT record - -S, --advertise-service Advertise SFTP service using multicast DNS (default true) + -S, --advertise-service Advertise SFTP service using multicast DNS --allowed-extensions stringArray Allowed file extensions case insensitive. The format is /dir::ext1,ext2. For example: "/somedir::.jpg,.png" --denied-extensions stringArray Denied file extensions case insensitive. The format is /dir::ext1,ext2. For example: "/somedir::.jpg,.png" -d, --directory string Path to the directory to serve. This can be an absolute path or a path relative to the current directory (default ".") diff --git a/docs/ssh-commands.md b/docs/ssh-commands.md index 5b87983f..29173d1b 100644 --- a/docs/ssh-commands.md +++ b/docs/ssh-commands.md @@ -21,7 +21,7 @@ SFTPGo support the following built-in SSH commands: - `scp`, SFTPGo implements the SCP protocol so we can support it for cloud filesystems too and we can avoid the other system commands limitations. SCP between two remote hosts is supported using the `-3` scp option. - `md5sum`, `sha1sum`, `sha256sum`, `sha384sum`, `sha512sum`. Useful to check message digests for uploaded files. - `cd`, `pwd`. Some SFTP clients do not support the SFTP SSH_FXP_REALPATH packet type, so they use `cd` and `pwd` SSH commands to get the initial directory. Currently `cd` does nothing and `pwd` always returns the `/` path. -- `sftpgo-copy`. This is a built-in copy implementation. It allows server side copy for files and directories. The first argument is the source file/directory and the second one is the destination file/directory, for example `sftpgo-copy `. The command will fail if the destination directory exists. Copy for directories spanning virtual folders is not supported. Only local filesystem is supported: recursive copy for Cloud Storage filesystems requires a new request for every file in any case, so a server side copy is not possibile. Please be aware that only the `list` permission for the source path and the `upload` and `create_dirs` (for directories) permissions for the destination path are checked. +- `sftpgo-copy`. This is a built-in copy implementation. It allows server side copy for files and directories. The first argument is the source file/directory and the second one is the destination file/directory, for example `sftpgo-copy `. The command will fail if the destination exists. Copy for directories spanning virtual folders is not supported. Only local filesystem is supported: recursive copy for Cloud Storage filesystems requires a new request for every file in any case, so a real server side copy is not possibile. - `sftpgo-remove`. This is a built-in remove implementation. It allows to remove single files and to recursively remove directories. The first argument is the file/directory to remove, for example `sftpgo-remove `. Only local filesystem is supported: recursive remove for Cloud Storage filesystems requires a new request for every file in any case, so a server side remove is not possibile. The following SSH commands are enabled by default: diff --git a/httpd/httpd_test.go b/httpd/httpd_test.go index 6f4d09e7..2020a451 100644 --- a/httpd/httpd_test.go +++ b/httpd/httpd_test.go @@ -107,6 +107,7 @@ func TestMain(m *testing.M) { providerConf.CredentialsPath = credentialsPath providerDriverName = providerConf.Driver os.RemoveAll(credentialsPath) //nolint:errcheck + logger.InfoToConsole("Starting HTTPD tests, provider: %v", providerConf.Driver) err = dataprovider.Initialize(providerConf, configDir) if err != nil { diff --git a/service/service_portable.go b/service/service_portable.go index 662519a0..ab85901d 100644 --- a/service/service_portable.go +++ b/service/service_portable.go @@ -32,7 +32,10 @@ func (s *Service) StartPortableMode(sftpdPort int, enabledSSHCommands []string, if len(s.PortableUser.Username) == 0 { s.PortableUser.Username = "user" } - printablePassword := "[redacted]" + printablePassword := "" + if len(s.PortableUser.Password) > 0 { + printablePassword = "[redacted]" + } if len(s.PortableUser.PublicKeys) == 0 && len(s.PortableUser.Password) == 0 { var b strings.Builder for i := 0; i < 8; i++ { diff --git a/sftpd/handler.go b/sftpd/handler.go index f131c50d..5975d1f7 100644 --- a/sftpd/handler.go +++ b/sftpd/handler.go @@ -5,6 +5,7 @@ import ( "net" "os" "path" + "strings" "sync" "time" @@ -301,9 +302,6 @@ func (c Connection) handleSFTPSetstat(filePath string, request *sftp.Request) er } func (c Connection) handleSFTPRename(sourcePath, targetPath string, request *sftp.Request) error { - if !c.isRenamePermitted(sourcePath, request) { - return sftp.ErrSSHFxPermissionDenied - } if c.User.IsMappedPath(sourcePath) { c.Log(logger.LevelWarn, logSender, "renaming a directory mapped as virtual folder is not allowed: %#v", sourcePath) return sftp.ErrSSHFxPermissionDenied @@ -312,24 +310,22 @@ func (c Connection) handleSFTPRename(sourcePath, targetPath string, request *sft c.Log(logger.LevelWarn, logSender, "renaming to a directory mapped as virtual folder is not allowed: %#v", targetPath) return sftp.ErrSSHFxPermissionDenied } - if c.User.HasVirtualFoldersInside(request.Filepath) { - if fi, err := c.fs.Stat(sourcePath); err == nil { - if fi.IsDir() { - c.Log(logger.LevelDebug, logSender, "renaming the folder %#v is not supported: it has virtual folders inside it", - request.Filepath) - return sftp.ErrSSHFxOpUnsupported - } - } + srcInfo, err := c.fs.Lstat(sourcePath) + if err != nil { + return vfs.GetSFTPError(c.fs, err) + } + if !c.isRenamePermitted(sourcePath, request.Filepath, request.Target, srcInfo) { + return sftp.ErrSSHFxPermissionDenied } initialSize := int64(-1) - if fi, err := c.fs.Lstat(targetPath); err == nil { - if fi.IsDir() { + if dstInfo, err := c.fs.Lstat(targetPath); err == nil { + if dstInfo.IsDir() { c.Log(logger.LevelWarn, logSender, "attempted to rename %#v overwriting an existing directory %#v", sourcePath, targetPath) return sftp.ErrSSHFxOpUnsupported } // we are overwriting an existing file/symlink - if fi.Mode().IsRegular() { - initialSize = fi.Size() + if dstInfo.Mode().IsRegular() { + initialSize = dstInfo.Size() } if !c.User.HasPerm(dataprovider.PermOverwrite, path.Dir(request.Target)) { c.Log(logger.LevelDebug, logSender, "renaming is not allowed, source: %#v target: %#v. "+ @@ -337,6 +333,17 @@ func (c Connection) handleSFTPRename(sourcePath, targetPath string, request *sft return sftp.ErrSSHFxPermissionDenied } } + if srcInfo.IsDir() { + if c.User.HasVirtualFoldersInside(request.Filepath) { + c.Log(logger.LevelDebug, logSender, "renaming the folder %#v is not supported: it has virtual folders inside it", + request.Filepath) + return sftp.ErrSSHFxOpUnsupported + } + if err = c.checkRecursiveRenameDirPermissions(sourcePath, targetPath); err != nil { + c.Log(logger.LevelDebug, logSender, "error checking recursive permissions before renaming %#v: %+v", sourcePath, err) + return vfs.GetSFTPError(c.fs, err) + } + } if !c.hasSpaceForRename(request, initialSize, sourcePath) { c.Log(logger.LevelInfo, logSender, "denying cross rename due to space limit") return sftp.ErrSSHFxFailure @@ -798,34 +805,77 @@ func (c Connection) isCrossFoldersRequest(request *sftp.Request) bool { return true } -func (c Connection) isRenamePermitted(sourcePath string, request *sftp.Request) bool { - if c.fs.GetRelativePath(sourcePath) == "/" { +func (c Connection) isRenamePermitted(fsSrcPath, sftpSrcPath, sftpDstPath string, fi os.FileInfo) bool { + if c.fs.GetRelativePath(fsSrcPath) == "/" { c.Log(logger.LevelWarn, logSender, "renaming root dir is not allowed") return false } - if c.User.IsVirtualFolder(request.Filepath) || c.User.IsVirtualFolder(request.Target) { + if c.User.IsVirtualFolder(sftpSrcPath) || c.User.IsVirtualFolder(sftpDstPath) { c.Log(logger.LevelWarn, logSender, "renaming a virtual folder is not allowed") return false } - if !c.User.IsFileAllowed(request.Filepath) || !c.User.IsFileAllowed(request.Target) { - if fi, err := c.fs.Lstat(sourcePath); err == nil && fi.Mode().IsRegular() { - c.Log(logger.LevelDebug, logSender, "renaming file is not allowed, source: %#v target: %#v", request.Filepath, - request.Target) + if !c.User.IsFileAllowed(sftpSrcPath) || !c.User.IsFileAllowed(sftpDstPath) { + if fi != nil && fi.Mode().IsRegular() { + c.Log(logger.LevelDebug, logSender, "renaming file is not allowed, source: %#v target: %#v", sftpSrcPath, + sftpDstPath) return false } } - if !c.User.HasPerm(dataprovider.PermRename, path.Dir(request.Target)) && - (!c.User.HasPerm(dataprovider.PermDelete, path.Dir(request.Filepath)) || - !c.User.HasPerm(dataprovider.PermUpload, path.Dir(request.Target))) { + if c.User.HasPerm(dataprovider.PermRename, path.Dir(sftpSrcPath)) && + c.User.HasPerm(dataprovider.PermRename, path.Dir(sftpDstPath)) { + return true + } + if !c.User.HasPerm(dataprovider.PermDelete, path.Dir(sftpSrcPath)) { return false } - if !c.User.HasPerm(dataprovider.PermRename, path.Dir(request.Target)) && - !c.User.HasPerm(dataprovider.PermCreateDirs, path.Dir(request.Target)) { - if fi, err := c.fs.Lstat(sourcePath); err == nil && fi.IsDir() { - return false + if fi != nil { + if fi.IsDir() { + return c.User.HasPerm(dataprovider.PermCreateDirs, path.Dir(sftpDstPath)) + } else if fi.Mode()&os.ModeSymlink == os.ModeSymlink { + return c.User.HasPerm(dataprovider.PermCreateSymlinks, path.Dir(sftpDstPath)) } } - return true + return c.User.HasPerm(dataprovider.PermUpload, path.Dir(sftpDstPath)) +} + +func (c Connection) checkRecursiveRenameDirPermissions(sourcePath, targetPath string) error { + dstPerms := []string{ + dataprovider.PermCreateDirs, + dataprovider.PermUpload, + dataprovider.PermCreateSymlinks, + } + + err := c.fs.Walk(sourcePath, func(walkedPath string, info os.FileInfo, err error) error { + if err != nil { + return err + } + dstPath := strings.Replace(walkedPath, sourcePath, targetPath, 1) + sftpSrcPath := c.fs.GetRelativePath(walkedPath) + sftpDstPath := c.fs.GetRelativePath(dstPath) + // walk scans the directory tree in order, checking the parent dirctory permissions we are sure that all contents + // inside the parent path was checked. If the current dir has no subdirs with defined permissions inside it + // and it has all the possible permissions we can stop scanning + if !c.User.HasPermissionsInside(path.Dir(sftpSrcPath)) && !c.User.HasPermissionsInside(path.Dir(sftpDstPath)) { + if c.User.HasPerm(dataprovider.PermRename, path.Dir(sftpSrcPath)) && + c.User.HasPerm(dataprovider.PermRename, path.Dir(sftpDstPath)) { + return errSkipPermissionsCheck + } + if c.User.HasPerm(dataprovider.PermDelete, path.Dir(sftpSrcPath)) && + c.User.HasPerms(dstPerms, path.Dir(sftpDstPath)) { + return errSkipPermissionsCheck + } + } + if !c.isRenamePermitted(walkedPath, sftpSrcPath, sftpDstPath, info) { + c.Log(logger.LevelInfo, logSender, "rename %#v -> %#v is not allowed, sftp destination path: %#v", + walkedPath, dstPath, sftpDstPath) + return os.ErrPermission + } + return nil + }) + if err == errSkipPermissionsCheck { + err = nil + } + return err } func (c Connection) updateQuotaMoveBetweenVFolders(sourceFolder, dstFolder vfs.VirtualFolder, initialSize, filesSize int64, numFiles int) { diff --git a/sftpd/internal_test.go b/sftpd/internal_test.go index e9309b10..ad54d756 100644 --- a/sftpd/internal_test.go +++ b/sftpd/internal_test.go @@ -470,6 +470,11 @@ func TestMockFsErrors(t *testing.T) { err = c.handleSFTPRemove(testfile, request) assert.EqualError(t, err, sftp.ErrSSHFxFailure.Error()) + request = sftp.NewRequest("Rename", filepath.Base(testfile)) + request.Target = filepath.Base(testfile) + "1" + err = c.handleSFTPRename(testfile, testfile+"1", request) + assert.EqualError(t, err, sftp.ErrSSHFxFailure.Error()) + err = os.Remove(testfile) assert.NoError(t, err) } @@ -782,10 +787,10 @@ func TestSSHCommandErrors(t *testing.T) { } cmd.connection.User.Permissions = make(map[string][]string) cmd.connection.User.Permissions["/"] = []string{dataprovider.PermDownload} - _, _, err = cmd.getCopyPaths() - if assert.Error(t, err) { - assert.EqualError(t, err, errPermissionDenied.Error()) - } + src, dst, err := cmd.getCopyPaths() + assert.NoError(t, err) + assert.False(t, cmd.hasCopyPermissions(src, dst, nil)) + cmd.connection.User.Permissions = make(map[string][]string) cmd.connection.User.Permissions["/"] = []string{dataprovider.PermAny} if runtime.GOOS != osWindows { @@ -2043,6 +2048,9 @@ func TestRenamePermission(t *testing.T) { permissions["/dir2"] = []string{dataprovider.PermUpload} permissions["/dir3"] = []string{dataprovider.PermDelete} permissions["/dir4"] = []string{dataprovider.PermListItems} + permissions["/dir5"] = []string{dataprovider.PermCreateDirs, dataprovider.PermUpload} + permissions["/dir6"] = []string{dataprovider.PermCreateDirs, dataprovider.PermUpload, + dataprovider.PermListItems, dataprovider.PermCreateSymlinks} user := dataprovider.User{ Permissions: permissions, @@ -2055,34 +2063,103 @@ func TestRenamePermission(t *testing.T) { fs: fs, } request := sftp.NewRequest("Rename", "/testfile") + request.Target = "/dir1/testfile" + // rename is granted on Source and Target + assert.True(t, conn.isRenamePermitted("", request.Filepath, request.Target, nil)) request.Target = "/dir4/testfile" // rename is not granted on Target - assert.False(t, conn.isRenamePermitted("", request)) + assert.False(t, conn.isRenamePermitted("", request.Filepath, request.Target, nil)) + request = sftp.NewRequest("Rename", "/dir1/testfile") + request.Target = "/dir2/testfile" //nolint:goconst + // rename is granted on Source but not on Target + assert.False(t, conn.isRenamePermitted("", request.Filepath, request.Target, nil)) request = sftp.NewRequest("Rename", "/dir4/testfile") request.Target = "/dir1/testfile" - // rename is granted on Target, this is enough - assert.True(t, conn.isRenamePermitted("", request)) + // rename is granted on Target but not on Source + assert.False(t, conn.isRenamePermitted("", request.Filepath, request.Target, nil)) request = sftp.NewRequest("Rename", "/dir4/testfile") request.Target = "/testfile" - // rename is granted on Target, this is enough - assert.True(t, conn.isRenamePermitted("", request)) + // rename is granted on Target but not on Source + assert.False(t, conn.isRenamePermitted("", request.Filepath, request.Target, nil)) request = sftp.NewRequest("Rename", "/dir3/testfile") request.Target = "/dir2/testfile" // delete is granted on Source and Upload on Target, the target is a file this is enough - assert.True(t, conn.isRenamePermitted("", request)) + assert.True(t, conn.isRenamePermitted("", request.Filepath, request.Target, nil)) request = sftp.NewRequest("Rename", "/dir2/testfile") request.Target = "/dir3/testfile" - assert.False(t, conn.isRenamePermitted("", request)) + assert.False(t, conn.isRenamePermitted("", request.Filepath, request.Target, nil)) tmpDir := filepath.Join(os.TempDir(), "dir") + tmpDirLink := filepath.Join(os.TempDir(), "link") err = os.Mkdir(tmpDir, os.ModePerm) assert.NoError(t, err) + err = os.Symlink(tmpDir, tmpDirLink) + assert.NoError(t, err) request.Filepath = "/dir" request.Target = "/dir2/dir" // the source is a dir and the target has no createDirs perm - assert.False(t, conn.isRenamePermitted(tmpDir, request)) - conn.User.Permissions["/dir2"] = []string{dataprovider.PermUpload, dataprovider.PermCreateDirs} - // the source is a dir and the target has createDirs perm - assert.True(t, conn.isRenamePermitted(tmpDir, request)) + info, err := os.Lstat(tmpDir) + if assert.NoError(t, err) { + assert.False(t, conn.isRenamePermitted(tmpDir, request.Filepath, request.Target, info)) + conn.User.Permissions["/dir2"] = []string{dataprovider.PermUpload, dataprovider.PermCreateDirs} + // the source is a dir and the target has createDirs perm + assert.True(t, conn.isRenamePermitted(tmpDir, request.Filepath, request.Target, info)) + + request = sftp.NewRequest("Rename", "/testfile") + request.Target = "/dir5/testfile" + // the source is a dir and the target has createDirs and upload perm + assert.True(t, conn.isRenamePermitted(tmpDir, request.Filepath, request.Target, info)) + } + info, err = os.Lstat(tmpDirLink) + if assert.NoError(t, err) { + assert.True(t, info.Mode()&os.ModeSymlink == os.ModeSymlink) + // the source is a symlink and the target has createDirs and upload perm + assert.False(t, conn.isRenamePermitted(tmpDir, request.Filepath, request.Target, info)) + } err = os.RemoveAll(tmpDir) assert.NoError(t, err) + err = os.Remove(tmpDirLink) + assert.NoError(t, err) + conn.User.VirtualFolders = append(conn.User.VirtualFolders, vfs.VirtualFolder{ + BaseVirtualFolder: vfs.BaseVirtualFolder{ + MappedPath: os.TempDir(), + }, + VirtualPath: "/dir1", + }) + request = sftp.NewRequest("Rename", "/dir1") + request.Target = "/dir2/testfile" + // renaming a virtual folder is not allowed + assert.False(t, conn.isRenamePermitted("", request.Filepath, request.Target, nil)) + err = conn.checkRecursiveRenameDirPermissions("invalid", "invalid") + assert.Error(t, err) + dir3 := filepath.Join(conn.User.HomeDir, "dir3") + dir6 := filepath.Join(conn.User.HomeDir, "dir6") + err = os.MkdirAll(filepath.Join(dir3, "subdir"), os.ModePerm) + assert.NoError(t, err) + err = ioutil.WriteFile(filepath.Join(dir3, "subdir", "testfile"), []byte("test"), os.ModePerm) + assert.NoError(t, err) + err = conn.checkRecursiveRenameDirPermissions(dir3, dir6) + assert.NoError(t, err) +} + +func TestRecursiveCopyErrors(t *testing.T) { + permissions := make(map[string][]string) + permissions["/"] = []string{dataprovider.PermAny} + user := dataprovider.User{ + Permissions: permissions, + HomeDir: os.TempDir(), + } + fs, err := user.GetFilesystem("123") + assert.NoError(t, err) + conn := Connection{ + User: user, + fs: fs, + } + sshCmd := sshCommand{ + command: "sftpgo-copy", + connection: conn, + args: []string{"adir", "another"}, + } + // try to copy a missing directory + err = sshCmd.checkRecursiveCopyPermissions("adir", "another", "/another") + assert.Error(t, err) } diff --git a/sftpd/sftpd_test.go b/sftpd/sftpd_test.go index deedeaf7..0ff4e49e 100644 --- a/sftpd/sftpd_test.go +++ b/sftpd/sftpd_test.go @@ -142,6 +142,7 @@ func TestMain(m *testing.M) { os.Exit(1) } providerConf := config.GetProviderConf() + logger.InfoToConsole("Starting SFTPD tests, provider: %v", providerConf.Driver) err = dataprovider.Initialize(providerConf, configDir) if err != nil { @@ -2158,12 +2159,19 @@ func TestVirtualFolders(t *testing.T) { u := getTestUser(usePubKey) mappedPath := filepath.Join(os.TempDir(), "vdir") vdirPath := "/vdir/subdir" + testDir := "/userDir" + testDir1 := "/userDir1" u.VirtualFolders = append(u.VirtualFolders, vfs.VirtualFolder{ BaseVirtualFolder: vfs.BaseVirtualFolder{ MappedPath: mappedPath, }, VirtualPath: vdirPath, }) + u.Permissions[testDir] = []string{dataprovider.PermCreateDirs} + u.Permissions[testDir1] = []string{dataprovider.PermCreateDirs, dataprovider.PermUpload, dataprovider.PermDelete} + u.Permissions[path.Join(testDir1, "subdir")] = []string{dataprovider.PermCreateSymlinks, dataprovider.PermUpload, + dataprovider.PermDelete} + err := os.MkdirAll(mappedPath, os.ModePerm) assert.NoError(t, err) user, _, err := httpd.AddUser(u, http.StatusOK) @@ -2195,8 +2203,48 @@ func TestVirtualFolders(t *testing.T) { assert.Error(t, err, "removing a directory with a virtual folder inside must fail") err = client.Mkdir("vdir1") assert.NoError(t, err) + // rename empty dir /vdir1, we have permission on / err = client.Rename("vdir1", "vdir2") assert.NoError(t, err) + err = sftpUploadFile(testFilePath, path.Join("vdir2", testFileName), testFileSize, client) + assert.NoError(t, err) + // we don't have upload permission on testDir, we can only create dirs + err = client.Rename("vdir2", testDir) + assert.Error(t, err) + // on testDir1 only symlink aren't allowed + err = client.Rename("vdir2", testDir1) + assert.NoError(t, err) + err = client.Rename(testDir1, "vdir2") + assert.NoError(t, err) + err = client.MkdirAll(path.Join("vdir2", "subdir")) + assert.NoError(t, err) + err = sftpUploadFile(testFilePath, path.Join("vdir2", "subdir", testFileName), testFileSize, client) + assert.NoError(t, err) + err = client.Rename("vdir2", testDir1) + assert.NoError(t, err) + err = client.Rename(testDir1, "vdir2") + assert.NoError(t, err) + err = client.MkdirAll(path.Join("vdir2", "subdir", "subdir")) + assert.NoError(t, err) + err = sftpUploadFile(testFilePath, path.Join("vdir2", "subdir", "subdir", testFileName), testFileSize, client) + assert.NoError(t, err) + // we cannot create dirs inside /userDir1/subdir + err = client.Rename("vdir2", testDir1) + assert.Error(t, err) + err = client.Rename("vdir2", "vdir3") + assert.NoError(t, err) + err = client.Remove(path.Join("vdir3", "subdir", "subdir", testFileName)) + assert.NoError(t, err) + err = client.RemoveDirectory(path.Join("vdir3", "subdir", "subdir")) + assert.NoError(t, err) + err = client.Rename("vdir3", testDir1) + assert.NoError(t, err) + err = client.Rename(testDir1, "vdir2") + assert.NoError(t, err) + err = client.Symlink(path.Join("vdir2", "subdir", testFileName), path.Join("vdir2", "subdir", "alink")) + assert.NoError(t, err) + err = client.Rename("vdir2", testDir1) + assert.NoError(t, err) err = os.Remove(testFilePath) assert.NoError(t, err) err = os.Remove(localDownloadPath) @@ -5499,11 +5547,16 @@ func TestSSHCopy(t *testing.T) { _, err = runSSHCommand(fmt.Sprintf("sftpgo-copy %v %v", path.Join(testDir, testFileName), testFileName+".denied"), user, usePubKey) assert.Error(t, err) if runtime.GOOS != osWindows { - err = os.Chmod(filepath.Join(mappedPath1, testDir1), 0001) + subPath := filepath.Join(mappedPath1, testDir1, "asubdir", "anothersub", "another") + err = os.MkdirAll(subPath, os.ModePerm) assert.NoError(t, err) - _, err = runSSHCommand(fmt.Sprintf("sftpgo-copy %v %v", path.Join(vdirPath1), "newdir"), user, usePubKey) + err = os.Chmod(subPath, 0001) + assert.NoError(t, err) + // c.connection.fs.GetDirSize(fsSourcePath) will fail scanning subdirs + // checkRecursiveCopyPermissions will work since it will skip subdirs with no permissions + _, err = runSSHCommand(fmt.Sprintf("sftpgo-copy %v %v", vdirPath1, "newdir"), user, usePubKey) assert.Error(t, err) - err = os.Chmod(filepath.Join(mappedPath1, testDir1), os.ModePerm) + err = os.Chmod(subPath, os.ModePerm) assert.NoError(t, err) err = os.Chmod(filepath.Join(user.GetHomeDir(), testDir1), 0555) assert.NoError(t, err) @@ -5528,18 +5581,6 @@ func TestSSHCopy(t *testing.T) { err = os.Remove(testFilePath1) assert.NoError(t, err) } - // test copy dir with no create dirs perm - user.Permissions["/"] = []string{dataprovider.PermUpload, dataprovider.PermDownload, dataprovider.PermListItems} - _, _, err = httpd.UpdateUser(user, http.StatusOK) - assert.NoError(t, err) - client, err = getSftpClient(user, usePubKey) - if assert.NoError(t, err) { - defer client.Close() - _, err = runSSHCommand(fmt.Sprintf("sftpgo-copy %v %v", path.Join(vdirPath1, testDir1), testDir1+"copy1"), - user, usePubKey) - assert.Error(t, err) - } - _, err = httpd.RemoveUser(user, http.StatusOK) assert.NoError(t, err) _, err = httpd.RemoveFolder(vfs.BaseVirtualFolder{MappedPath: mappedPath1}, http.StatusOK) @@ -5554,8 +5595,71 @@ func TestSSHCopy(t *testing.T) { assert.NoError(t, err) } -func TestSSHCopyQuotaLimits(t *testing.T) { +func TestSSHCopyPermissions(t *testing.T) { usePubKey := false + u := getTestUser(usePubKey) + u.Permissions["/dir1"] = []string{dataprovider.PermUpload, dataprovider.PermDownload, dataprovider.PermListItems} + u.Permissions["/dir2"] = []string{dataprovider.PermCreateDirs, dataprovider.PermUpload, dataprovider.PermDownload, + dataprovider.PermListItems} + u.Permissions["/dir3"] = []string{dataprovider.PermCreateDirs, dataprovider.PermCreateSymlinks, dataprovider.PermDownload, + dataprovider.PermListItems} + user, _, err := httpd.AddUser(u, http.StatusOK) + assert.NoError(t, err) + client, err := getSftpClient(user, usePubKey) + if assert.NoError(t, err) { + defer client.Close() + testDir := "tDir" + testFileSize := int64(131072) + testFileName := "test_file.dat" + testFilePath := filepath.Join(homeBasePath, testFileName) + err = createTestFile(testFilePath, testFileSize) + assert.NoError(t, err) + err = client.Mkdir(testDir) + assert.NoError(t, err) + err = sftpUploadFile(testFilePath, path.Join("/", testDir, testFileName), testFileSize, client) + assert.NoError(t, err) + // test copy file with no permission + _, err = runSSHCommand(fmt.Sprintf("sftpgo-copy %v %v", path.Join("/", testDir, testFileName), path.Join("/dir3", testFileName)), + user, usePubKey) + assert.Error(t, err) + // test copy dir with no create dirs perm + _, err = runSSHCommand(fmt.Sprintf("sftpgo-copy %v %v", path.Join("/", testDir), "/dir1/"), user, usePubKey) + assert.Error(t, err) + // dir2 has the needed permissions + _, err = runSSHCommand(fmt.Sprintf("sftpgo-copy %v %v", path.Join("/", testDir), "/dir2/"), user, usePubKey) + assert.NoError(t, err) + info, err := client.Stat(path.Join("/dir2", testDir)) + if assert.NoError(t, err) { + assert.True(t, info.IsDir()) + } + info, err = client.Stat(path.Join("/dir2", testDir, testFileName)) + if assert.NoError(t, err) { + assert.True(t, info.Mode().IsRegular()) + } + // now create a symlink, dir2 has no create symlink permission + err = client.Symlink(path.Join("/", testDir, testFileName), path.Join("/", testDir, testFileName+".link")) + assert.NoError(t, err) + _, err = runSSHCommand(fmt.Sprintf("sftpgo-copy %v %v", path.Join("/", testDir), "/dir2/sub"), user, usePubKey) + assert.Error(t, err) + _, err = runSSHCommand(fmt.Sprintf("sftpgo-copy %v %v", path.Join("/", testDir), "/newdir"), user, usePubKey) + assert.NoError(t, err) + // now delete the file and copy inside /dir3 + err = client.Remove(path.Join("/", testDir, testFileName)) + assert.NoError(t, err) + _, err = runSSHCommand(fmt.Sprintf("sftpgo-copy %v %v", path.Join("/", testDir), "/dir3"), user, usePubKey) + assert.NoError(t, err) + + err = os.Remove(testFilePath) + assert.NoError(t, err) + } + _, err = httpd.RemoveUser(user, http.StatusOK) + assert.NoError(t, err) + err = os.RemoveAll(user.GetHomeDir()) + assert.NoError(t, err) +} + +func TestSSHCopyQuotaLimits(t *testing.T) { + usePubKey := true testFileSize := int64(131072) testFileSize1 := int64(65536) testFileSize2 := int64(32768) diff --git a/sftpd/ssh_cmd.go b/sftpd/ssh_cmd.go index c845800c..b1987e3d 100644 --- a/sftpd/ssh_cmd.go +++ b/sftpd/ssh_cmd.go @@ -30,9 +30,10 @@ import ( const scpCmdName = "scp" var ( - errQuotaExceeded = errors.New("denying write due to space limit") - errPermissionDenied = errors.New("Permission denied. You don't have the permissions to execute this command") - errUnsupportedConfig = errors.New("command unsupported for this configuration") + errQuotaExceeded = errors.New("denying write due to space limit") + errPermissionDenied = errors.New("Permission denied. You don't have the permissions to execute this command") + errUnsupportedConfig = errors.New("command unsupported for this configuration") + errSkipPermissionsCheck = errors.New("permission check skipped") ) type sshCommand struct { @@ -134,12 +135,12 @@ func (c *sshCommand) handeSFTPGoCopy() error { if err != nil { return c.sendErrorResponse(err) } + if err := c.checkCopyPermissions(fsSourcePath, fsDestPath, sshSourcePath, sshDestPath, fi); err != nil { + return c.sendErrorResponse(err) + } filesNum := 0 filesSize := int64(0) if fi.IsDir() { - if !c.connection.User.HasPerm(dataprovider.PermCreateDirs, path.Dir(sshDestPath)) { - return c.sendErrorResponse(errPermissionDenied) - } filesNum, filesSize, err = c.connection.fs.GetDirSize(fsSourcePath) if err != nil { return c.sendErrorResponse(err) @@ -593,13 +594,69 @@ func (c *sshCommand) getCopyPaths() (string, string, error) { err := errors.New("usage sftpgo-copy ") return "", "", err } - if !c.connection.User.HasPerm(dataprovider.PermListItems, path.Dir(sshSourcePath)) || - !c.connection.User.HasPerm(dataprovider.PermUpload, path.Dir(sshDestPath)) { - return "", "", errPermissionDenied - } return sshSourcePath, sshDestPath, nil } +func (c *sshCommand) hasCopyPermissions(sshSourcePath, sshDestPath string, srcInfo os.FileInfo) bool { + if !c.connection.User.HasPerm(dataprovider.PermListItems, path.Dir(sshSourcePath)) { + return false + } + if srcInfo.IsDir() { + return c.connection.User.HasPerm(dataprovider.PermCreateDirs, path.Dir(sshDestPath)) + } else if srcInfo.Mode()&os.ModeSymlink == os.ModeSymlink { + return c.connection.User.HasPerm(dataprovider.PermCreateSymlinks, path.Dir(sshDestPath)) + } + return c.connection.User.HasPerm(dataprovider.PermUpload, path.Dir(sshDestPath)) +} + +// fsSourcePath must be a directory +func (c *sshCommand) checkRecursiveCopyPermissions(fsSourcePath, fsDestPath, sshDestPath string) error { + if !c.connection.User.HasPerm(dataprovider.PermCreateDirs, path.Dir(sshDestPath)) { + return errPermissionDenied + } + dstPerms := []string{ + dataprovider.PermCreateDirs, + dataprovider.PermCreateSymlinks, + dataprovider.PermUpload, + } + + err := c.connection.fs.Walk(fsSourcePath, func(walkedPath string, info os.FileInfo, err error) error { + if err != nil { + return err + } + fsDstSubPath := strings.Replace(walkedPath, fsSourcePath, fsDestPath, 1) + sshSrcSubPath := c.connection.fs.GetRelativePath(walkedPath) + sshDstSubPath := c.connection.fs.GetRelativePath(fsDstSubPath) + // If the current dir has no subdirs with defined permissions inside it + // and it has all the possible permissions we can stop scanning + if !c.connection.User.HasPermissionsInside(path.Dir(sshSrcSubPath)) && + !c.connection.User.HasPermissionsInside(path.Dir(sshDstSubPath)) { + if c.connection.User.HasPerm(dataprovider.PermListItems, path.Dir(sshSrcSubPath)) && + c.connection.User.HasPerms(dstPerms, path.Dir(sshDstSubPath)) { + return errSkipPermissionsCheck + } + } + if !c.hasCopyPermissions(sshSrcSubPath, sshDstSubPath, info) { + return errPermissionDenied + } + return nil + }) + if err == errSkipPermissionsCheck { + err = nil + } + return err +} + +func (c *sshCommand) checkCopyPermissions(fsSourcePath, fsDestPath, sshSourcePath, sshDestPath string, info os.FileInfo) error { + if info.IsDir() { + return c.checkRecursiveCopyPermissions(fsSourcePath, fsDestPath, sshDestPath) + } + if !c.hasCopyPermissions(sshSourcePath, sshDestPath, info) { + return errPermissionDenied + } + return nil +} + func (c *sshCommand) getRemovePath() (string, error) { sshDestPath := c.getDestPath() if len(sshDestPath) == 0 || len(c.args) != 1 { diff --git a/utils/utils.go b/utils/utils.go index b899d665..c94870fa 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -266,7 +266,7 @@ func GetDirsForSFTPPath(p string) []string { return dirsForPath } -// CleanSFTPPath returns a clean sftp path +// CleanSFTPPath returns a cleaned SFTP path func CleanSFTPPath(p string) string { sftpPath := filepath.ToSlash(p) if !path.IsAbs(p) { diff --git a/vfs/gcsfs.go b/vfs/gcsfs.go index 4caa9673..9f0e3fbe 100644 --- a/vfs/gcsfs.go +++ b/vfs/gcsfs.go @@ -10,6 +10,7 @@ import ( "net/http" "os" "path" + "path/filepath" "strings" "time" @@ -420,7 +421,7 @@ func (fs GCSFs) ScanRootDirContents() (int, int64, error) { // GetDirSize returns the number of files and the size for a folder // including any subfolders func (GCSFs) GetDirSize(dirname string) (int, int64, error) { - return 0, 0, errors.New("Not implemented") + return 0, 0, errUnsupported } // GetAtomicUploadPath returns the path to use for an atomic upload. @@ -448,6 +449,12 @@ func (fs GCSFs) GetRelativePath(name string) string { return rel } +// Walk walks the file tree rooted at root, calling walkFn for each file or +// directory in the tree, including root +func (GCSFs) Walk(root string, walkFn filepath.WalkFunc) error { + return errUnsupported +} + // Join joins any number of path elements into a single path func (GCSFs) Join(elem ...string) string { return strings.TrimPrefix(path.Join(elem...), "/") diff --git a/vfs/osfs.go b/vfs/osfs.go index 61f7da2d..20b1adac 100644 --- a/vfs/osfs.go +++ b/vfs/osfs.go @@ -221,6 +221,12 @@ func (fs OsFs) GetRelativePath(name string) string { return path.Join(virtualPath, filepath.ToSlash(rel)) } +// Walk walks the file tree rooted at root, calling walkFn for each file or +// directory in the tree, including root +func (OsFs) Walk(root string, walkFn filepath.WalkFunc) error { + return filepath.Walk(root, walkFn) +} + // Join joins any number of path elements into a single path func (OsFs) Join(elem ...string) string { return filepath.Join(elem...) diff --git a/vfs/s3fs.go b/vfs/s3fs.go index 76de994e..17a648a6 100644 --- a/vfs/s3fs.go +++ b/vfs/s3fs.go @@ -8,6 +8,7 @@ import ( "fmt" "os" "path" + "path/filepath" "strings" "time" @@ -441,7 +442,7 @@ func (fs S3Fs) ScanRootDirContents() (int, int64, error) { // GetDirSize returns the number of files and the size for a folder // including any subfolders func (S3Fs) GetDirSize(dirname string) (int, int64, error) { - return 0, 0, errors.New("Not implemented") + return 0, 0, errUnsupported } // GetAtomicUploadPath returns the path to use for an atomic upload. @@ -469,6 +470,12 @@ func (fs S3Fs) GetRelativePath(name string) string { return rel } +// Walk walks the file tree rooted at root, calling walkFn for each file or +// directory in the tree, including root +func (S3Fs) Walk(root string, walkFn filepath.WalkFunc) error { + return errUnsupported +} + // Join joins any number of path elements into a single path func (S3Fs) Join(elem ...string) string { return path.Join(elem...) diff --git a/vfs/vfs.go b/vfs/vfs.go index 0fc403ce..9b011101 100644 --- a/vfs/vfs.go +++ b/vfs/vfs.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path" + "path/filepath" "runtime" "strings" "time" @@ -42,9 +43,12 @@ type Fs interface { GetDirSize(dirname string) (int, int64, error) GetAtomicUploadPath(name string) string GetRelativePath(name string) string + Walk(root string, walkFn filepath.WalkFunc) error Join(elem ...string) string } +var errUnsupported = errors.New("Not supported") + // QuotaCheckResult defines the result for a quota check type QuotaCheckResult struct { HasSpace bool