*: mitigate DNS rebinding attacks in insecure etcd server #9372

gyuho · 2018-02-28T00:19:47Z

Reference

This works

make docker-dns-test-insecure-run -f ./hack/scripts-dev/Makefile

Will add more documentation once reviewed.

/cc @mitake @zelivans @ericchiang

ericchiang · 2018-02-28T00:37:04Z

For better or for worse I think defaulting to only loopback address is going to break a lot of people.

Since we can't really guarantee anything when auth is disabled, does it make sense to only enable this when --host-whitelist is provided? This seems like a useful feature even when auth is enabled anyway.

ericchiang · 2018-02-28T00:38:16Z

And just to clarify, CVE-2018-5702 isn't about etcd, so it probably shouldn't be the issue title unless you like giving people heart attacks :)

gyuho · 2018-02-28T01:35:19Z

Also, as @ericchiang pointed out, this might break a lot of etcd clusters with no auth.
Will make host-whiltelist optional (default value to * to allow any hostnames).

codecov-io · 2018-02-28T06:19:33Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f0eb772). Click here to learn what that means.
The diff coverage is 50%.

@@            Coverage Diff            @@
##             master    #9372   +/-   ##
=========================================
  Coverage          ?   72.46%           
=========================================
  Files             ?      362           
  Lines             ?    30779           
  Branches          ?        0           
=========================================
  Hits              ?    22303           
  Misses            ?     6851           
  Partials          ?     1625

Impacted Files	Coverage Δ
etcdserver/config.go	`79.8% <ø> (ø)`
embed/config.go	`60.71% <100%> (ø)`
embed/serve.go	`64.23% <31.57%> (ø)`
embed/etcd.go	`67.32% <40%> (ø)`
etcdserver/server.go	`78.69% <60%> (ø)`
pkg/httputil/httputil.go	`80% <71.42%> (ø)`
etcdmain/config.go	`81.11% <71.42%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0eb772...9f0027d. Read the comment docs.

ericchiang · 2018-02-28T17:32:56Z

embed/serve.go

+// https://github.com/transmission/transmission/pull/468
+func errCVE20185702(host string) string {
+	return fmt.Sprintf(`
+etcd received your request, but the hostname was unrecognized.


"but the Host header was unrecognized"

ericchiang · 2018-02-28T17:36:53Z

embed/serve.go

+			// connection is not secure, or auth is not enabled
+			if req.TLS == nil || !m.s.AuthStore().IsAuthEnabled() {
+				// allow if host is in whitelist
+				ok = m.s.IsHostWhitelisted(host)


We still want to enforce a Host whitelist even if auth is enabled, right? I'd just recommend

if !m.s.IsHostWhitelisted("*") && !m.s.IsHostWhitelisted(host) { // error }

ericchiang · 2018-02-28T17:37:57Z

embed/serve.go

+		host := netutil.GetHost(req.Host)
+
+		// allow if loopback address or "*"
+		ok := netutil.IsLoopback(host) || m.s.IsHostWhitelisted("*")


I'd recommend dropping the Loopback check. If you've gone through the trouble of specifying a Host header whitelist, then you don't care about loopback (plus non-browser clients can arbitrarily set whatever Host header they want).

You are right. I was first replicating the logic in transmission/transmission#468, but this can be simplified as you suggested. Will rework on this.

ericchiang · 2018-02-28T17:40:41Z

pkg/netutil/loopback.go

+// and only returns the host in the address.
+func GetHost(addr string) (host string) {
+	addr = strings.TrimSpace(addr)
+	u, _ := url.Parse(addr)


I don't think http.Request.Host can be a URL. It can only be a host name or "host:port"

ericchiang · 2018-02-28T17:41:07Z

pkg/netutil/loopback.go

+
+// GetHost removes all schemes and ports from address
+// and only returns the host in the address.
+func GetHost(addr string) (host string) {


nit: GetHost should probably take an http.Request so it's clearer what string it's acting on.

ericchiang · 2018-02-28T17:41:46Z

etcdserver/server.go

@@ -251,6 +251,9 @@ type EtcdServer struct {

 	leadTimeMu      sync.RWMutex
 	leadElectedTime time.Time
+
+	hostWhitelistMu sync.RWMutex


nit: don't need the mutex because you're only reading from the map?

ericchiang · 2018-02-28T17:42:54Z

embed/serve.go

+
+	// enforce client origin (CVE-2018-5702)
+	// https://bugs.chromium.org/p/project-zero/issues/detail?id=1447#c2
+	if req != nil {


Why can the request be nil? Should it be rejected if there's a Host header whitelist?

gyuho · 2018-02-28T23:58:45Z

@ericchiang All addressed. PTAL. Thanks!

gyuho · 2018-02-28T23:59:23Z

Documentation/upgrades/upgrade_3_4.md

+
+`etcdserver.ServerConfig` has added a new field `HostWhitelist` and its default value is empty. If empty and etcd server does not enable TLS, all HTTP client requests will be rejected. This is to prevent [DNS Rebinding](https://en.wikipedia.org/wiki/DNS_rebinding) [attacks](https://bugs.chromium.org/p/project-zero/issues/detail?id=1447). Set it to `"*"` to allow all hostnames.
+
+Before and after (e.g. [k8s.io/kubernetes/test/e2e_node/services/etcd.go](https://github.com/kubernetes/kubernetes/blob/release-1.10/test/e2e_node/services/etcd.go#L78-L81))


@jpbetz FYI, this will be breaking change for Kubernetes testing with etcd v3.4.

Can we avoid this by having an empty HostWhiteList mean "allow all" instead of []string{"*"}.

@ericchiang Good idea. Then, we won't have to worry about breaking changes. Will change.

ericchiang

Just nits, other than that lgtm.

Thanks for getting this done!

ericchiang · 2018-03-01T00:08:21Z

embed/serve.go

@@ -265,9 +268,37 @@ func (m *httpWrapper) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 	if req != nil && req.URL != nil && strings.HasPrefix(req.URL.Path, "/v3beta/") {
 		req.URL.Path = strings.Replace(req.URL.Path, "/v3beta/", "/v3/", 1)
 	}
+
+	// client connection is insecure, then enforce client origin policy


nit: outdated comment

ericchiang · 2018-03-01T00:10:14Z

pkg/httputil/httputil.go

+	host, _, err := net.SplitHostPort(req.Host)
+	if host == "" &&
+		err != nil &&
+		strings.Contains(err.Error(), "missing port in address") {


strings.Contains? hmmm

Seems fine since there's pretty thorough testing, but this definitely isn't best practice.

ericchiang · 2018-03-01T00:11:41Z

pkg/httputil/httputil.go

+		return ""
+	}
+	host, _, err := net.SplitHostPort(req.Host)
+	if host == "" &&


err != nil should probably indicate that any host value returned is invalid, right? Why not just

h, _, err := net.SplitHostPort(req.Host) if err != nil { return req.Host } return h

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

gyuho · 2018-03-01T02:56:31Z

@ericchiang All addressed. Now Host whitelist is empty by default, to allow all hostnames.

Merging in greens. Thanks for review!

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

ericchiang · 2018-03-01T08:14:27Z

Awesome thanks!

gyuho added Release-Note/v3.4 area/security labels Feb 28, 2018

gyuho changed the title ~~CVE-2018-5702: mitigate DNS rebinding attacks in insecure server~~ *: mitigate DNS rebinding attacks in insecure server Feb 28, 2018

gyuho changed the title *: mitigate DNS rebinding attacks in insecure server *: mitigate DNS rebinding attacks in insecure etcd server Feb 28, 2018

gyuho added the WIP - DO NOT MERGE label Feb 28, 2018

gyuho force-pushed the origin branch 2 times, most recently from edd327c to 8009030 Compare February 28, 2018 05:11

gyuho removed the WIP - DO NOT MERGE label Feb 28, 2018

ericchiang reviewed Feb 28, 2018

View reviewed changes

gyuho added the WIP - DO NOT MERGE label Feb 28, 2018

gyuho force-pushed the origin branch from 8009030 to 7628420 Compare February 28, 2018 23:29

gyuho removed the WIP - DO NOT MERGE label Feb 28, 2018

gyuho commented Feb 28, 2018

View reviewed changes

ericchiang reviewed Mar 1, 2018

View reviewed changes

gyuho added the WIP - DO NOT MERGE label Mar 1, 2018

gyuho added 7 commits February 28, 2018 17:27

pkg/httputil: add "GetHostname"

418bb92

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

embed: add "HostWhitelist"

00c1f16

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

etcdmain: add "--host-whitelist" flag

0179d81

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

etcdserver: add "HostWhitelist" to "ServerConfig"

3648649

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

embed: enforce client origin policy

8edaeca

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

hack/scripts-dev/docker-dns: add "--host-whitelist" tests

e9969aa

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

CHANGELOG: add "--host-whitelist" change

d28c092

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

gyuho force-pushed the origin branch from 7628420 to b5e6954 Compare March 1, 2018 02:45

gyuho removed the WIP - DO NOT MERGE label Mar 1, 2018

Documentation/op-guide: document "--host-whitelist"

02217cb

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

gyuho force-pushed the origin branch from b5e6954 to 02217cb Compare March 1, 2018 02:55

*: do not whitelist empty hosts

9f0027d

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

gyuho merged commit a7e5790 into etcd-io:master Mar 1, 2018

gyuho deleted the origin branch March 1, 2018 04:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: mitigate DNS rebinding attacks in insecure etcd server #9372

*: mitigate DNS rebinding attacks in insecure etcd server #9372

gyuho commented Feb 28, 2018 •

edited

Loading

ericchiang commented Feb 28, 2018 •

edited

Loading

ericchiang commented Feb 28, 2018 •

edited

Loading

gyuho commented Feb 28, 2018 •

edited

Loading

codecov-io commented Feb 28, 2018 •

edited

Loading

ericchiang Feb 28, 2018

ericchiang Feb 28, 2018

ericchiang Feb 28, 2018

gyuho Feb 28, 2018 •

edited

Loading

ericchiang Feb 28, 2018

ericchiang Feb 28, 2018

ericchiang Feb 28, 2018

ericchiang Feb 28, 2018

gyuho commented Feb 28, 2018

gyuho Feb 28, 2018

ericchiang Mar 1, 2018

gyuho Mar 1, 2018

ericchiang left a comment

ericchiang Mar 1, 2018

ericchiang Mar 1, 2018

ericchiang Mar 1, 2018

gyuho commented Mar 1, 2018

ericchiang commented Mar 1, 2018


		`etcdserver.ServerConfig` has added a new field `HostWhitelist` and its default value is empty. If empty and etcd server does not enable TLS, all HTTP client requests will be rejected. This is to prevent [DNS Rebinding](https://en.wikipedia.org/wiki/DNS_rebinding) [attacks](https://bugs.chromium.org/p/project-zero/issues/detail?id=1447). Set it to `"*"` to allow all hostnames.

		Before and after (e.g. [k8s.io/kubernetes/test/e2e_node/services/etcd.go](https://github.com/kubernetes/kubernetes/blob/release-1.10/test/e2e_node/services/etcd.go#L78-L81))

*: mitigate DNS rebinding attacks in insecure etcd server #9372

*: mitigate DNS rebinding attacks in insecure etcd server #9372

Conversation

gyuho commented Feb 28, 2018 • edited Loading

ericchiang commented Feb 28, 2018 • edited Loading

ericchiang commented Feb 28, 2018 • edited Loading

gyuho commented Feb 28, 2018 • edited Loading

codecov-io commented Feb 28, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Mar 1, 2018

ericchiang commented Mar 1, 2018

gyuho commented Feb 28, 2018 •

edited

Loading

ericchiang commented Feb 28, 2018 •

edited

Loading

ericchiang commented Feb 28, 2018 •

edited

Loading

gyuho commented Feb 28, 2018 •

edited

Loading

codecov-io commented Feb 28, 2018 •

edited

Loading

gyuho Feb 28, 2018 •

edited

Loading