netstorage: stop a listing resume token moving signed requests to another host GHSA-xqmq-v4wj-rw5m CVE-PENDING

The resume start value returned by the NetStorage list API is joined
onto the endpoint URL as a URL reference to build the next listing
request. A value beginning with `//` is a network-path reference, so a
server (or an on-path attacker when `protocol = http`) which returns
`//other.example/path` could make rclone send the next request, signed
with the account secret, to a host other than the configured one.

Use rest.URLJoinRoot so the resume token can only change the path.

backend/netstorage/netstorage.go

@@ -573,10 +573,12 @@ func (f *Fs) ListR(ctx context.Context, dir string, callback fs.ListRCallback) (
 				fs.Errorf(nil, "Unable to parse URL %q: %v", f.endpointURL, err)
 				return fs.ErrorDirNotFound
 			}
-			resumeURL, err := rest.URLJoin(u, rest.URLPathEscape(resumeStart))
+			// A bad resume start must fail the listing rather than
+			// return fs.ErrorDirNotFound which sync treats as an empty
+			// directory
+			resumeURL, err := rest.URLJoinRoot(u, resumeStart)
 			if err != nil {
-				fs.Errorf(nil, "Unable to join URL %q for resumeStart %s: %v", f.endpointURL, resumeStart, err)
-				return fs.ErrorDirNotFound
+				return fmt.Errorf("unable to join URL %q for resumeStart %q: %w", f.endpointURL, resumeStart, err)
 			}
 			URL = resumeURL.String()
 		}

backend/netstorage/netstorage_internal_test.go

@@ -0,0 +1,61 @@
+package netstorage
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/rclone/rclone/fs"
+	"github.com/rclone/rclone/fs/config/configmap"
+	"github.com/rclone/rclone/fs/config/obscure"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestListResumeCannotChangeHost(t *testing.T) {
+	// The resume start returned by a list request comes from the
+	// server. It must not be able to move the signed continuation
+	// request to another host.
+	var requests int
+	other := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		requests++
+		http.Error(w, "not found", http.StatusNotFound)
+	}))
+	defer other.Close()
+	otherHost := strings.TrimPrefix(other.URL, "http://")
+
+	var listRequests int
+	configured := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		action := r.Header.Get("X-Akamai-ACS-Action")
+		switch {
+		case strings.Contains(action, "action=stat"):
+			_, _ = w.Write([]byte(`<stat directory="/123456/dir"><file type="dir" name="dir" mtime="0"/></stat>`))
+		case strings.Contains(action, "action=list"):
+			listRequests++
+			_, _ = w.Write([]byte(`<list><file type="file" name="123456/dir/a.txt" size="1" mtime="0"/><resume start="//` + otherHost + `/123456/dir/b.txt"/></list>`))
+		default:
+			http.Error(w, "unexpected action "+action, http.StatusBadRequest)
+		}
+	}))
+	defer configured.Close()
+
+	m := configmap.Simple{
+		"type":     "netstorage",
+		"protocol": "http",
+		"host":     strings.TrimPrefix(configured.URL, "http://") + "/123456/",
+		"account":  "account",
+		"secret":   obscure.MustObscure("secret"),
+	}
+	f, err := NewFs(context.Background(), "TestNetStorage", "dir", m)
+	require.NoError(t, err)
+
+	// The listing must fail loudly rather than with
+	// fs.ErrorDirNotFound which sync treats as an empty directory
+	err = f.(*Fs).ListR(context.Background(), "", func(entries fs.DirEntries) error { return nil })
+	assert.ErrorContains(t, err, "must not change the scheme, host or user")
+	assert.NotErrorIs(t, err, fs.ErrorDirNotFound)
+	assert.Equal(t, 1, listRequests, "list request sent to the configured host")
+	assert.Equal(t, 0, requests, "request sent to the other host")
+}