rsync: enforce a supported format and limit the allowed options

Many rsync options are unsafe to use in restricted environments
and may pose security risks.

Signed-off-by: Nicola Murino <nicola.murino@gmail.com>

internal/sftpd/internal_test.go

@@ -844,7 +844,7 @@ func TestRsyncOptions(t *testing.T) {
 	}
 	cmd, err := sshCmd.getSystemCommand()
 	assert.NoError(t, err)
-	assert.True(t, slices.Contains(cmd.cmd.Args, "--safe-links"),
+	assert.Equal(t, []string{"rsync", "--server", "-vlogDtprze.iLsfxC", "--safe-links", ".", user.HomeDir + "/"}, cmd.cmd.Args,
 		"--safe-links must be added if the user has the create symlinks permission")
 
 	permissions["/"] = []string{dataprovider.PermDownload, dataprovider.PermUpload, dataprovider.PermCreateDirs,
@@ -854,6 +854,12 @@ func TestRsyncOptions(t *testing.T) {
 	conn = &Connection{
 		BaseConnection: common.NewBaseConnection("", common.ProtocolSFTP, "", "", user),
 	}
+	sshCmd = sshCommand{
+		command:    "rsync",
+		connection: conn,
+	}
+	_, err = sshCmd.getSystemCommand()
+	assert.Error(t, err)
 	sshCmd = sshCommand{
 		command:    "rsync",
 		connection: conn,
@@ -861,8 +867,8 @@ func TestRsyncOptions(t *testing.T) {
 	}
 	cmd, err = sshCmd.getSystemCommand()
 	assert.NoError(t, err)
-	assert.True(t, slices.Contains(cmd.cmd.Args, "--munge-links"),
+	assert.Equal(t, []string{"rsync", "--server", "-vlogDtprze.iLsfxC", "--munge-links", ".", user.HomeDir + "/"}, cmd.cmd.Args,
-		"--munge-links must be added if the user has the create symlinks permission")
+		"--munge-links must be added if the user hasn't the create symlinks permission")
 
 	sshCmd.connection.User.VirtualFolders = append(sshCmd.connection.User.VirtualFolders, vfs.VirtualFolder{
 		BaseVirtualFolder: vfs.BaseVirtualFolder{
@@ -2241,3 +2247,43 @@ func TestAuthenticationErrors(t *testing.T) {
 	assert.ErrorIs(t, err, sftpAuthError)
 	assert.NotErrorIs(t, err, util.ErrNotFound)
 }
+
+func TestRsyncArguments(t *testing.T) {
+	assert.False(t, canAcceptRsyncArgs(nil))
+	args := []string{"-e", "--server"}
+	assert.False(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", "--sender", "-vlogDtpre.iLsfxCIvu", ".", "."}
+	assert.True(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", "--sender", "--server", "-vlogDtpre.iLsfxCIvu", ".", "."}
+	assert.False(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", "..", "/"}
+	assert.False(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", ".", "/"}
+	assert.False(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", "--sender", "-vlogDtpre.iLsfxCIvu", ".", "."}
+	assert.True(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", "--sender", "-vlogDtpre.iLsfxCIvu", "--delete", ".", "/"}
+	assert.True(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", "-vlogDtpre.iLsfxCIvu", "--delete", ".", "/"}
+	assert.True(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", "-vlogDtpre.iLsfxCIvu", "--delete", "/", ".", "/"}
+	assert.False(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", "--sender", "-vlogDtpre.iLsfxCIvu", ".", "path1", "path2"}
+	assert.False(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", "--sender", "-vlogDtpre.iLsfxCIvu", "."}
+	assert.False(t, canAcceptRsyncArgs(args))
+	args = []string{"--sender", "-vlogDtpre.iLsfxCIvu", "--delete", ".", "/"}
+	assert.False(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", "-vlogDtpre.", "--delete", ".", "/"}
+	assert.False(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", "--sender", "-vlogDtpre.", "--delete", ".", "/"}
+	assert.False(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", "--sender", "-e.iLsfxCIvu", "--delete", ".", "/"}
+	assert.False(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", "-vlogDtpre.iLsfxCIvu", "--delete", "/"}
+	assert.False(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", "-vlogDtpre.iLsfxCIvu", "--delete", "--safe-links"}
+	assert.False(t, canAcceptRsyncArgs(args))
+	args = []string{"--server", "-vlogDtpre.iLsfxCIvu", "--unsupported-option", ".", "/"}
+	assert.False(t, canAcceptRsyncArgs(args))
+}

internal/sftpd/ssh_cmd.go

@@ -427,6 +427,10 @@ func (c *sshCommand) getSystemCommand() (systemCommand, error) {
 		return command, errUnsupportedConfig
 	}
 	if c.command == "rsync" {
+		if !canAcceptRsyncArgs(args) {
+			c.connection.Log(logger.LevelWarn, "invalid rsync command, args: %+v", args)
+			return command, errors.New("invalid or unsupported rsync command")
+		}
 		// we cannot avoid that rsync creates symlinks so if the user has the permission
 		// to create symlinks we add the option --safe-links to the received rsync command if
 		// it is not already set. This should prevent to create symlinks that point outside
@@ -435,11 +439,11 @@ func (c *sshCommand) getSystemCommand() (systemCommand, error) {
 		// already set. This should make symlinks unusable (but manually recoverable)
 		if c.connection.User.HasPerm(dataprovider.PermCreateSymlinks, c.getDestPath()) {
 			if !slices.Contains(args, "--safe-links") {
-				args = append([]string{"--safe-links"}, args...)
+				args = slices.Insert(args, len(args)-2, "--safe-links")
 			}
 		} else {
 			if !slices.Contains(args, "--munge-links") {
-				args = append([]string{"--munge-links"}, args...)
+				args = slices.Insert(args, len(args)-2, "--munge-links")
 			}
 		}
 	}
@@ -456,6 +460,85 @@ func (c *sshCommand) getSystemCommand() (systemCommand, error) {
 	return command, nil
 }
 
+var (
+	acceptedRsyncOptions = []string{
+		"--existing",
+		"--ignore-existing",
+		"--remove-source-files",
+		"--delete",
+		"--delete-before",
+		"--delete-during",
+		"--delete-delay",
+		"--delete-after",
+		"--delete-excluded",
+		"--ignore-errors",
+		"--force",
+		"--partial",
+		"--delay-updates",
+		"--size-only",
+		"--blocking-io",
+		"--stats",
+		"--progress",
+		"--list-only",
+		"--dry-run",
+	}
+)
+
+func canAcceptRsyncArgs(args []string) bool {
+	// We support the following formats:
+	//
+	// rsync --server -vlogDtpre.iLsfxCIvu --supported-options . ARG  # push
+	// rsync --server --sender -vlogDtpre.iLsfxCIvu --supported-options . ARG  # pull
+	//
+	// Then some options with a single dash and containing "e."" followed by
+	// supported options, listed in acceptedRsyncOptions, with double dash then
+	// dot and a finally single argument specifying the path to operate on.
+	idx := 0
+	if len(args) < 4 {
+		return false
+	}
+	// The first argument must be --server.
+	if args[idx] != "--server" {
+		return false
+	}
+	idx++
+	// The second argument must be --sender or an argument starting with a
+	// single dash and containing "e."
+	if args[idx] == "--sender" {
+		idx++
+	}
+	// Check that this argument starts with a dash and contains e. but does start or end with e.
+	if !strings.HasPrefix(args[idx], "-") ||
+		strings.HasPrefix(args[idx], "--") || strings.HasPrefix(args[idx], "-e") ||
+		!strings.Contains(args[idx], "e.") || strings.HasSuffix(args[idx], "e.") {
+		return false
+	}
+	idx++
+	// We now expect optional supported options like --delete or a dot followed
+	// by the path to operate on. We don't support multiple paths in sender
+	// mode.
+	if len(args) < idx+2 {
+		return false
+	}
+	// A dot is required we'll check the expected position later.
+	if !slices.Contains(args, ".") {
+		return false
+	}
+	for _, arg := range args[idx:] {
+		if slices.Contains(acceptedRsyncOptions, arg) {
+			idx++
+		} else {
+			if arg == "." {
+				idx++
+				break
+			}
+			// Unsupported argument.
+			return false
+		}
+	}
+	return len(args) == idx+1
+}
+
 // for the supported commands, the destination path, if any, is the last argument
 func (c *sshCommand) getDestPath() string {
 	if len(c.args) == 0 {