diff --git a/cmd/serve/ftp/ftp.go b/cmd/serve/ftp/ftp.go index db71b90b1..6da559d79 100644 --- a/cmd/serve/ftp/ftp.go +++ b/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") } diff --git a/cmd/serve/ftp/ftp_crosssession_test.go b/cmd/serve/ftp/ftp_crosssession_test.go new file mode 100644 index 000000000..3d1896e6c --- /dev/null +++ b/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) +} diff --git a/cmd/serve/ftp/proxy_crosssession_code.go b/cmd/serve/ftp/proxy_crosssession_code.go new file mode 100644 index 000000000..744f501f3 --- /dev/null +++ b/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) + } +}