diff --git a/backend/webdav/webdav.go b/backend/webdav/webdav.go index 172f2c6ae..e3eac5239 100644 --- a/backend/webdav/webdav.go +++ b/backend/webdav/webdav.go @@ -180,6 +180,10 @@ to an unknown webserver. However this is desirable in some circumstances. If you are getting an error like "401 Unauthorized" when rclone is attempting to read files from the webdav server then you can try this option. + +Note that enabling this also permits sending your credentials over a +plaintext HTTP connection if the server redirects from HTTPS to HTTP, +which rclone otherwise refuses to do. `, Advanced: true, Default: false, @@ -511,6 +515,8 @@ func NewFs(ctx context.Context, name, root string, m configmap.Mapper) (fs.Fs, e rt: ntlmssp.Negotiator{RoundTripper: t}, } } + // Refuse redirects that downgrade HTTPS to plaintext HTTP. + client.CheckRedirect = rest.RefuseHTTPSDowngradeRedirectFn f.srv = rest.NewClient(client).SetRoot(u.String()) f.features = (&fs.Features{ diff --git a/lib/rest/rest.go b/lib/rest/rest.go index 3a36a0343..91d324ee8 100644 --- a/lib/rest/rest.go +++ b/lib/rest/rest.go @@ -215,22 +215,55 @@ func ClientWithNoRedirects(c *http.Client) *http.Client { return &clientCopy } +// ErrHTTPSDowngrade is returned by the redirect handlers when a server tries to +// redirect an HTTPS request to a plaintext HTTP URL. Following such a redirect +// would replay any credentials over the network in cleartext, so rclone refuses. +var ErrHTTPSDowngrade = errors.New("refusing to follow HTTPS to HTTP redirect: would send credentials in cleartext") + +// isHTTPSDowngrade reports whether following the redirect to req would +// move from an https:// URL to a plaintext http:// URL. +func isHTTPSDowngrade(req *http.Request, via []*http.Request) bool { + if len(via) == 0 { + return false + } + prev := via[len(via)-1] + return prev.URL.Scheme == "https" && req.URL.Scheme == "http" +} + // PreserveMethodRedirectFn is a CheckRedirect function that // preserves the original HTTP method on redirects. // // By default Go's http.Client changes the method to GET on 301, 302, // and 303 redirects. This function overrides that behaviour so the // original method (e.g. PROPFIND being preserved across a 307) is kept. +// +// It refuses an HTTPS to HTTP downgrade with ErrHTTPSDowngrade. func PreserveMethodRedirectFn(req *http.Request, via []*http.Request) error { if len(via) >= 10 { return errors.New("stopped after 10 redirects") } + if isHTTPSDowngrade(req, via) { + return ErrHTTPSDowngrade + } if len(via) > 0 { req.Method = via[0].Method } return nil } +// RefuseHTTPSDowngradeRedirectFn is a CheckRedirect function that follows +// redirects like the default net/http client but refuses to follow one +// that downgrades from HTTPS to plaintext HTTP, returning ErrHTTPSDowngrade. +func RefuseHTTPSDowngradeRedirectFn(req *http.Request, via []*http.Request) error { + if len(via) >= 10 { + return errors.New("stopped after 10 redirects") + } + if isHTTPSDowngrade(req, via) { + return ErrHTTPSDowngrade + } + return nil +} + // Do calls the internal http.Client.Do method func (api *Client) Do(req *http.Request) (*http.Response, error) { return api.c.Do(req) diff --git a/lib/rest/rest_test.go b/lib/rest/rest_test.go new file mode 100644 index 000000000..94fa6dfd1 --- /dev/null +++ b/lib/rest/rest_test.go @@ -0,0 +1,146 @@ +package rest + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// mkRedirectReq makes a minimal *http.Request pointing at rawURL for +// exercising the CheckRedirect functions. +func mkRedirectReq(t *testing.T, rawURL, method string) *http.Request { + u, err := url.Parse(rawURL) + require.NoError(t, err) + return &http.Request{URL: u, Method: method, Header: http.Header{}} +} + +func TestPreserveMethodRedirectFn(t *testing.T) { + t.Run("PreservesMethod", func(t *testing.T) { + orig := mkRedirectReq(t, "https://example.com/a", "PROPFIND") + next := mkRedirectReq(t, "https://example.com/b", "GET") + require.NoError(t, PreserveMethodRedirectFn(next, []*http.Request{orig})) + assert.Equal(t, "PROPFIND", next.Method) + }) + t.Run("RefusesDowngrade", func(t *testing.T) { + orig := mkRedirectReq(t, "https://example.com/a", "PROPFIND") + next := mkRedirectReq(t, "http://example.com/b", "PROPFIND") + assert.ErrorIs(t, PreserveMethodRedirectFn(next, []*http.Request{orig}), ErrHTTPSDowngrade) + }) + t.Run("AllowsCrossHostHTTPS", func(t *testing.T) { + orig := mkRedirectReq(t, "https://example.com/a", "PROPFIND") + next := mkRedirectReq(t, "https://other.example.com/b", "PROPFIND") + assert.NoError(t, PreserveMethodRedirectFn(next, []*http.Request{orig})) + }) + t.Run("AllowsPlainHTTP", func(t *testing.T) { + orig := mkRedirectReq(t, "http://example.com/a", "PROPFIND") + next := mkRedirectReq(t, "http://example.com/b", "PROPFIND") + assert.NoError(t, PreserveMethodRedirectFn(next, []*http.Request{orig})) + }) + t.Run("TooManyRedirects", func(t *testing.T) { + next := mkRedirectReq(t, "https://example.com/b", "GET") + via := make([]*http.Request, 10) + assert.Error(t, PreserveMethodRedirectFn(next, via)) + }) +} + +func TestRefuseHTTPSDowngradeRedirectFn(t *testing.T) { + t.Run("RefusesDowngrade", func(t *testing.T) { + orig := mkRedirectReq(t, "https://example.com/a", "GET") + next := mkRedirectReq(t, "http://example.com/b", "GET") + assert.ErrorIs(t, RefuseHTTPSDowngradeRedirectFn(next, []*http.Request{orig}), ErrHTTPSDowngrade) + }) + t.Run("AllowsUpgrade", func(t *testing.T) { + orig := mkRedirectReq(t, "http://example.com/a", "GET") + next := mkRedirectReq(t, "https://example.com/b", "GET") + assert.NoError(t, RefuseHTTPSDowngradeRedirectFn(next, []*http.Request{orig})) + }) + t.Run("AllowsSameScheme", func(t *testing.T) { + orig := mkRedirectReq(t, "https://example.com/a", "GET") + next := mkRedirectReq(t, "https://example.com/b", "GET") + assert.NoError(t, RefuseHTTPSDowngradeRedirectFn(next, []*http.Request{orig})) + }) + t.Run("TooManyRedirects", func(t *testing.T) { + next := mkRedirectReq(t, "https://example.com/b", "GET") + via := make([]*http.Request, 10) + assert.Error(t, RefuseHTTPSDowngradeRedirectFn(next, via)) + }) +} + +// newDowngradeServers returns an HTTPS server that redirects every +// request to a plaintext HTTP server on the same host, together with a +// flag that records whether the plaintext server ever received an +// Authorization header. Use tlsSrv.Client() for a client that trusts the +// test certificate. +func newDowngradeServers(t *testing.T) (tlsSrv *httptest.Server, sawAuth *atomic.Bool) { + t.Helper() + sawAuth = new(atomic.Bool) + + // Plaintext HTTP target that records whether it received credentials. + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") != "" { + sawAuth.Store(true) + } + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(target.Close) + + // HTTPS server that redirects to the plaintext target on the same host. + tlsSrv = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, target.URL+r.URL.Path, http.StatusTemporaryRedirect) + })) + t.Cleanup(tlsSrv.Close) + + return tlsSrv, sawAuth +} + +// TestRefuseHTTPSDowngradeRedirectEndToEnd drives a real HTTPS-to-HTTP +// redirect through rest.Client with credentials set via SetUserPass, the +// same way the webdav backend does. It shows that the default client +// leaks credentials to the plaintext hop, whereas the redirect handlers +// refuse to follow the downgrade and the plaintext hop never sees them. +func TestRefuseHTTPSDowngradeRedirectEndToEnd(t *testing.T) { + ctx := context.Background() + + // Baseline: without any check the credentials set by SetUserPass are + // forwarded over the plaintext hop, which is the vulnerability. + t.Run("DefaultLeaks", func(t *testing.T) { + tlsSrv, sawAuth := newDowngradeServers(t) + api := NewClient(tlsSrv.Client()).SetRoot(tlsSrv.URL) + api.SetUserPass("user", "pass") + _, err := api.Call(ctx, &Opts{Method: "GET", Path: "/file", NoResponse: true}) + require.NoError(t, err) + assert.True(t, sawAuth.Load(), "expected credentials to be sent over plaintext") + }) + + // Default path (all normal webdav calls): the client refuses the + // downgrade so nothing is sent. + t.Run("DefaultPathRefused", func(t *testing.T) { + tlsSrv, sawAuth := newDowngradeServers(t) + client := tlsSrv.Client() + client.CheckRedirect = RefuseHTTPSDowngradeRedirectFn + api := NewClient(client).SetRoot(tlsSrv.URL) + api.SetUserPass("user", "pass") + _, err := api.Call(ctx, &Opts{Method: "GET", Path: "/file", NoResponse: true}) + require.Error(t, err) + assert.ErrorIs(t, err, ErrHTTPSDowngrade) + assert.False(t, sawAuth.Load(), "plaintext hop must not receive credentials") + }) + + // PROPFIND path (readMetaDataForPath): the per-call CheckRedirect + // refuses the downgrade too. + t.Run("PropfindPathRefused", func(t *testing.T) { + tlsSrv, sawAuth := newDowngradeServers(t) + api := NewClient(tlsSrv.Client()).SetRoot(tlsSrv.URL) + api.SetUserPass("user", "pass") + _, err := api.Call(ctx, &Opts{Method: "PROPFIND", Path: "/dir", NoResponse: true, CheckRedirect: PreserveMethodRedirectFn}) + require.Error(t, err) + assert.ErrorIs(t, err, ErrHTTPSDowngrade) + assert.False(t, sawAuth.Load(), "plaintext hop must not receive credentials") + }) +}