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

Never bind INADDR_ANY by default, and warn when doing so explicitly #14532

Merged
merged 3 commits into from Nov 22, 2018

Conversation

Projects
None yet
@luke-jr
Copy link
Member

commented Oct 20, 2018

A disturbingly large number of listening nodes appear to be also exposing their RPC server to the public internet. To attempt to mitigate this:

  • Only ever bind localhost by default, even if rpcallowip is specified. (A warning is given if rpcallowip is specified without rpcbind, since it doesn't really make sense to do.)
  • Warn about exposing the RPC server to untrusted networks if the user explicitly binds to any INADDR_ANY address.
  • Include a warning about untrusted networks in the --help documentation for rpcbind.
@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

what is the problem with binding to INADDR_ANY if it's rejecting all but specific IPs?

@kristapsk

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

If we change this default behaviour, it must be mentioned in the release notes.

@ch4ot1c

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

Is this is only concerning for nodes running with --enable-wallet (without --disable-wallet)?

@practicalswift

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

Very nice find @luke-jr

Very strong concept ACK

The expected contract for security or privacy sensitive daemons is to never bind to all interfaces unless the user explicitly asks for that.

@laanwj

Consider a machine with three IP-addresses on different networks: A, B and C.

Let’s say that all Bitcoin related activity such as outgoing connections etc takes place on network A.

First: the privacy leak aspect

We should not assume that the user is willing to share the information that he/she is running bitcoind also to participants on network B or C.

By listening on the RPC port also on the IP-addresses on networks B and C we leak that information to port scanning adversaries on those networks.

The fact that we are not providing any services to unknown clients (thanks to !ClientAllowed(…)) does not stop that privacy leak. The privacy leak happens with the SYN/ACK response.

Second: the attack surface aspect

Even if we are not providing any services to unknown clients (thanks to !ClientAllowed(…)) we’re still providing adversaries on network B and C an unnecessary attack surface by listening on those networks.

@luke-jr

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2018

@laanwj I don't think we've done anything to make the RPC server robust to connection attacks?

@luke-jr luke-jr force-pushed the luke-jr:rpcbind_explicit branch Oct 21, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

@laanwj I don't think we've done anything to make the RPC server robust to connection attacks?

Correct! Even though the early stage at which non-allowed connections are dropped makes it unlikely for there to be significant attack surface.

I love how clightning handles this: RPC is a local interface exposed through a UNIX socket only
sure, with some craftiness it's possible to forward it to other machines over ssh, but there's none of that logic in the client

And I think it was a mistake how bitcoind does this, it's one of those historical things, in an ideal world I'd like to get rid of all binding/IP access control logic around RPC. But it seems unrealistic to change any of that, at this stage, that'll result in even more user complaints than this.

So yes this is a step forward.

endpoints.push_back(std::make_pair("::1", defaultPort));
endpoints.push_back(std::make_pair("127.0.0.1", defaultPort));
if (gArgs.IsArgSet("-rpcallowip")) {
LogPrintf("WARNING: option -rpcallowip was specified without -rpcbind; this doesn't usually make sense\n");

This comment has been minimized.

Copy link
@hsjoberg

hsjoberg Oct 22, 2018

Contributor

Maybe we should be more clear here what the issue is instead of just suggesting that the configuration doesn't make sense.

This comment has been minimized.

Copy link
@pstratem

pstratem Nov 15, 2018

Contributor

suggest mirroring the warning for rpcbind

LogPrintf("WARNING: option -rpcallowip was ignored because -rpcbind was not specified, refusing to allow everyone to connect\n")

@promag

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

Concept ACK.

@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

utACK e0520135795563f13feedb28bad5aa3a7e9bc15d

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14741 (Indicate -rpcauth option password hashing alg by dongcarl)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@bitcoin bitcoin deleted a comment from Bitpes-stablecoin Nov 5, 2018

@meshcollider

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

utACK e052013

Should probably have a release note

@kallewoof
Copy link
Member

left a comment

utACK e0520135795563f13feedb28bad5aa3a7e9bc15d

@@ -296,9 +296,12 @@ static bool HTTPBindAddresses(struct evhttp* http)
std::vector<std::pair<std::string, uint16_t> > endpoints;

// Determine what addresses to bind to
if (!gArgs.IsArgSet("-rpcallowip")) { // Default to loopback if not allowing external IPs
if (!(gArgs.IsArgSet("-rpcallowip") && gArgs.IsArgSet("-rpcbind"))) { // Default to loopback if not allowing external IPs

This comment has been minimized.

Copy link
@kallewoof

kallewoof Nov 6, 2018

Member

In commit net: Always default rpcbind to localhost, never "all interfaces":

Nit: I think if !a || !b is easier on the eyes than if !(a && b).

@practicalswift

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

Needs rebase :-)

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

@luke-jr This has had some review, but need rebase to be eligible for merge.

@luke-jr luke-jr force-pushed the luke-jr:rpcbind_explicit branch to 27c44ef Nov 22, 2018

@luke-jr

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2018

Rebased

@DrahtBot DrahtBot removed the Needs rebase label Nov 22, 2018

@practicalswift

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

utACK 27c44ef

@meshcollider

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

re-utACK 27c44ef

@laanwj laanwj merged commit 27c44ef into bitcoin:master Nov 22, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 22, 2018

Merge #14532: Never bind INADDR_ANY by default, and warn when doing s…
…o explicitly

27c44ef rpcbind: Warn about exposing RPC to untrusted networks (Luke Dashjr)
d6a1287 CNetAddr: Add IsBindAny method to check for INADDR_ANY (Luke Dashjr)
3615003 net: Always default rpcbind to localhost, never "all interfaces" (Luke Dashjr)

Pull request description:

  A disturbingly large number of listening nodes appear to be also exposing their RPC server to the public internet. To attempt to mitigate this:

  * Only ever bind localhost by default, even if `rpcallowip` is specified. (A warning is given if `rpcallowip` is specified without `rpcbind`, since it doesn't really make sense to do.)
  * Warn about exposing the RPC server to untrusted networks if the user explicitly binds to any INADDR_ANY address.
  * Include a warning about untrusted networks in the `--help` documentation for `rpcbind`.

Tree-SHA512: 755bbca3db416a31393672eccf6675a5ee4d1eb1812cba73ebb4ff8c6b855ecc5df4c692566e9aa7b0f7d4dce6fedb9c0e9f3c265b9663aca36c4a6ba5efdbd4

@fanquake fanquake removed this from Blockers in High-priority for review Nov 26, 2018

dongcarl added a commit to dongcarl/bitcoin that referenced this pull request Dec 3, 2018

tests: Modify rpc_bind to conform to bitcoin#14532 behaviour.
- Even when rpcallowip is specified, only bind localhost
- Explicitly bind in run_allowip_test

laanwj added a commit that referenced this pull request Dec 4, 2018

Merge #14861: tests: Modify rpc_bind to conform to #14532 behaviour.
f3cf95f tests: Modify rpc_bind to conform to #14532 behaviour. (Carl Dong)

Pull request description:

  Fixes: #14792

Tree-SHA512: 5ee63a06c92dae5406515e9e483188309b82e07b760f363d8c8ec46a42fe5f75f88724759b0ac8ef596ee28a135626e0582f575855c5dfec2fbfff2249a109f7

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Dec 7, 2018

tests: Modify rpc_bind to conform to bitcoin#14532 behaviour.
- Even when rpcallowip is specified, only bind localhost
- Explicitly bind in run_allowip_test
@laanwj

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Removing "needs release note" we have

from: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.18.0-Release-Notes-Draft

The rpcallowip option can no longer be used to automatically listen
on all network interfaces. Instead, the rpcbind parameter must also
be used to specify the IP addresses to listen on. Listening for RPC
commands over a public network connection is insecure and should be
disabled, so a warning is now printed if a user selects such a
configuration. If you need to expose RPC in order to use a tool
like Docker, ensure you only bind RPC to your localhost, e.g. docker run [...] -p 127.0.0.1:8332:8332 (this is an extra :8332 over the
normal Docker port specification).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.