f-denkena/rclone
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

serve sftp: don't crash the whole server on a bad request GHSA-6jcg-q3wp-x2f4

Browse Source 
Three ways a single client could deny service to everyone else connected to
the same serve sftp process:

A session "subsystem" request payload is a length-prefixed string, but it was
sliced at a fixed offset without checking its length, so a client sending a
truncated payload panicked the out-of-band request goroutine and killed the
process. Decode the payload instead, the way the neighbouring "exec" request
already does.

Rejecting a request then left the goroutine handling that channel waiting
forever to learn what kind of channel it was, because nothing was ever sent
on the channel it waits on. A client could open channels in a loop making
unsupported requests and grow the server's goroutines and memory without
bound. Signal the waiter when the requests run out so the channel is torn
down.

Separately, the request handlers - and reads, writes and closes on the file
handles they return - run on pkg/sftp packet worker goroutines which have no
panic recovery. A panic raised by a backend while serving one request took the
process down with it. Recover panics at that boundary, log them with a stack
trace, and return them to the requesting client as an error instead.

Addresses GHSA-6jcg-q3wp-x2f4.
This commit is contained in:
Nick Craig-Wood
2026-07-31 13:21:59 +01:00
parent 31f3856281
commit 50b4d4c745
3 changed files with 163 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
cmd/serve/sftp/connection.go
+21 -3
View File
@@ -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)
}
cmd/serve/sftp/handler.go
+45 -8
View File
@@ -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 {
cmd/serve/sftp/handler_test.go
+97 -4
View File
@@ -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 {