recursive permissions check before renaming/copying directories

devopsmakers · Jun 26, 2020 · cf541d6 · cf541d6
1 parent 19fc58d
commit cf541d6
Show file tree

Hide file tree

Showing 17 changed files with 414 additions and 81 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 <src> <dst>`. 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 <src> <dst>`. 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 <dst>`. 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
@@ -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
@@ -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
@@ -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,31 +310,40 @@ 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. "+
 				"Target exists but the user has no overwrite permission", request.Filepath, request.Target)
 			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) {