From 49910a1d853a8c392e5faa00c3c2aee0fc060d78 Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Sat, 3 Sep 2016 08:33:34 +0000 Subject: [PATCH] lib/config, cmd/syncthing: Enforce localhost only connections When the GUI/API is bound to localhost, we enforce that the Host header looks like localhost. This can be disabled by setting insecureSkipHostCheck in the GUI config. GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3558 --- cmd/syncthing/gui.go | 31 +++++- cmd/syncthing/gui_test.go | 195 ++++++++++++++++++++++++++++++++- lib/config/guiconfiguration.go | 19 ++-- 3 files changed, 233 insertions(+), 12 deletions(-) diff --git a/cmd/syncthing/gui.go b/cmd/syncthing/gui.go index a39def1da..ce36bd926 100644 --- a/cmd/syncthing/gui.go +++ b/cmd/syncthing/gui.go @@ -313,6 +313,11 @@ func (s *apiService) Serve() { // Add the CORS handling handler = corsMiddleware(handler) + if addressIsLocalhost(guiCfg.Address()) && !guiCfg.InsecureSkipHostCheck { + // Verify source host + handler = localhostMiddleware(handler) + } + handler = debugMiddleware(handler) srv := http.Server{ @@ -495,6 +500,17 @@ func withDetailsMiddleware(id protocol.DeviceID, h http.Handler) http.Handler { }) } +func localhostMiddleware(h http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if addressIsLocalhost(r.Host) { + h.ServeHTTP(w, r) + return + } + + http.Error(w, "Host check error", http.StatusForbidden) + }) +} + func (s *apiService) whenDebugging(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if s.cfg.GUI().Debugging { @@ -503,7 +519,6 @@ func (s *apiService) whenDebugging(h http.Handler) http.Handler { } http.Error(w, "Debugging disabled", http.StatusBadRequest) - return }) } @@ -1292,3 +1307,17 @@ func dirNames(dir string) []string { sort.Strings(dirs) return dirs } + +func addressIsLocalhost(addr string) bool { + host, _, err := net.SplitHostPort(addr) + if err != nil { + // There was no port, so we assume the address was just a hostname + host = addr + } + switch host { + case "127.0.0.1", "::1", "localhost": + return true + default: + return false + } +} diff --git a/cmd/syncthing/gui_test.go b/cmd/syncthing/gui_test.go index bdbf58a16..239d4328c 100644 --- a/cmd/syncthing/gui_test.go +++ b/cmd/syncthing/gui_test.go @@ -16,6 +16,7 @@ import ( "net" "net/http" "net/http/httptest" + "strconv" "strings" "testing" "time" @@ -460,7 +461,6 @@ func TestHTTPLogin(t *testing.T) { if resp.StatusCode != http.StatusOK { t.Errorf("Unexpected non-200 return code %d for authed request (ISO-8859-1)", resp.StatusCode) } - } func startHTTP(cfg *mockedConfig) (string, error) { @@ -491,7 +491,12 @@ func startHTTP(cfg *mockedConfig) (string, error) { if err != nil { return "", fmt.Errorf("Weird address from API service: %v", err) } - baseURL := fmt.Sprintf("http://127.0.0.1:%d", tcpAddr.Port) + + host, _, _ := net.SplitHostPort(cfg.gui.RawAddress) + if host == "" || host == "0.0.0.0" { + host = "127.0.0.1" + } + baseURL := fmt.Sprintf("http://%s", net.JoinHostPort(host, strconv.Itoa(tcpAddr.Port))) return baseURL, nil } @@ -666,3 +671,189 @@ func testConfigPost(data io.Reader) (*http.Response, error) { req.Header.Set("X-API-Key", testAPIKey) return cli.Do(req) } + +func TestHostCheck(t *testing.T) { + // An API service bound to localhost should reject non-localhost host Headers + + cfg := new(mockedConfig) + cfg.gui.RawAddress = "127.0.0.1:0" + baseURL, err := startHTTP(cfg) + if err != nil { + t.Fatal(err) + } + + // A normal HTTP get to the localhost-bound service should succeed + + resp, err := http.Get(baseURL) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Error("Regular HTTP get: expected 200 OK, not", resp.Status) + } + + // A request with a suspicious Host header should fail + + req, _ := http.NewRequest("GET", baseURL, nil) + req.Host = "example.com" + resp, err = http.DefaultClient.Do(req) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusForbidden { + t.Error("Suspicious Host header: expected 403 Forbidden, not", resp.Status) + } + + // A request with an explicit "localhost:8384" Host header should pass + + req, _ = http.NewRequest("GET", baseURL, nil) + req.Host = "localhost:8384" + resp, err = http.DefaultClient.Do(req) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Error("Explicit localhost:8384: expected 200 OK, not", resp.Status) + } + + // A request with an explicit "localhost" Host header (no port) should pass + + req, _ = http.NewRequest("GET", baseURL, nil) + req.Host = "localhost" + resp, err = http.DefaultClient.Do(req) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Error("Explicit localhost: expected 200 OK, not", resp.Status) + } + + // A server with InsecureSkipHostCheck set behaves differently + + cfg = new(mockedConfig) + cfg.gui.RawAddress = "127.0.0.1:0" + cfg.gui.InsecureSkipHostCheck = true + baseURL, err = startHTTP(cfg) + if err != nil { + t.Fatal(err) + } + + // A request with a suspicious Host header should be allowed + + req, _ = http.NewRequest("GET", baseURL, nil) + req.Host = "example.com" + resp, err = http.DefaultClient.Do(req) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Error("Incorrect host header, check disabled: expected 200 OK, not", resp.Status) + } + + // A server bound to a wildcard address also doesn't do the check + + cfg = new(mockedConfig) + cfg.gui.RawAddress = "0.0.0.0:0" + cfg.gui.InsecureSkipHostCheck = true + baseURL, err = startHTTP(cfg) + if err != nil { + t.Fatal(err) + } + + // A request with a suspicious Host header should be allowed + + req, _ = http.NewRequest("GET", baseURL, nil) + req.Host = "example.com" + resp, err = http.DefaultClient.Do(req) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Error("Incorrect host header, wildcard bound: expected 200 OK, not", resp.Status) + } + + // This should all work over IPv6 as well + + cfg = new(mockedConfig) + cfg.gui.RawAddress = "[::1]:0" + baseURL, err = startHTTP(cfg) + if err != nil { + t.Fatal(err) + } + + // A normal HTTP get to the localhost-bound service should succeed + + resp, err = http.Get(baseURL) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Error("Regular HTTP get (IPv6): expected 200 OK, not", resp.Status) + } + + // A request with a suspicious Host header should fail + + req, _ = http.NewRequest("GET", baseURL, nil) + req.Host = "example.com" + resp, err = http.DefaultClient.Do(req) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusForbidden { + t.Error("Suspicious Host header (IPv6): expected 403 Forbidden, not", resp.Status) + } + + // A request with an explicit "localhost:8384" Host header should pass + + req, _ = http.NewRequest("GET", baseURL, nil) + req.Host = "localhost:8384" + resp, err = http.DefaultClient.Do(req) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Error("Explicit localhost:8384 (IPv6): expected 200 OK, not", resp.Status) + } +} + +func TestAddressIsLocalhost(t *testing.T) { + testcases := []struct { + address string + result bool + }{ + // These are all valid localhost addresses + {"localhost", true}, + {"::1", true}, + {"127.0.0.1", true}, + {"localhost:8080", true}, + {"[::1]:8080", true}, + {"127.0.0.1:8080", true}, + + // These are all non-localhost addresses + {"example.com", false}, + {"example.com:8080", false}, + {"192.0.2.10", false}, + {"192.0.2.10:8080", false}, + {"0.0.0.0", false}, + {"0.0.0.0:8080", false}, + {"::", false}, + {"[::]:8080", false}, + {":8080", false}, + } + + for _, tc := range testcases { + result := addressIsLocalhost(tc.address) + if result != tc.result { + t.Errorf("addressIsLocalhost(%q)=%v, expected %v", tc.address, result, tc.result) + } + } +} diff --git a/lib/config/guiconfiguration.go b/lib/config/guiconfiguration.go index 6a4e7e473..193c2a950 100644 --- a/lib/config/guiconfiguration.go +++ b/lib/config/guiconfiguration.go @@ -13,15 +13,16 @@ import ( ) type GUIConfiguration struct { - Enabled bool `xml:"enabled,attr" json:"enabled" default:"true"` - RawAddress string `xml:"address" json:"address" default:"127.0.0.1:8384"` - User string `xml:"user,omitempty" json:"user"` - Password string `xml:"password,omitempty" json:"password"` - RawUseTLS bool `xml:"tls,attr" json:"useTLS"` - APIKey string `xml:"apikey,omitempty" json:"apiKey"` - InsecureAdminAccess bool `xml:"insecureAdminAccess,omitempty" json:"insecureAdminAccess"` - Theme string `xml:"theme" json:"theme" default:"default"` - Debugging bool `xml:"debugging,attr" json:"debugging"` + Enabled bool `xml:"enabled,attr" json:"enabled" default:"true"` + RawAddress string `xml:"address" json:"address" default:"127.0.0.1:8384"` + User string `xml:"user,omitempty" json:"user"` + Password string `xml:"password,omitempty" json:"password"` + RawUseTLS bool `xml:"tls,attr" json:"useTLS"` + APIKey string `xml:"apikey,omitempty" json:"apiKey"` + InsecureAdminAccess bool `xml:"insecureAdminAccess,omitempty" json:"insecureAdminAccess"` + Theme string `xml:"theme" json:"theme" default:"default"` + Debugging bool `xml:"debugging,attr" json:"debugging"` + InsecureSkipHostCheck bool `xml:"insecureSkipHostcheck,omitempty" json:"insecureSkipHostcheck"` } func (c GUIConfiguration) Address() string {