Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Mar 27, 2018
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+8 −2
Diff settings

Always

Just for now

Copy path View file
@@ -158,8 +158,9 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
}

JSONRPCRequest jreq;
jreq.peerAddr = req->GetPeer().ToString();
if (!RPCAuthorized(authHeader.second, jreq.authUser)) {
LogPrintf("ThreadRPCServer incorrect password attempt from %s\n", req->GetPeer().ToString());
LogPrintf("ThreadRPCServer incorrect password attempt from %s\n", jreq.peerAddr);

/* Deter brute-forcing
If this results in a DoS the user really
Copy path View file
@@ -367,7 +367,11 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
if (!valMethod.isStr())
throw JSONRPCError(RPC_INVALID_REQUEST, "Method must be a string");
strMethod = valMethod.get_str();
LogPrint(BCLog::RPC, "ThreadRPCServer method=%s\n", SanitizeString(strMethod));
if (fLogIPs)
LogPrint(BCLog::RPC, "ThreadRPCServer method=%s user=%s peeraddr=%s\n", SanitizeString(strMethod),
this->authUser, this->peerAddr);

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Mar 26, 2018

Member

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

This comment has been minimized.

Copy link
@GabrielDav

GabrielDav Mar 26, 2018

Author Contributor

authUser comes from RPCAuthorized:

static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUsernameOut)

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.

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 27, 2018

Member

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.

else
LogPrint(BCLog::RPC, "ThreadRPCServer method=%s user=%s\n", SanitizeString(strMethod), this->authUser);

// Parse params
UniValue valParams = find_value(request, "params");
Copy path View file
@@ -45,6 +45,7 @@ class JSONRPCRequest
bool fHelp;
std::string URI;
std::string authUser;
std::string peerAddr;

JSONRPCRequest() : id(NullUniValue), params(NullUniValue), fHelp(false) {}
void parse(const UniValue& valRequest);
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.