diff --git a/cmd/serve/sftp/connection.go b/cmd/serve/sftp/connection.go index b869eaad6..28650e268 100644 --- a/cmd/serve/sftp/connection.go +++ b/cmd/serve/sftp/connection.go @@ -57,6 +57,7 @@ type conn struct { // execCommand implements an extremely limited number of commands to // interoperate with the rclone sftp backend func (c *conn) execCommand(ctx context.Context, out io.Writer, command string) (err error) { + defer recoverPanic(&err) binary, args := command, "" before, after, ok := strings.Cut(command, " ") if ok { @@ -269,6 +270,11 @@ func (c *conn) handleChannel(newChannel ssh.NewChannel) { // Handle out-of-band requests go func(in <-chan *ssh.Request) { + // Unblock the main routine when the requests run out, otherwise a + // channel which never makes a supported request - because it was + // rejected, or was closed straight away - leaks this goroutine and + // the channel for the lifetime of the connection. + defer close(isSFTP) for req := range in { fs.Debugf(c.what, "Request: %v\n", req.Type) ok := false @@ -276,8 +282,15 @@ func (c *conn) handleChannel(newChannel ssh.NewChannel) { var reply []byte switch req.Type { case "subsystem": - fs.Debugf(c.what, "Subsystem: %s\n", req.Payload[4:]) - if string(req.Payload[4:]) == "sftp" { + // The payload is a length-prefixed string, so it must be + // decoded rather than sliced to avoid panics if too short. + var subsystem struct{ Name string } + if err := ssh.Unmarshal(req.Payload, &subsystem); err != nil { + fs.Errorf(c.what, "ignoring bad subsystem request: %v", err) + break + } + fs.Debugf(c.what, "Subsystem: %s\n", subsystem.Name) + if subsystem.Name == "sftp" { ok = true subSystemIsSFTP = true } @@ -304,7 +317,12 @@ func (c *conn) handleChannel(newChannel ssh.NewChannel) { }(requests) // Wait for either subsystem "sftp" or "exec" request - if <-isSFTP { + subSystemIsSFTP, ok := <-isSFTP + if !ok { + fs.Debugf(c.what, "Channel closed without a supported request") + return + } + if subSystemIsSFTP { if err := serveChannel(channel, c.handlers, c.what); err != nil { fs.Errorf(c.what, "Failed to serve SFTP: %v", err) } diff --git a/cmd/serve/sftp/handler.go b/cmd/serve/sftp/handler.go index 1ce26e062..8cb7e522a 100644 --- a/cmd/serve/sftp/handler.go +++ b/cmd/serve/sftp/handler.go @@ -3,8 +3,10 @@ package sftp import ( + "fmt" "io" "os" + "runtime/debug" "syscall" "time" @@ -13,6 +15,36 @@ import ( "github.com/rclone/rclone/vfs" ) +// recoverPanic turns a panic into an error assigned through err. +func recoverPanic(err *error) { + if r := recover(); r != nil { + fs.Errorf("sftp", "panic in request handler: %v\n%s", r, debug.Stack()) + *err = fmt.Errorf("request handler: %v", r) + } +} + +// recoveringHandle wraps a vfs.Handle so panics in ReadAt and WriteAt, +// which pkg/sftp calls from its packet worker goroutines, are returned +// as errors. +type recoveringHandle struct { + vfs.Handle +} + +func (h recoveringHandle) ReadAt(b []byte, off int64) (n int, err error) { + defer recoverPanic(&err) + return h.Handle.ReadAt(b, off) +} + +func (h recoveringHandle) WriteAt(b []byte, off int64) (n int, err error) { + defer recoverPanic(&err) + return h.Handle.WriteAt(b, off) +} + +func (h recoveringHandle) Close() (err error) { + defer recoverPanic(&err) + return h.Handle.Close() +} + // vfsHandler converts the VFS to be served by SFTP type vfsHandler struct { *vfs.VFS @@ -29,15 +61,17 @@ func newVFSHandler(vfs *vfs.VFS) sftp.Handlers { } } -func (v vfsHandler) Fileread(r *sftp.Request) (io.ReaderAt, error) { +func (v vfsHandler) Fileread(r *sftp.Request) (ra io.ReaderAt, err error) { + defer recoverPanic(&err) file, err := v.OpenFile(r.Filepath, os.O_RDONLY, 0777) if err != nil { return nil, err } - return file, nil + return recoveringHandle{file}, nil } -func (v vfsHandler) Filewrite(r *sftp.Request) (io.WriterAt, error) { +func (v vfsHandler) Filewrite(r *sftp.Request) (wa io.WriterAt, err error) { + defer recoverPanic(&err) // Respect the flags requested in the SFTP OPEN packet p := r.Pflags() flags := os.O_WRONLY @@ -57,10 +91,11 @@ func (v vfsHandler) Filewrite(r *sftp.Request) (io.WriterAt, error) { if err != nil { return nil, err } - return file, nil + return recoveringHandle{file}, nil } -func (v vfsHandler) Filecmd(r *sftp.Request) error { +func (v vfsHandler) Filecmd(r *sftp.Request) (err error) { + defer recoverPanic(&err) switch r.Method { case "Setstat": attr := r.Attributes() @@ -119,7 +154,8 @@ func (v vfsHandler) Filecmd(r *sftp.Request) error { // StatVFS implements the statvfs@openssh.com extension, returning filesystem // usage information from the VFS. It satisfies sftp.StatVFSFileCmder. -func (v vfsHandler) StatVFS(r *sftp.Request) (*sftp.StatVFS, error) { +func (v vfsHandler) StatVFS(r *sftp.Request) (st *sftp.StatVFS, err error) { + defer recoverPanic(&err) const blockSize = 4096 total, _, free := v.Statfs() blocks := uint64(total) / blockSize @@ -140,8 +176,8 @@ func (v vfsHandler) StatVFS(r *sftp.Request) (*sftp.StatVFS, error) { type listerat []os.FileInfo // Modeled after strings.Reader's ReadAt() implementation -func (f listerat) ListAt(ls []os.FileInfo, offset int64) (int, error) { - var n int +func (f listerat) ListAt(ls []os.FileInfo, offset int64) (n int, err error) { + defer recoverPanic(&err) if offset >= int64(len(f)) { return 0, io.EOF } @@ -153,6 +189,7 @@ func (f listerat) ListAt(ls []os.FileInfo, offset int64) (int, error) { } func (v vfsHandler) Filelist(r *sftp.Request) (l sftp.ListerAt, err error) { + defer recoverPanic(&err) var node vfs.Node var handle vfs.Handle switch r.Method { diff --git a/cmd/serve/sftp/handler_test.go b/cmd/serve/sftp/handler_test.go index 32cdec212..27e7c1232 100644 --- a/cmd/serve/sftp/handler_test.go +++ b/cmd/serve/sftp/handler_test.go @@ -10,6 +10,7 @@ import ( "context" "io" "os" + "runtime" "strings" "testing" "time" @@ -18,15 +19,16 @@ import ( _ "github.com/rclone/rclone/backend/local" "github.com/rclone/rclone/cmd/serve/proxy" "github.com/rclone/rclone/fs" + "github.com/rclone/rclone/vfs" "github.com/rclone/rclone/vfs/vfscommon" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/crypto/ssh" ) -// startTestServer starts an sftp server serving a temporary local directory -// with the given VFS options and returns a connected sftp client. -func startTestServer(t *testing.T, vfsOpt *vfscommon.Options) *sftp.Client { +// startTestSSHClient starts an sftp server serving a temporary local directory +// with the given VFS options and returns an ssh client connected to it. +func startTestSSHClient(t *testing.T, vfsOpt *vfscommon.Options) *ssh.Client { ctx := context.Background() f, err := fs.NewFs(ctx, t.TempDir()) @@ -57,7 +59,13 @@ func startTestServer(t *testing.T, vfsOpt *vfscommon.Options) *sftp.Client { _ = conn.Close() }) - client, err := sftp.NewClient(conn) + return conn +} + +// startTestServer starts an sftp server as startTestSSHClient does and +// returns a connected sftp client. +func startTestServer(t *testing.T, vfsOpt *vfscommon.Options) *sftp.Client { + client, err := sftp.NewClient(startTestSSHClient(t, vfsOpt)) require.NoError(t, err) t.Cleanup(func() { _ = client.Close() @@ -202,6 +210,91 @@ func TestSetstatMtime(t *testing.T) { assert.True(t, fi.ModTime().Equal(epoch), "mtime not applied: got %v want %v", fi.ModTime(), epoch) } +// Test that a panic in a request handler is recovered and returned as an +// error to the client rather than crashing the whole server. +func TestHandlerRecoversPanic(t *testing.T) { + // A nil VFS makes every handler panic with a nil pointer dereference + // inside the vfs package, standing in for a panicking backend. + v := vfsHandler{} + + _, err := v.Fileread(&sftp.Request{Filepath: "/file"}) + assert.ErrorContains(t, err, "request handler") + + _, err = v.Filewrite(&sftp.Request{Filepath: "/file"}) + assert.ErrorContains(t, err, "request handler") + + err = v.Filecmd(&sftp.Request{Method: "Mkdir", Filepath: "/dir"}) + assert.ErrorContains(t, err, "request handler") + + _, err = v.Filelist(&sftp.Request{Method: "List", Filepath: "/"}) + assert.ErrorContains(t, err, "request handler") + + _, err = v.StatVFS(&sftp.Request{Filepath: "/"}) + assert.ErrorContains(t, err, "request handler") +} + +// panickingHandle panics on ReadAt and WriteAt, standing in for a backend +// which panics during data transfer. +type panickingHandle struct { + vfs.Handle +} + +func (panickingHandle) ReadAt([]byte, int64) (int, error) { panic("boom") } +func (panickingHandle) WriteAt([]byte, int64) (int, error) { panic("boom") } +func (panickingHandle) Close() error { panic("boom") } + +// Test that a panic during data transfer on a handle returned from +// Fileread/Filewrite is recovered and returned as an error. +func TestRecoveringHandle(t *testing.T) { + h := recoveringHandle{panickingHandle{}} + + _, err := h.ReadAt(make([]byte, 16), 0) + assert.ErrorContains(t, err, "boom") + + _, err = h.WriteAt(make([]byte, 16), 0) + assert.ErrorContains(t, err, "boom") + + assert.ErrorContains(t, h.Close(), "boom") +} + +// Test that a session request with a truncated payload is rejected rather +// than panicking the out-of-band request goroutine, which would kill the +// whole server. The subsystem payload is a length-prefixed string, so an +// empty one used to be sliced out of range. +func TestShortSubsystemRequest(t *testing.T) { + vfsOpt := vfscommon.Opt + conn := startTestSSHClient(t, &vfsOpt) + + // Rejecting the request must not leak the goroutine waiting to find out + // what kind of channel this is, or a client could exhaust the server by + // opening bad channels in a loop. + before := runtime.NumGoroutine() + const channels = 50 + for range channels { + channel, requests, err := conn.OpenChannel("session", nil) + require.NoError(t, err) + go ssh.DiscardRequests(requests) + + // Empty payload: no 4-byte length prefix at all + _, err = channel.SendRequest("subsystem", true, []byte{}) + require.NoError(t, err) + require.NoError(t, channel.Close()) + } + + assert.Eventually(t, func() bool { + return runtime.NumGoroutine() < before+channels/2 + }, 10*time.Second, 50*time.Millisecond, + "goroutines leaked: started at %d, now %d after %d rejected channels", + before, runtime.NumGoroutine(), channels) + + // The server must still be alive and serving + client, err := sftp.NewClient(conn) + require.NoError(t, err, "server died after a truncated subsystem request") + defer func() { _ = client.Close() }() + _, err = client.Stat("/") + assert.NoError(t, err) +} + // writeFile writes contents to fileName via the client truncating any existing // data, the way a normal upload does. func writeFile(client *sftp.Client, fileName, contents string) error {