From bf7fcc612d79133372b3d663fe0639a403e5467c Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Sat, 21 May 2016 13:48:55 +0000 Subject: [PATCH] cmd/syncthing: Enforce stricter CSRF policy on /rest GET requests (fixes #3134) GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3137 --- cmd/syncthing/gui.go | 4 -- cmd/syncthing/gui_csrf.go | 15 +------ cmd/syncthing/gui_test.go | 87 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 85 insertions(+), 21 deletions(-) diff --git a/cmd/syncthing/gui.go b/cmd/syncthing/gui.go index cbd745509..9df183727 100644 --- a/cmd/syncthing/gui.go +++ b/cmd/syncthing/gui.go @@ -461,10 +461,6 @@ func corsMiddleware(next http.Handler) http.Handler { // // See https://www.w3.org/TR/cors/ for details. return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Add a generous access-control-allow-origin header since we may be - // redirecting REST requests over protocols - w.Header().Add("Access-Control-Allow-Origin", "*") - // Process OPTIONS requests if r.Method == "OPTIONS" { // Only GET/POST Methods are supported diff --git a/cmd/syncthing/gui_csrf.go b/cmd/syncthing/gui_csrf.go index ce314171c..c17c394af 100644 --- a/cmd/syncthing/gui_csrf.go +++ b/cmd/syncthing/gui_csrf.go @@ -41,7 +41,8 @@ func csrfMiddleware(unique string, prefix string, cfg config.GUIConfiguration, n return } - // Allow requests for the front page, and set a CSRF cookie if there isn't already a valid one. + // Allow requests for anything not under the protected path prefix, + // and set a CSRF cookie if there isn't already a valid one. if !strings.HasPrefix(r.URL.Path, prefix) { cookie, err := r.Cookie("CSRF-Token-" + unique) if err != nil || !validCsrfToken(cookie.Value) { @@ -56,18 +57,6 @@ func csrfMiddleware(unique string, prefix string, cfg config.GUIConfiguration, n return } - if r.Method == "GET" { - // Allow GET requests unconditionally, but if we got the CSRF - // token cookie do the verification anyway so we keep the - // csrfTokens list sorted by recent usage. We don't care about the - // outcome of the validity check. - if cookie, err := r.Cookie("CSRF-Token-" + unique); err == nil { - validCsrfToken(cookie.Value) - } - next.ServeHTTP(w, r) - return - } - // Verify the CSRF token token := r.Header.Get("X-CSRF-Token-" + unique) if !validCsrfToken(token) { diff --git a/cmd/syncthing/gui_test.go b/cmd/syncthing/gui_test.go index bee17377d..4e58dda8a 100644 --- a/cmd/syncthing/gui_test.go +++ b/cmd/syncthing/gui_test.go @@ -189,7 +189,9 @@ type httpTestCase struct { } func TestAPIServiceRequests(t *testing.T) { + const testAPIKey = "foobarbaz" cfg := new(mockedConfig) + cfg.gui.APIKey = testAPIKey baseURL, err := startHTTP(cfg) if err != nil { t.Fatal(err) @@ -344,13 +346,13 @@ func TestAPIServiceRequests(t *testing.T) { for _, tc := range cases { t.Log("Testing", tc.URL, "...") - testHTTPRequest(t, baseURL, tc) + testHTTPRequest(t, baseURL, tc, testAPIKey) } } // testHTTPRequest tries the given test case, comparing the result code, // content type, and result prefix. -func testHTTPRequest(t *testing.T, baseURL string, tc httpTestCase) { +func testHTTPRequest(t *testing.T, baseURL string, tc httpTestCase, apikey string) { timeout := time.Second if tc.Timeout > 0 { timeout = tc.Timeout @@ -359,7 +361,14 @@ func testHTTPRequest(t *testing.T, baseURL string, tc httpTestCase) { Timeout: timeout, } - resp, err := cli.Get(baseURL + tc.URL) + req, err := http.NewRequest("GET", baseURL+tc.URL, nil) + if err != nil { + t.Errorf("Unexpected error requesting %s: %v", tc.URL, err) + return + } + req.Header.Set("X-API-Key", apikey) + + resp, err := cli.Do(req) if err != nil { t.Errorf("Unexpected error requesting %s: %v", tc.URL, err) return @@ -400,7 +409,7 @@ func TestHTTPLogin(t *testing.T) { // Verify rejection when not using authorization - req, _ := http.NewRequest("GET", baseURL+"/rest/system/status", nil) + req, _ := http.NewRequest("GET", baseURL, nil) resp, err := http.DefaultClient.Do(req) if err != nil { t.Fatal(err) @@ -491,3 +500,73 @@ func startHTTP(cfg *mockedConfig) (string, error) { return baseURL, nil } + +func TestCSRFRequired(t *testing.T) { + const testAPIKey = "foobarbaz" + cfg := new(mockedConfig) + cfg.gui.APIKey = testAPIKey + baseURL, err := startHTTP(cfg) + + cli := &http.Client{ + Timeout: time.Second, + } + + // Getting the base URL (i.e. "/") should succeed. + + resp, err := cli.Get(baseURL) + if err != nil { + t.Fatal("Unexpected error from getting base URL:", err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatal("Getting base URL should succeed, not", resp.Status) + } + + // Find the returned CSRF token for future use + + var csrfTokenName, csrfTokenValue string + for _, cookie := range resp.Cookies() { + if strings.HasPrefix(cookie.Name, "CSRF-Token") { + csrfTokenName = cookie.Name + csrfTokenValue = cookie.Value + break + } + } + + // Calling on /rest without a token should fail + + resp, err = cli.Get(baseURL + "/rest/system/config") + if err != nil { + t.Fatal("Unexpected error from getting /rest/system/config:", err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusForbidden { + t.Fatal("Getting /rest/system/config without CSRF token should fail, not", resp.Status) + } + + // Calling on /rest with a token should succeed + + req, _ := http.NewRequest("GET", baseURL+"/rest/system/config", nil) + req.Header.Set("X-"+csrfTokenName, csrfTokenValue) + resp, err = cli.Do(req) + if err != nil { + t.Fatal("Unexpected error from getting /rest/system/config:", err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatal("Getting /rest/system/config with CSRF token should succeed, not", resp.Status) + } + + // Calling on /rest with the API key should succeed + + req, _ = http.NewRequest("GET", baseURL+"/rest/system/config", nil) + req.Header.Set("X-API-Key", testAPIKey) + resp, err = cli.Do(req) + if err != nil { + t.Fatal("Unexpected error from getting /rest/system/config:", err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatal("Getting /rest/system/config with API key should succeed, not", resp.Status) + } +}