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

pull luke-jr wants to merge 3 commits into bitcoin:master from luke-jr:rpcbind_explicit changing 4 files +20 −5
  1. luke-jr commented at 3:01 pm on October 20, 2018: member

    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.
  2. laanwj commented at 3:44 pm on October 20, 2018: member
    what is the problem with binding to INADDR_ANY if it’s rejecting all but specific IPs?
  3. kristapsk commented at 4:01 pm on October 20, 2018: contributor
    If we change this default behaviour, it must be mentioned in the release notes.
  4. ch4ot1c commented at 5:27 pm on October 20, 2018: contributor
    Is this is only concerning for nodes running with --enable-wallet (without --disable-wallet)?
  5. practicalswift commented at 8:11 pm on October 20, 2018: contributor

    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.

  6. fanquake added the label RPC/REST/ZMQ on Oct 21, 2018
  7. fanquake added the label P2P on Oct 21, 2018
  8. luke-jr commented at 1:08 am on October 21, 2018: member
    @laanwj I don’t think we’ve done anything to make the RPC server robust to connection attacks?
  9. luke-jr force-pushed on Oct 21, 2018
  10. laanwj commented at 6:03 am on October 22, 2018: member

    @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.

  11. in src/httpserver.cpp:307 in e052013579 outdated 
    295@@ -296,9 +296,12 @@ static bool HTTPBindAddresses(struct evhttp* http)
296     std::vector<std::pair<std::string, uint16_t> > endpoints;
297 
298     // Determine what addresses to bind to
299-    if (!gArgs.IsArgSet("-rpcallowip")) { // Default to loopback if not allowing external IPs
300+    if (!(gArgs.IsArgSet("-rpcallowip") && gArgs.IsArgSet("-rpcbind"))) { // Default to loopback if not allowing external IPs
301         endpoints.push_back(std::make_pair("::1", defaultPort));
302         endpoints.push_back(std::make_pair("127.0.0.1", defaultPort));
303+        if (gArgs.IsArgSet("-rpcallowip")) {
304+            LogPrintf("WARNING: option -rpcallowip was specified without -rpcbind; this doesn't usually make sense\n");
    hsjoberg commented at 10:15 am on October 22, 2018:
    Maybe we should be more clear here what the issue is instead of just suggesting that the configuration doesn’t make sense.
    pstratem commented at 7:08 pm on November 15, 2018:

    suggest mirroring the warning for rpcbind

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

  12. promag commented at 6:36 pm on October 22, 2018: member
    Concept ACK.
  13. laanwj commented at 6:30 pm on October 25, 2018: member
    utACK e0520135795563f13feedb28bad5aa3a7e9bc15d
  14. laanwj added this to the "Blockers" column in a project
  15. DrahtBot commented at 10:16 am on November 1, 2018: member

    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.

  16. DrahtBot added the label Needs rebase on Nov 5, 2018
  17. fanquake deleted a comment on Nov 5, 2018
  18. meshcollider commented at 8:32 am on November 6, 2018: contributor

    utACK https://github.com/bitcoin/bitcoin/pull/14532/commits/e0520135795563f13feedb28bad5aa3a7e9bc15d

    Should probably have a release note

  19. in src/httpserver.cpp:303 in 763ffe50d1 outdated 
    295@@ -296,9 +296,12 @@ static bool HTTPBindAddresses(struct evhttp* http)
296     std::vector<std::pair<std::string, uint16_t> > endpoints;
297 
298     // Determine what addresses to bind to
299-    if (!gArgs.IsArgSet("-rpcallowip")) { // Default to loopback if not allowing external IPs
300+    if (!(gArgs.IsArgSet("-rpcallowip") && gArgs.IsArgSet("-rpcbind"))) { // Default to loopback if not allowing external IPs
    kallewoof commented at 8:35 am on November 6, 2018:

    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).

  20. kallewoof commented at 8:39 am on November 6, 2018: member
    utACK e0520135795563f13feedb28bad5aa3a7e9bc15d
  21. practicalswift commented at 7:10 pm on November 14, 2018: contributor
    Needs rebase :-)
  22. MarcoFalke commented at 7:07 pm on November 15, 2018: member
    @luke-jr This has had some review, but need rebase to be eligible for merge.
  23. net: Always default rpcbind to localhost, never "all interfaces" 
    We don't support binding to untrusted networks, so avoid a default where that is typical
    3615003952
  24. CNetAddr: Add IsBindAny method to check for INADDR_ANY d6a1287481
  25. rpcbind: Warn about exposing RPC to untrusted networks 27c44ef9c6
  26. luke-jr force-pushed on Nov 22, 2018
  27. luke-jr commented at 1:45 am on November 22, 2018: member
    Rebased
  28. DrahtBot removed the label Needs rebase on Nov 22, 2018
  29. practicalswift commented at 6:10 am on November 22, 2018: contributor
    utACK 27c44ef9c61f64d941ab82ec232a68141a2fde90
  31. laanwj merged this on Nov 22, 2018
  32. laanwj closed this on Nov 22, 2018
  33. laanwj referenced this in commit e77a2258e4 on Nov 22, 2018
  34. meshcollider added the label Needs release notes on Nov 22, 2018
  35. fanquake removed this from the "Blockers" column in a project
  36. dongcarl referenced this in commit eddd50d70a on Dec 3, 2018
  37. dongcarl referenced this in commit f3cf95ffdf on Dec 3, 2018
  38. laanwj referenced this in commit abbf4be181 on Dec 4, 2018
  39. HashUnlimited referenced this in commit cfd28bcf36 on Dec 7, 2018
  40. laanwj removed the label Needs release note on Mar 20, 2019
  41. laanwj commented at 2:05 pm on March 20, 2019: member

    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).

  42. kallewoof referenced this in commit 66c015529e on Oct 4, 2019
  43. PastaPastaPasta referenced this in commit 8272513994 on Apr 16, 2020
  44. PastaPastaPasta referenced this in commit acf2b18a26 on Apr 16, 2020
  45. PastaPastaPasta referenced this in commit a336969366 on Apr 19, 2020
  46. PastaPastaPasta referenced this in commit 9898400059 on Apr 20, 2020
  47. deadalnix referenced this in commit 71030e6af4 on Apr 29, 2020
  48. PastaPastaPasta referenced this in commit b8814fd4e2 on May 10, 2020
  49. PastaPastaPasta referenced this in commit df0ef1077c on May 12, 2020
  50. PastaPastaPasta referenced this in commit f08419f5ea on Jun 9, 2020
  51. PastaPastaPasta referenced this in commit 4636ff15eb on Jun 9, 2020
  52. PastaPastaPasta referenced this in commit c13a0c192c on Jun 10, 2020
  53. PastaPastaPasta referenced this in commit 2c2a7ea64d on Jun 11, 2020
  54. UdjinM6 referenced this in commit 8fd4c5ad80 on Jul 9, 2020
  55. UdjinM6 referenced this in commit e7a730f7af on Jul 9, 2020
  56. furszy referenced this in commit e44cadae3e on Jul 22, 2021
  57. furszy referenced this in commit 40ef122fb0 on Jul 22, 2021
  58. furszy referenced this in commit 89ad6ccea6 on Jul 23, 2021
  59. furszy referenced this in commit f02afa277a on Jul 25, 2021
  60. furszy referenced this in commit 3ea187a904 on Jul 26, 2021
  61. furszy referenced this in commit 453490de52 on Jul 27, 2021
  62. furszy referenced this in commit eb977cf93c on Jul 27, 2021
  63. furszy referenced this in commit 1aa138543e on Jul 29, 2021
  64. furszy referenced this in commit 6ddacfb4a0 on Jul 29, 2021
  65. furszy referenced this in commit e14f2ce1e6 on Jul 29, 2021
  66. furszy referenced this in commit 57df8b274b on Jul 30, 2021
  67. furszy referenced this in commit b6630b9ab1 on Jul 31, 2021
  68. furszy referenced this in commit b8756078f8 on Aug 1, 2021
  69. furszy referenced this in commit f6ac28da9b on Aug 4, 2021
  70. furszy referenced this in commit 9ee016117f on Aug 5, 2021
  71. furszy referenced this in commit 4101654a7b on Aug 8, 2021
  72. furszy referenced this in commit 647d60b69e on Aug 10, 2021
  73. random-zebra referenced this in commit b4751e10ce on Aug 11, 2021
  74. DrahtBot locked this on Dec 16, 2021


luke-jr laanwj kristapsk ch4ot1c practicalswift hsjoberg pstratem promag DrahtBot meshcollider kallewoof MarcoFalke

Labels
RPC/REST/ZMQ P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me