webdav: fix HTTPS to HTTP redirects leaking credentials GHSA-h4mf-4v27-hggj
A server that redirects an HTTPS request to a plaintext HTTP URL on the same host would cause Go's http.Client to replay the configured credentials (Basic Authorization, cookies, secret headers) over the network in cleartext. Refuse to follow such downgrade redirects by default in lib/rest and wire the webdav backend's client to use it. The `auth_redirect` option remains the opt-in escape hatch for servers that legitimately need auth preserved across redirects. Fixes GHSA-h4mf-4v27-hggj
This commit is contained in:
Nick Craig-Wood
@@ -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)
|
||||
|
||||
@@ -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")
|
||||
})
|
||||
}
|
||||
Reference in New Issue
Block a user