Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RPC: Dns rebind protection #15962

Merged
merged 7 commits into from
Feb 12, 2018
Merged

RPC: Dns rebind protection #15962

merged 7 commits into from
Feb 12, 2018

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Jan 24, 2018

This pr introduces the flag --rpcvhosts=<hosts>, which tells the RPC server about the hostname(s) to expect for incoming requests (defaults to localhost unless specified). This protection mechanism also does not bother preventing requests made against a raw ip address, either v4 or v6.

This prevents attacks like DNS rebinding, which bypasses SOP by simply masquerading as being within the same origin. These attacks do not utilize CORS, since they are not cross-domain.

By explicitly checking the Host-header, the server will not allow requests made against the server with a malicious host domain.

Additionally, this PR fixes a bug introduced in 54aeb8e, which caused the dumpconfig command to fail with

#build/bin/geth  --maxpeers=0 --nodiscover --rpc dumpconfig
toml: cannot marshal nil log.Logger

This PR potentially breaks applications which publish RPC ports towards the world; in order to continue doing so, geth needs to be started like this:

geth <flags> --rpcvhosts=infura.io,www.infura.io,...

There's also the possibility to use wildcards, effectively disabling the host-protection:

geth <flags> --rpcvhosts=*

@holiman holiman changed the title Dns rebind protection RPC: Dns rebind protection Jan 24, 2018
@holiman
Copy link
Contributor Author

holiman commented Jan 25, 2018

Travis errors:

ok  	github.com/ethereum/go-ethereum/eth	4.447s	coverage: 25.9% of statements
--- FAIL: TestFastCriticalRestarts (0.00s)
    --- FAIL: TestFastCriticalRestarts/protocol_64_progress_true (4.58s)
    	downloader_test.go:1819: pivot block not locked in after critical section failure
FAIL

and

--- FAIL: TestSimulation (7.30s)
	peer_test.go:426: node 5: failed to exchange bloom filter requirement in round 1. 
		

Not related.

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny changes, otherwise lgtm

@@ -383,6 +383,11 @@ var (
Usage: "Comma separated list of domains from which to accept cross origin requests (browser enforced)",
Value: "",
}
RPCVirtualHostsFlag = cli.StringFlag{
Name: "rpcvhosts",
Usage: "Comma separated list of virtual hostnames from which to accept requests (server enforced). Set to * to disable this protection.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, do we really need Set to * to disable this protection.? Just afraid the help usage text will wrap.

node/api.go Outdated
@@ -114,7 +114,7 @@ func (api *PrivateAdminAPI) PeerEvents(ctx context.Context) (*rpc.Subscription,
}

// StartRPC starts the HTTP RPC API server.
func (api *PrivateAdminAPI) StartRPC(host *string, port *int, cors *string, apis *string) (bool, error) {
func (api *PrivateAdminAPI) StartRPC(host *string, port *int, cors *string, apis *string, hosts *string) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets call this vhosts, otherwise host and hosts gets ambiguous.

node/config.go Outdated
// By explicitly checking the Host-header, the server will not allow requests
// made against the server with a malicious host domain.
// Requests using ip address directly are not affected
HTTPVirtualHostnames []string `toml:",omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, can we just call this HTTPVirtualHosts? I.e. shorted than hostNAMEs and I don't see the added value.

node/node.go Outdated
@@ -365,7 +365,7 @@ func (n *Node) stopIPC() {
}

// startHTTP initializes and starts the HTTP RPC endpoint.
func (n *Node) startHTTP(endpoint string, apis []rpc.API, modules []string, cors []string) error {
func (n *Node) startHTTP(endpoint string, apis []rpc.API, modules []string, cors []string, allowedHosts []string) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allowedHosts -> vhosts (keep te api consistent).

node/node.go Outdated
n.log.Info(fmt.Sprintf("HTTP endpoint opened: http://%s", endpoint))

n.log.Info(fmt.Sprintf("HTTP config: cors %v, vhosts%v", cors, allowedHosts))
// All listeners booted successfully
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these logs were leftovers from the auto log refactor. Could you change them to:

n.log.Info("HTTP endpoint opened", "url", fmt.Sprintf("http://%s", endpoint), "cors", strings.Join(cors, ","), "hvosts", strings.Join(vhosts, ","))

rpc/http.go Outdated
func NewHTTPServer(cors []string, hosts []string, srv *Server) *http.Server {
// Wrap the CORS-handler within a host-handler
handler := newCorsHandler(srv, cors)
handler = newHostHandler(hosts, handler)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps hosts -> vhosts and newHostHandler -> newVHostHandler

rpc/http.go Outdated
@@ -195,6 +199,50 @@ func newCorsHandler(srv *Server, allowedOrigins []string) http.Handler {
AllowedMethods: []string{http.MethodPost, http.MethodGet},
MaxAge: 600,
AllowedHeaders: []string{"*"},
Debug: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for?

rpc/http.go Outdated

// ServeHTTP serves JSON-RPC requests over HTTP, implements http.Handler
func (h *hostHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls no empty lines in these cases :P

rpc/http.go Outdated
// since they do in-domain requests against the RPC api. Instead, we can see on the Host-header
// which domain was used, and validate that against a whitelist.
type hostHandler struct {
AllowedHosts []string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of storing a slice, lets store a map[string]struct{} instead. That way we can do quick lookups without incurring a linear scanning cost for every single request.

@karalabe karalabe added this to the 1.8.0 milestone Feb 5, 2018
@holiman
Copy link
Contributor Author

holiman commented Feb 5, 2018

Addressed the concerns. I also made a last batch ot testing, after the changes were made.

testscript:

#!/bin/bash
data='{"jsonrpc":"2.0","method":"web3_clientVersion","params":[],"id":67}'

test(){
	h=$1
	echo "Target $h"
	curl -H "Content-Type: application/json" -H "host: $h" -X POST --data $data 127.0.0.1:8545
}
test localhost
test "127.0.0.1"
test "foobar.com"
test "x.localhost.com"

With std settings build/bin/geth --nodiscover --maxpeers 0 --rpc:

#bash testhosts.sh
Target localhost
{"jsonrpc":"2.0","id":67,"result":"Geth/v1.8.0-unstable-543df3f6/linux-amd64/go1.9.2"}
Target 127.0.0.1
{"jsonrpc":"2.0","id":67,"result":"Geth/v1.8.0-unstable-543df3f6/linux-amd64/go1.9.2"}
Target foobar.com
invalid host specified
Target x.localhost.com
invalid host specified

With #build/bin/geth --nodiscover --maxpeers 0 --rpc --rpcvhosts "foobar.com"

#bash testhosts.sh
Target localhost
invalid host specified
Target 127.0.0.1
{"jsonrpc":"2.0","id":67,"result":"Geth/v1.8.0-unstable-543df3f6/linux-amd64/go1.9.2"}
Target foobar.com
{"jsonrpc":"2.0","id":67,"result":"Geth/v1.8.0-unstable-543df3f6/linux-amd64/go1.9.2"}
Target x.localhost.com
invalid host specified

With #build/bin/geth --nodiscover --maxpeers 0 --rpc --rpcvhosts "foobar.com,localhost.com"

#bash testhosts.sh
Target localhost
invalid host specified
Target 127.0.0.1
{"jsonrpc":"2.0","id":67,"result":"Geth/v1.8.0-unstable-543df3f6/linux-amd64/go1.9.2"}
Target foobar.com
{"jsonrpc":"2.0","id":67,"result":"Geth/v1.8.0-unstable-543df3f6/linux-amd64/go1.9.2"}
Target x.localhost.com
invalid host specified

With #build/bin/geth --nodiscover --maxpeers 0 --rpc --rpcvhosts "foobar.com,x.localhost.com"

#bash testhosts.sh
Target localhost
invalid host specified
Target 127.0.0.1
{"jsonrpc":"2.0","id":67,"result":"Geth/v1.8.0-unstable-543df3f6/linux-amd64/go1.9.2"}
Target foobar.com
{"jsonrpc":"2.0","id":67,"result":"Geth/v1.8.0-unstable-543df3f6/linux-amd64/go1.9.2"}
Target x.localhost.com
{"jsonrpc":"2.0","id":67,"result":"Geth/v1.8.0-unstable-543df3f6/linux-amd64/go1.9.2"}

With wildcard #build/bin/geth --nodiscover --maxpeers 0 --rpc --rpcvhosts "a,b,*":

#bash testhosts.sh
Target localhost
{"jsonrpc":"2.0","id":67,"result":"Geth/v1.8.0-unstable-543df3f6/linux-amd64/go1.9.2"}
Target 127.0.0.1
{"jsonrpc":"2.0","id":67,"result":"Geth/v1.8.0-unstable-543df3f6/linux-amd64/go1.9.2"}
Target foobar.com
{"jsonrpc":"2.0","id":67,"result":"Geth/v1.8.0-unstable-543df3f6/linux-amd64/go1.9.2"}
Target x.localhost.com
{"jsonrpc":"2.0","id":67,"result":"Geth/v1.8.0-unstable-543df3f6/linux-amd64/go1.9.2"}

rpc/http.go Outdated
// The virtalHostHandler can prevent DNS rebinding attacks, which do not utilize CORS-headers,
// since they do in-domain requests against the RPC api. Instead, we can see on the Host-header
// which domain was used, and validate that against a whitelist.
type virtalHostHandler struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

virtalHostHandler forgot a u from virtUal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@karalabe karalabe merged commit 589b603 into ethereum:master Feb 12, 2018
prestonvanloon pushed a commit to prestonvanloon/go-ethereum that referenced this pull request Apr 2, 2018
* cmd,node,rpc: add allowedHosts to prevent dns rebinding attacks

* p2p,node: Fix bug with dumpconfig introduced in r54aeb8e4c0bb9f0e7a6c67258af67df3b266af3d

* rpc: add wildcard support for rpcallowedhosts + go fmt

* cmd/geth, cmd/utils, node, rpc: ignore direct ip(v4/6) addresses in rpc virtual hostnames check

* http, rpc, utils: make vhosts into map, address review concerns

* node: change log messages to use geth standard (not sprintf)

* rpc: fix spelling
mariameda pushed a commit to NiluPlatform/go-nilu that referenced this pull request Aug 23, 2018
* cmd,node,rpc: add allowedHosts to prevent dns rebinding attacks

* p2p,node: Fix bug with dumpconfig introduced in r54aeb8e4c0bb9f0e7a6c67258af67df3b266af3d

* rpc: add wildcard support for rpcallowedhosts + go fmt

* cmd/geth, cmd/utils, node, rpc: ignore direct ip(v4/6) addresses in rpc virtual hostnames check

* http, rpc, utils: make vhosts into map, address review concerns

* node: change log messages to use geth standard (not sprintf)

* rpc: fix spelling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants