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: contributor

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

MarkLTZ cross-referenced this on Apr 4, 2020 from issue Bitcoin PR tracking by MarkLTZ

bitcoin locked this on Dec 16, 2021