serve ftp: fix auth-proxy sessions sharing credentials by username GHSA-c476-6w5q-jw77 CVE-PENDING

When serving FTP with --auth-proxy, the obscured password was cached in a
driver-global map keyed only by the username. Two sessions that logged in
with the same username but different credentials shared one map entry, so a
later login overwrote it and every subsequent operation on the earlier,
still-authenticated session was re-authorized with the later session's
credential and executed against the later session's backend.

Bind the credential to the FTP session by storing the obscured password in
the per-session goftp Session.Data map instead, so each session always
resolves the backend it authenticated for.

cmd/serve/ftp/ftp.go

@@ -16,7 +16,6 @@ import (
 	"regexp"
 	"strconv"
 	"strings"
-	"sync"
 	"time"
 
 	"github.com/rclone/rclone/cmd"
@@ -166,16 +165,23 @@ You can set a single username and password with the --user and --pass flags.
 
 // driver contains everything to run the driver for the FTP server
 type driver struct {
-	f          fs.Fs
-	srv        *ftp.Server
-	ctx        context.Context // for global config
-	opt        Options
-	provider   *proxy.Provider
-	useTLS     bool
-	userPassMu sync.Mutex        // to protect userPass
-	userPass   map[string]string // cache of username => password when using vfs proxy
+	f        fs.Fs
+	srv      *ftp.Server
+	ctx      context.Context // for global config
+	opt      Options
+	provider *proxy.Provider
+	useTLS   bool
 }
 
+// sessionObscuredPassKey is the key under which the obscured password is
+// stored in the per-session ftp.Session.Data map when using the auth proxy.
+//
+// The credential must be bound to the FTP session, not to the username: two
+// sessions can share a username but resolve to different proxy backends, so a
+// username-keyed store would let a later login rebind an earlier session's
+// operations to the later session's backend.
+const sessionObscuredPassKey = "rclone-obscured-pass"
+
 func init() {
 	fs.RegisterGlobalOptions(fs.OptionsInfo{Name: "ftp", Opt: &Opt, Options: OptionsInfo})
 }
@@ -205,9 +211,6 @@ func newServer(ctx context.Context, f fs.Fs, opt *Options, vfsOpt *vfscommon.Opt
 		}
 	}()
 
-	if d.provider.IsProxy() {
-		d.userPass = make(map[string]string, 16)
-	}
 	d.useTLS = d.opt.TLSKey != ""
 
 	// Check PassivePorts format since the server library doesn't!
@@ -327,17 +330,18 @@ func (d *driver) CheckPasswd(sctx *ftp.Context, user, pass string) (ok bool, err
 			fs.Infof(nil, "proxy login failed: %v", err)
 			return false, nil
 		}
-		// Cache obscured password for later lookup.
+		// Cache the obscured password on the session for later lookup.
 		//
-		// We don't cache the VFS directly in the driver as we want them
-		// to be expired and the auth proxy does that for us.
+		// We don't cache the VFS directly as we want it to be expired and
+		// the auth proxy does that for us. We bind the credential to this
+		// FTP session rather than to the username so a later login with the
+		// same username but a different credential can't rebind this
+		// session's operations to a different backend.
 		oPass, err := obscure.Obscure(pass)
 		if err != nil {
 			return false, err
 		}
-		d.userPassMu.Lock()
-		d.userPass[user] = oPass
-		d.userPassMu.Unlock()
+		sctx.Sess.Data[sessionObscuredPassKey] = oPass
 	} else {
 		userOK := subtle.ConstantTimeCompare([]byte(d.opt.User), []byte(user))
 		// No password configured means any password is accepted
@@ -366,9 +370,7 @@ func (d *driver) getVFS(sctx *ftp.Context) (VFS *vfs.VFS, err error) {
 		return d.provider.VFS(), nil
 	}
 	user := sctx.Sess.LoginUser()
-	d.userPassMu.Lock()
-	oPass, ok := d.userPass[user]
-	d.userPassMu.Unlock()
+	oPass, ok := sctx.Sess.Data[sessionObscuredPassKey].(string)
 	if !ok {
 		return nil, fmt.Errorf("proxy user not logged in")
 	}

cmd/serve/ftp/ftp_crosssession_test.go

@@ -0,0 +1,108 @@
+// Test that auth-proxy credentials are bound to the FTP session and not
+// shared between sessions that happen to use the same username.
+
+//go:build !windows && !darwin && !plan9
+
+package ftp
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	ftpclient "github.com/jlaffaye/ftp"
+	_ "github.com/rclone/rclone/backend/local"
+	"github.com/rclone/rclone/cmd/serve/proxy"
+	"github.com/rclone/rclone/lib/israce"
+	"github.com/rclone/rclone/vfs/vfscommon"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestProxyCrossSession checks that two FTP sessions authenticating with the
+// same username but different auth-proxy credentials stay bound to their own
+// backends. A later login must not rebind an earlier, still-open session to
+// the later session's backend.
+func TestProxyCrossSession(t *testing.T) {
+	if israce.Enabled {
+		t.Skip("Skipping under race detector as underlying library is racy")
+	}
+
+	// Two roots reached with the same username but different passwords.
+	attackerRoot := t.TempDir()
+	victimRoot := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(attackerRoot, "attacker.txt"), []byte("attacker-only\n"), 0600))
+	require.NoError(t, os.WriteFile(filepath.Join(victimRoot, "victim.txt"), []byte("victim-secret\n"), 0600))
+	t.Setenv("RCLONE_TEST_ATTACKER_ROOT", attackerRoot)
+	t.Setenv("RCLONE_TEST_VICTIM_ROOT", victimRoot)
+
+	const addr = "127.0.0.1:52121"
+
+	opt := Opt
+	opt.ListenAddr = addr
+	opt.PassivePorts = testPASSIVEPORTRANGE
+
+	// The auth-proxy branch is selected from the global proxy.Opt.
+	oldAuthProxy := proxy.Opt.AuthProxy
+	proxy.Opt.AuthProxy = "go run proxy_crosssession_code.go"
+	defer func() { proxy.Opt.AuthProxy = oldAuthProxy }()
+	proxyOpt := proxy.Opt
+
+	w, err := newServer(context.Background(), nil, &opt, &vfscommon.Opt, &proxyOpt)
+	require.NoError(t, err)
+
+	quit := make(chan struct{})
+	go func() {
+		assert.NoError(t, w.Serve())
+		close(quit)
+	}()
+	defer func() {
+		require.NoError(t, w.Shutdown())
+		<-quit
+	}()
+
+	dial := func(pass string) *ftpclient.ServerConn {
+		var c *ftpclient.ServerConn
+		var err error
+		for range 100 {
+			c, err = ftpclient.Dial(addr, ftpclient.DialWithTimeout(5*time.Second))
+			if err == nil {
+				break
+			}
+			time.Sleep(10 * time.Millisecond)
+		}
+		require.NoError(t, err)
+		require.NoError(t, c.Login("shared", pass))
+		return c
+	}
+
+	// The attacker logs in and establishes its authority over its own root.
+	attacker := dial("attacker-token")
+	defer func() { _ = attacker.Quit() }()
+
+	names, err := attacker.NameList("/")
+	require.NoError(t, err)
+	assert.Contains(t, names, "attacker.txt")
+	assert.NotContains(t, names, "victim.txt")
+
+	// A second principal logs in with the same username but a different token.
+	victim := dial("victim-token")
+	defer func() { _ = victim.Quit() }()
+
+	vnames, err := victim.NameList("/")
+	require.NoError(t, err)
+	assert.Contains(t, vnames, "victim.txt")
+
+	// The still-open attacker session must remain bound to the attacker
+	// backend rather than being rebound to the victim's.
+	names, err = attacker.NameList("/")
+	require.NoError(t, err)
+	assert.Contains(t, names, "attacker.txt")
+	assert.NotContains(t, names, "victim.txt", "attacker session was rebound to the victim backend")
+
+	// And the attacker must not be able to read the victim's file.
+	_, err = attacker.Retr("victim.txt")
+	assert.Error(t, err)
+}

cmd/serve/ftp/proxy_crosssession_code.go

@@ -0,0 +1,38 @@
+//go:build ignore
+
+// A test auth proxy that maps the supplied password to a backend root.
+//
+// Both roots require the same FTP username ("shared") but different
+// passwords, so it exercises two credentials that share a username but
+// resolve to different backends. The roots are passed in the environment.
+package main
+
+import (
+	"encoding/json"
+	"log"
+	"os"
+)
+
+func main() {
+	var in map[string]string
+	if err := json.NewDecoder(os.Stdin).Decode(&in); err != nil {
+		log.Fatal(err)
+	}
+
+	roots := map[string]string{
+		"attacker-token": os.Getenv("RCLONE_TEST_ATTACKER_ROOT"),
+		"victim-token":   os.Getenv("RCLONE_TEST_VICTIM_ROOT"),
+	}
+	root, ok := roots[in["pass"]]
+	if in["user"] != "shared" || !ok {
+		os.Exit(1)
+	}
+
+	out := map[string]string{
+		"type":  "local",
+		"_root": root,
+	}
+	if err := json.NewEncoder(os.Stdout).Encode(&out); err != nil {
+		log.Fatal(err)
+	}
+}