api.log contains passwords in plaintext
Closed, ResolvedPublic

Description

Those should probably be edited out.

Tgr created this task.Jan 29 2016, 1:02 AM
Tgr updated the task description. (Show Details)
Tgr raised the priority of this task from to Needs Triage.
Tgr added projects: MediaWiki-API, Security.
Tgr changed the visibility from "Public (No Login Required)" to "Custom Policy".
Tgr changed the edit policy from "All Users" to "Custom Policy".
Tgr changed Security from None to Software security bug.
Tgr added subscribers: Tgr, Anomie.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 29 2016, 1:02 AM
Tgr added a comment.Jan 29 2016, 1:06 AM

Not really a security bug, I guess, since api.log access means shell access which would already allow people to intercept passwords, but should still be redacted.

Not really a security bug, I guess, since api.log access means shell access which would already allow people to intercept passwords, but should still be redacted.

eh, I'd still say security bug, while yes you could intercept them with shell access it's more difficult and in general the system is designed to try and make it as close to impossible as possible to figure out a users password. It is both relatively difficult (even most people with shell access likely don't know how to do the interception, maybe not on WMF servers but almost certainly for reusers who have a wider range of tech skills) to intercept and interception requires 'real time' (basically) compared to log entries which, more similar to the database where we hash them with a salt, can be looked at retroactively "at their leisure" and so makes it easier to target specific users and hide your access.

Not really a security bug, I guess, since api.log access means shell access which would already allow people to intercept passwords, but should still be redacted.

You mean, deployers could still change the code to log the passwords anyway? Yes, but restricted and mw-log-readers can read logs without being able to do that.

csteipp triaged this task as High priority.Jan 29 2016, 1:31 AM
csteipp added a subscriber: csteipp.

Definitely security bug, and I'd like us to get this fixed quickly, if possible. @Tgr, is this something you or @Anomie can get a patch for?

Tgr added a comment.EditedJan 29 2016, 2:04 AM

covers passwords for the login and accountcreation API. Do we have anything else that could be sensitive?

only handles action=login. For action=createaccount, the parameter is "password" with no prefix.

Once AuthManager comes out, passwords are likely to also be in a parameter named "retype". But the AuthenticationRequest might use any parameter name it wants, if some extension decides to do something particularly weird with it.

Tgr added a comment.EditedJan 29 2016, 5:32 AM

It looks for password and lgpassword; I "amended" it by editing the comment (probably a bad habit) and Phabricator does not send any email about that.

For AuthManager we should just redact fields with type=password.

The hard part there is in (1) knowing that the request is for an AuthManager-using module in the first place and then (2) actually determining the AuthenticationRequests that were involved. And asking the module itself could be problematic since things might have errored out before even getting to that point.

Anomie moved this task from Unsorted to Needs Review on the MediaWiki-API board.Jan 29 2016, 2:27 PM

Is there an easy way to filter edit tokens here too? That's less important, but would be nice.

For centralauth tokens and OATH tokens, they expire when used, so having those logged isn't a problem.

@Anomie, for other AuthManger providers, I guess letting them append to some list of blacklisted api parameter names would be nice. I don't think we need to necessarily know that it's an authentication request in order to redact the log.

Tgr added a comment.Jun 10 2016, 5:22 PM

Maybe add a sensitive key to the authrequest field definition and then clients of AuthManager can decide what to do about it?

Maybe add a sensitive key to the authrequest field definition and then clients of AuthManager can decide what to do about it?

That's probably the thing to do.

I just now decided to do that for a different purpose (I saw T143285 and thought that that should be an error, eventually), and then I was reminded of this bug.

This patch applies on top of https://gerrit.wikimedia.org/r/305545

Tgr added a comment.Aug 18 2016, 10:26 PM

CR+2

This will be deployed on August 29th.

This patch applies on top of https://gerrit.wikimedia.org/r/305545

@Anomie, this needs a rebase. Can you do it?

dpatrick closed this task as Resolved.EditedAug 29 2016, 10:38 PM
dpatrick claimed this task.

22:39 dapatrick: Deployed patch for T125177 to 1.28.0-wmf.16

Rebased patch was

.

Anomie added a comment.Nov 1 2016, 6:14 PM

Rebased for 1.29:

Anomie added a comment.Dec 1 2016, 7:06 PM

Rebased again:

It'd be nice if we could get this released at some point soon.

Reedy added a subscriber: Reedy.Apr 1 2017, 9:32 PM

Hmm. So on my supporting backport of https://gerrit.wikimedia.org/r/#/c/336838/

@Tgr wrote:

Seems pointless (unless it blocks other commits from merging cleanly). All this does is break tools with insecure password practices, it seems pretty useless to do that a few weeks before the end of the support period.

If we don't merge that, we can't really put the fix for this into REL1_23... Anyone got any strong opinions either way?

For most client libraries.... Moving passwords/tokens from GET into POST (if they've not already done so for support of newer MW versions) should be trivial

Reedy changed the visibility from "Custom Policy" to "All Users".Apr 6 2017, 8:56 PM
Reedy changed the edit policy from "Custom Policy" to "All Users".
Reedy changed the visibility from "All Users" to "Public (No Login Required)".

Change 346841 merged by jenkins-bot:
[mediawiki/core@master] SECURITY: API: Don't log "sensitive" parameters

https://gerrit.wikimedia.org/r/346841

Change 346860 merged by jenkins-bot:
[mediawiki/core@REL1_28] SECURITY: API: Don't log "sensitive" parameters

https://gerrit.wikimedia.org/r/346860

Change 346850 merged by jenkins-bot:
[mediawiki/core@REL1_27] SECURITY: API: Don't log "sensitive" parameters

https://gerrit.wikimedia.org/r/346850

Change 347027 had a related patch set uploaded (by Chad; owner: Anomie):
[mediawiki/core@wmf/1.29.0-wmf.19] SECURITY: API: Don't log "sensitive" parameters

https://gerrit.wikimedia.org/r/347027

Change 347027 merged by jenkins-bot:
[mediawiki/core@wmf/1.29.0-wmf.19] SECURITY: API: Don't log "sensitive" parameters

https://gerrit.wikimedia.org/r/347027