rpc: Add rpcallowstop #16544

emilengler commented at 5:30 AM on August 4, 2019: contributor

This adds a feature to disable to ability to stop Bitcoin Core over the JSON RPC interface except if the sender is localhost. Based on a suggestion from hebasto in #16543.

All in all it adds a new parameter called rpcallowstop which is by default set to 1. If the parameter is set to 0 it will block all RPC requests with stop Bitcoin Core except if the sender is localhost.

I tested it with the following config file:

regtest=1

[regtest]
server=1
rpcallowstop=0
rpcport=8332
rpcuser=foo
rpcpassword=bar
rpcallowip=192.168.178.71
rpcallowip=127.0.0.1
rpcbind=192.168.178.71
rpcbind=127.0.0.1

A curl request using the internal ip would result in an error. However using 127.0.0.1 would work.

rpc: Add rpcallowstop 514faf30a2

fanquake added the label RPC/REST/ZMQ on Aug 4, 2019

DrahtBot commented at 7:43 AM on August 4, 2019: 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:

#16489 (log: harmonize bitcoind logging by jonatack)

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.

marpme changes_requested

marpme commented at 7:46 AM on August 4, 2019: none

I suggest changing the default value to: false.

As it opens an attack vector to stop all core nodes remotely and additionally previously none of the users would expect that remote servers could stop their nodes.

in src/rpc/server.cpp:173 in 514faf30a2 outdated

 169 | @@ -169,11 +170,39 @@ UniValue stop(const JSONRPCRequest& jsonRequest)
 170 |              }.ToString());
 171 |      // Event loop will exit after current HTTP requests have been handled, so
 172 |      // this reply will get back to the client.
 173 | -    StartShutdown();
 174 | -    if (jsonRequest.params[0].isNum()) {
 175 | -        MilliSleep(jsonRequest.params[0].get_int());
 176 | +    if(rpcallowstop)

hebasto commented at 8:03 AM on August 4, 2019:

Could you follow Coding Style (C++)?

emilengler commented at 3:14 PM on August 4, 2019:

Sure thing

emilengler commented at 3:18 PM on August 4, 2019:

Done

hebasto commented at 3:35 PM on August 4, 2019:

$ git diff -U0 HEAD~2.. src/rpc/server.cpp | ./contrib/devtools/clang-format-diff.py -p1 -v
--- src/rpc/server.cpp	(before formatting)
+++ src/rpc/server.cpp	(after formatting)
@@ -170,36 +170,28 @@
             }.ToString());
     // Event loop will exit after current HTTP requests have been handled, so
     // this reply will get back to the client.
-    if (rpcallowstop)
-    {
+    if (rpcallowstop) {
         StartShutdown();
         if (jsonRequest.params[0].isNum()) {
             MilliSleep(jsonRequest.params[0].get_int());
         }
         return "Bitcoin server stopping";
-    }
-    else
-    {
+    } else {
         // Check if IP is 127.0.0.1
         std::string ip;
-        for (int i = 0; i < jsonRequest.peerAddr.length(); i++)
-        {
-            if (jsonRequest.peerAddr[i] == ':')
-            {
+        for (int i = 0; i < jsonRequest.peerAddr.length(); i++) {
+            if (jsonRequest.peerAddr[i] == ':') {
                 break;
             }
             ip += jsonRequest.peerAddr[i];
         }
-        if (ip == "127.0.0.1")
-        {
+        if (ip == "127.0.0.1") {
             StartShutdown();
             if (jsonRequest.params[0].isNum()) {
                 MilliSleep(jsonRequest.params[0].get_int());
             }
             return "Bitcoin server stopping";
-        }
-        else
-        {
+        } else {
             throw JSONRPCError(RPC_FORBIDDEN_BY_SAFE_MODE, "You are not allowed to stop");
         }
     }

hebasto commented at 3:37 PM on August 4, 2019:

You can use the provided clang-format-diff script tool to clean up patches automatically before submission.

hebasto changes_requested

hebasto commented at 8:03 AM on August 4, 2019: member

Concept ACK

in src/rpc/server.cpp:195 in 514faf30a2 outdated

 193 | +            }
 194 | +            ip += jsonRequest.peerAddr[i];
 195 | +        }
 196 | +        if(ip == "127.0.0.1")
 197 | +        {
 198 | +            StartShutdown();

jonatack commented at 10:02 AM on August 4, 2019:

This block is redundant with the one at L174 and could be extracted.

emilengler commented at 3:13 PM on August 4, 2019:

Are you sure about this? This block should only be executed if remote stop is disabled so a separate string slicing even if it isn't necessary is a bit too much overhead imo.

in src/init.cpp:520 in 514faf30a2 outdated

 516 | @@ -517,6 +517,7 @@ void SetupServerArgs()
 517 |  
 518 |      gArgs.AddArg("-rest", strprintf("Accept public REST requests (default: %u)", DEFAULT_REST_ENABLE), ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
 519 |      gArgs.AddArg("-rpcallowip=<ip>", "Allow JSON-RPC connections from specified source. Valid for <ip> are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0) or a network/CIDR (e.g. 1.2.3.4/24). This option can be specified multiple times", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
 520 | +    gArgs.AddArg("-rpcallowstop", "Allow stop over a remote RPC client", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);

jonatack commented at 10:07 AM on August 4, 2019:

Suggest: "Accept shutdown (stop) commands from remote RPC clients."

emilengler commented at 3:11 PM on August 4, 2019:

I also thought about this but I want that it is clear that it is referring to the "stop" command.

jonatack commented at 10:08 AM on August 4, 2019: member

Needs test coverage. EDIT:

#12763 is a more general solution

Agreed.

promag commented at 1:46 PM on August 4, 2019: member

I suggest changing the default value to: false.

As it opens an attack vector to stop all core nodes remotely and additionally previously none of the users would expect that remote servers could stop their nodes. @marpme that would be a breaking change. You should always restrict RPC access anyway.

promag commented at 1:51 PM on August 4, 2019: member

Maybe support any RPC, like -rpcrestrict=sendtoaddress,sendmany,stop?

Edit: 👇

ryanofsky commented at 1:53 PM on August 4, 2019: member

#12763 is a more general solution

emilengler commented at 3:09 PM on August 4, 2019: contributor

@marpme I'm skeptical about this. Maybe some people are relying on a remote stop. I think your suggestion could be added later if the PR gets into the code

rpc: Fix malformed code formation 6f50c7834c

rpc: Better if else statement 58e825919e

in src/init.cpp:754 in 6f50c7834c outdated

 749 | @@ -749,7 +750,8 @@ static bool AppInitServers()
 750 |      RPCServer::OnStopped(&OnRPCStopped);
 751 |      if (!InitHTTPServer())
 752 |          return false;
 753 | -    StartRPC();
 754 | +    if (gArgs.GetBoolArg("-rpcallowstop", true)) StartRPC();
 755 | +    else StartRPC(false);

hebasto commented at 4:09 PM on August 4, 2019:

Just StartRPC(gArgs.GetBoolArg("-rpcallowstop", true)); ?

emilengler commented at 4:26 PM on August 4, 2019:

Yes, I was orienting myself at the code below

emilengler commented at 4:32 PM on August 4, 2019:

Fixed

luke-jr commented at 6:05 PM on August 4, 2019: member

Yeah, concept NACK on something this specific.

laanwj commented at 4:38 AM on August 5, 2019: member

Tend towards concept NACK, seems awfully specific. I'd prefer like to avoid wild growth of features such as this.

Bitcoin core RPC is not safe for use remotely in any case, definitely not with untrusted users. Blocking one "attack vector" leaves tons of DoS and potential other vulnerabilities.

I guess the use case for this is to have some kind of public RPC query service? If so, the common suggestion is to use a proxy that filters what commands are allowed to be used and passes only those on. (alternatively, doesn't REST provide the needed queries? this could avoid messing with authentication completely)

emilengler commented at 5:09 PM on August 5, 2019: contributor

Ok I will close this now because of all the arguments above especially that it is too specific, feel free to steal if you need this in future :)

emilengler closed this on Aug 5, 2019

emilengler deleted the branch on Sep 1, 2019

DrahtBot locked this on Dec 16, 2021