diff --git a/internal/sftpd/internal_test.go b/internal/sftpd/internal_test.go index edb8a10f..01cbdd5e 100644 --- a/internal/sftpd/internal_test.go +++ b/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"), - "--munge-links must be added if the user has the create symlinks permission") + assert.Equal(t, []string{"rsync", "--server", "-vlogDtprze.iLsfxC", "--munge-links", ".", user.HomeDir + "/"}, cmd.cmd.Args, + "--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)) +} diff --git a/internal/sftpd/ssh_cmd.go b/internal/sftpd/ssh_cmd.go index 604a0bb0..d9109ea6 100644 --- a/internal/sftpd/ssh_cmd.go +++ b/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 {