RPC: Add username and ip logging for RPC method requests #12778

pull GabrielDav wants to merge 1 commits into bitcoin:master from GabrielDav:12223-rpc-log-username changing 3 files +8 −2

GabrielDav commented at 6:36 PM on March 25, 2018: contributor

Adds username and IP logging (if enabled via -logips command) to RPC method request logging. This closes #12223
Add username and ip logging for RPC method requests 4d74c78c69
randolf approved
fanquake added the label RPC/REST/ZMQ on Mar 25, 2018
conscott commented at 10:23 AM on March 26, 2018: contributor

Test ACK 4d74c78c69123a3d97293f40337e2efa5dadbff0
in src/rpc/server.cpp:372 in 4d74c78c69
```
 366 | @@ -367,7 +367,11 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
 367 |      if (!valMethod.isStr())
 368 |          throw JSONRPCError(RPC_INVALID_REQUEST, "Method must be a string");
 369 |      strMethod = valMethod.get_str();
 370 | -    LogPrint(BCLog::RPC, "ThreadRPCServer method=%s\n", SanitizeString(strMethod));
 371 | +    if (fLogIPs)
 372 | +        LogPrint(BCLog::RPC, "ThreadRPCServer method=%s user=%s peeraddr=%s\n", SanitizeString(strMethod),
 373 | +            this->authUser, this->peerAddr);
```
jonasschnelli commented at 11:48 AM on March 26, 2018:

Haven't checked, but could it be possible that authUsers needs sanitizing?

GabrielDav commented at 8:35 PM on March 26, 2018:

authUser comes from RPCAuthorized: https://github.com/bitcoin/bitcoin/blob/0a018431c447bbf18bdaa6a1037aad6a87c1294a/src/httprpc.cpp#L125 on top of that it cannot come from somewhere else because user needs to be authenticated before rpc method can be executed (I cannot find any other reference to parse method). If username is malicious then it must be validated during the authentication, otherwise this might be much worse than non sanitized string in logs. I have no problem adding Sanitize if you think this is necessary. However, I try to maintain assumption that non-user input string should be already validated.

laanwj commented at 4:28 PM on March 27, 2018:

Right, in any case, this would be an issue only for logging failed attempts at authorization.

And in the oft case you don't trust the sanity of the configured user names then sanitizing (instead of say, escaping) before logging creates an auditing issue. It means multiple usernames can map to the same name in the log.

So it's ok to keep it like this.
sipa commented at 3:26 AM on March 27, 2018: member

utACK 4d74c78c69123a3d97293f40337e2efa5dadbff0. I don't think sanitization is necessary as all user names are already adminstrator-configured.
laanwj commented at 4:31 PM on March 27, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12778/commits/4d74c78c69123a3d97293f40337e2efa5dadbff0
laanwj merged this on Mar 27, 2018
laanwj closed this on Mar 27, 2018
laanwj referenced this in commit b648974cc3 on Mar 27, 2018
jnewbery commented at 7:49 PM on March 27, 2018: member

Does this need release notes? It's a small change, but users may not be expecting usernames/IPs to be logged.
deadalnix referenced this in commit ed4486dbf4 on Jun 10, 2020
PastaPastaPasta referenced this in commit b25feef8ef on Nov 1, 2020
gades referenced this in commit ab07df2287 on Jun 25, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#12223 Log origin IP of RPC request