doc: Add release note for RPC Whitelist #17743

emilengler commented at 1:26 PM on December 13, 2019: contributor

A release note for #12763

fanquake added the label Docs on Dec 13, 2019

emilengler force-pushed on Dec 13, 2019

instagibbs commented at 3:02 PM on December 13, 2019: member

it should probably at least mention one or all of the config options, otherwise users will have to just grep and guess

emilengler commented at 4:50 PM on December 13, 2019: contributor

@instagibbs Updated, however it isn't updated on the PR page for some weird reason. If you look into my branch you will see the update o.O

emilengler commented at 4:52 PM on December 13, 2019: contributor

See https://www.githubstatus.com/, they have issues at the moment

emilengler force-pushed on Dec 13, 2019

in doc/release-notes.md:82 in cb664087c2 outdated

  77 | @@ -78,6 +78,9 @@ New RPCs
  78 |  New settings
  79 |  ------------
  80 |  
  81 | +- RPC Whitelist system. It can give certain RPC users permissions to only some RPC calls.
  82 | +It can be accessed with two command line arguments (`rpcwhitelist` and `rpcwhitelistdefault`)

MarcoFalke commented at 5:07 PM on December 13, 2019:

It can be accessed with two command line arguments (`rpcwhitelist` and `rpcwhitelistdefault`). (#12763)

instagibbs commented at 7:03 PM on December 13, 2019:

s/accessed/set/ ?

emilengler commented at 8:41 PM on December 13, 2019:

ACK

JeremyRubin commented at 6:09 PM on December 13, 2019: contributor

You could just copy the help-text if you want it to be more thorough.

    gArgs.AddArg("-rpcwhitelist=<whitelist>", "Set a whitelist to filter incoming RPC calls for a specific user. The field <whitelist> comes in the format: <USERNAME>:<rpc 1>,<rpc 2>,...,<rpc n>. If multiple whitelists are set for a given user, they are set-intersected. See -rpcwhitelistdefault documentation for information on default whitelist behavior.", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
    gArgs.AddArg("-rpcwhitelistdefault", "Sets default behavior for rpc whitelisting. Unless rpcwhitelistdefault is set to 0, if any -rpcwhitelist is set, the rpc server acts as if all rpc users are subject to empty-unless-otherwise-specified whitelists. If rpcwhitelistdefault is set to 1 and no -rpcwhitelist is set, rpc server acts as if all rpc users are subject to empty whitelists.", ArgsManager::ALLOW_BOOL, OptionsCategory::RPC);

MarcoFalke commented at 7:02 PM on December 13, 2019: member

The release notes are meant as a brief pointer, not to duplicate the rpc and/or command line help. I think the current patch is ok

emilengler force-pushed on Dec 13, 2019

instagibbs commented at 8:44 PM on December 13, 2019: member

thanks for the cleanups

emilengler force-pushed on Dec 13, 2019

emilengler commented at 8:46 PM on December 13, 2019: contributor

@instagibbs Sorry but could you re-ACK? The commit hash just changed as I included the PR reference :P

instagibbs commented at 8:47 PM on December 13, 2019: member

argh, needed to refresh one more time :)

ACK https://github.com/bitcoin/bitcoin/pull/17743/commits/3ae22b0c8fb9a59ab95cbf6c970166b01274f846

promag commented at 3:53 PM on December 14, 2019: member

ACK 3ae22b0c8fb9a59ab95cbf6c970166b01274f846.

practicalswift commented at 5:34 PM on December 14, 2019: contributor

Oh, before merging this I think we should either bump UniValue or add a temporary warning about this RPC DoS gotcha (https://github.com/bitcoin/bitcoin/issues/17742#issuecomment-565736432):

$ share/rpcauth/rpcauth.py u p
String to be appended to bitcoin.conf:
rpcauth=u:d7ca300b5aeceb73e9825068fc4496ef$166cbdedfe11c30a95c28d13b3383682825308d1069d347f9705b8cf922637c5
Your password:
p
$ src/bitcoind -regtest -rpcwhitelist=u:help '-rpcauth=u:d7ca300b5aeceb73e9825068fc4496ef$166cbdedfe11c30a95c28d13b3383682825308d1069d347f9705b8cf922637c5' &
$ src/bitcoin-cli -regtest -rpcuser=u -rpcpassword=p help | head -1
== Blockchain ==
$ src/bitcoin-cli -regtest -rpcuser=u -rpcpassword=p getnetworkinfo
2019-12-13T02:01:37Z RPC User u not allowed to call method getnetworkinfo
error: server returned HTTP error 403
$ curl -u u:p --request POST \
     --data $(python -c 'print(50000 * "[");') http://127.0.0.1:18443
curl: (52) Empty reply from server
[1]+  Segmentation fault      (core dumped) src/bitcoind -regtest -rpcwhitelist=u:help '-rpcauth=u:d7ca300b5aeceb73e9825068fc4496ef$166cbdedfe11c30a95c28d13b3383682825308d1069d347f9705b8cf922637c5'

Temporary NACK until this is addressed. Sorry about that, but end-user security is first, second and third priority :)

JeremyRubin commented at 6:05 PM on December 14, 2019: contributor

This is just release notes so I don't think a nack here makes sense, but I would get the univalve bump added to high priority.

While this is certainly a bad bug, if it's limited to segfaulting the node it's at least "Fail-Safe" insofar as stopping a node is generally safe and doesn't lead to an un-permitted rpc being called.

Now, a segfault may be able to be pivoted to something more extreme but I've not looked at the particulars of the univalue issue.

On Sat, Dec 14, 2019, 9:34 AM practicalswift notifications@github.com wrote:

Oh, before merging this I think we should either bump UniValue or add a temporary warning about this RPC DoS gotcha: #17742 (comment) https://github.com/bitcoin/bitcoin/issues/17742#issuecomment-565736432.

$ share/rpcauth/rpcauth.py u p String to be appended to bitcoin.conf: rpcauth=u:d7ca300b5aeceb73e9825068fc4496ef$166cbdedfe11c30a95c28d13b3383682825308d1069d347f9705b8cf922637c5 Your password: p $ src/bitcoind -regtest -rpcwhitelist=u:help '-rpcauth=u:d7ca300b5aeceb73e9825068fc4496ef$166cbdedfe11c30a95c28d13b3383682825308d1069d347f9705b8cf922637c5' & $ src/bitcoin-cli -regtest -rpcuser=u -rpcpassword=p help | head -1 == Blockchain == $ src/bitcoin-cli -regtest -rpcuser=u -rpcpassword=p getnetworkinfo 2019-12-13T02:01:37Z RPC User u not allowed to call method getnetworkinfo error: server returned HTTP error 403 $ curl -u u:p --request POST
--data $(python -c 'print(50000 * "[");') http://127.0.0.1:18443 curl: (52) Empty reply from server [1]+ Segmentation fault (core dumped) src/bitcoind -regtest -rpcwhitelist=u:help '-rpcauth=u:d7ca300b5aeceb73e9825068fc4496ef$166cbdedfe11c30a95c28d13b3383682825308d1069d347f9705b8cf922637c5'

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/17743?email_source=notifications&email_token=AAGYN65EZC5DA4FXM2KDFMLQYUKLXA5CNFSM4J2M7TC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG4HMDI#issuecomment-565736973, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGYN647K22T3GN3HWASFBLQYUKLXANCNFSM4J2M7TCQ .

practicalswift commented at 6:11 PM on December 14, 2019: contributor

@JeremyRubin I'm just NACK:ing the change as-is (per 3ae22b0c8fb9a59ab95cbf6c970166b01274f846): if @emilengler adds a (temporary) warning about this gotcha that would be perfectly fine and I'll ACK immediately :)

We should do what we can to help our users stay safe: not adding a warning about a known gotcha when we have the opportunity would feel bad :)

in doc/release-notes.md:81 in 3ae22b0c8f outdated

  77 | @@ -78,6 +78,9 @@ New RPCs
  78 |  New settings
  79 |  ------------
  80 |  
  81 | +- RPC Whitelist system. It can give certain RPC users permissions to only some RPC calls.

jonatack commented at 6:15 PM on December 14, 2019:

suggest: An RPC whitelisting feature has been added to prevent unintended access on a per-user, per-call basis to help enforce application policies for services built on Bitcoin Core.

emilengler commented at 10:42 PM on December 14, 2019:

I think your formulation talks around the feature a bit. I think my formulation brings it more on the point as I think that the term "permission" should be included.

jonatack commented at 6:16 PM on December 14, 2019: member

Suggestion below.

emilengler force-pushed on Dec 14, 2019

emilengler commented at 10:48 PM on December 14, 2019: contributor

@practicalswift Adjusted, could you check it?

in doc/release-notes.md:83 in f1cf747596 outdated

  77 | @@ -78,6 +78,10 @@ New RPCs
  78 |  New settings
  79 |  ------------
  80 |  
  81 | +- RPC Whitelist system. It can give certain RPC users permissions to only some RPC calls.
  82 | +It can be set with two command line arguments (`rpcwhitelist` and `rpcwhitelistdefault`). (#12763)
  83 | +Keep in mind that due to UniValue CVE-2019-18936 segfaults could occur if the user gives too much input.

laanwj commented at 10:32 AM on December 15, 2019:

These are 0.20.0 release notes. Fixing this is a matter of pulling in a subtree, and it should definitely be fixed before the release! (see #17742)

If there's need to mention this in the release notes like this, shipping with a known issue, we've failed very much.

emilengler commented at 5:14 PM on December 15, 2019:

@laanwj I agree, will revert

emilengler commented at 7:50 PM on December 15, 2019:

Done

doc: Add release note for RPC Whitelist 7965e0b41a

emilengler force-pushed on Dec 15, 2019

laanwj commented at 10:01 AM on December 16, 2019: member

ACK 7965e0b41ae03110ad784a1b374f831600c0cca1

laanwj referenced this in commit c3628e4483 on Dec 16, 2019

laanwj merged this on Dec 16, 2019

laanwj closed this on Dec 16, 2019

MarcoFalke locked this on Dec 16, 2021