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

Conversation

Projects
None yet
8 participants
@GabrielDav
Copy link
Contributor

GabrielDav commented Mar 25, 2018

Adds username and IP logging (if enabled via -logips command) to RPC method request logging.
This closes #12223

@conscott
Copy link
Contributor

conscott left a comment

Test ACK 4d74c78

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.

@jonasschnelli

jonasschnelli Mar 26, 2018

Member

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

This comment has been minimized.

@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.

@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.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Mar 27, 2018

utACK 4d74c78. I don't think sanitization is necessary as all user names are already adminstrator-configured.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 27, 2018

utACK 4d74c78

@laanwj laanwj merged commit 4d74c78 into bitcoin:master Mar 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 27, 2018

Merge #12778: RPC: Add username and ip logging for RPC method requests
4d74c78 Add username and ip logging for RPC method requests (Gabriel Davidian)

Pull request description:

  Adds username and IP logging (if enabled via -logips command) to RPC method request logging.
  This closes #12223

Tree-SHA512: a441228e80ea6884ec379c66e949d86df3689770f1b3c3608015cf5a36d2dfb38051298a7f6ea6dfdfbf0b3b6c896e414c8dc54e9833bb73dd65bdb1832f4395
@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 27, 2018

Does this need release notes? It's a small change, but users may not be expecting usernames/IPs to be logged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.