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.

In T125177#1980550, @Tgr wrote:

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.

In T125177#1980550, @Tgr wrote:

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.

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.

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.

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.

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

In T125177#2372392, @Tgr wrote:

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

In T125177#2565227, @Anomie wrote:

0001-SECURITY-API-Don-t-log-sensitive-parameters.patch6 KBDownload

CR+2

This will be deployed on August 29th.

In T125177#2565227, @Anomie wrote:

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

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

Rebased for 1.29:

Rebased again:

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

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

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