If you throw an exception inside of User::setPassword(), you'll end up with an entry like this in your logs:
#0 /path/1.18wmf1/includes/specials/SpecialUserlogin.php(413): User->setPassword('passwordzomg')
This is pretty bad, since we don't want to leak a user's password to system administrators.
Version: 1.22.0
Severity: normal
This sort of thing is why we have display of exception details to output off by default. :P
This is alas a fairly general problem with PHP's backtrace generation -- it adds in parameters values which are sometimes helpful, often hugely verbose, and sometimes insanely insecure.
Not sure if we can remove the args from the trace as such, though we probably can by changing manual exception handling to do our own logging also, if we haven't already...
(In reply to comment #1)
This sort of thing is why we have display of exception details to output off by
default. :P
Right, so this doesn't really affect most non-development installs. But still, gives me the willies!
This is alas a fairly general problem with PHP's backtrace generation -- it
adds in parameters values which are sometimes helpful, often hugely verbose,
and sometimes insanely insecure.Not sure if we can remove the args from the trace as such, though we probably
can by changing manual exception handling to do our own logging also, if we
haven't already...
Right, I knew it was PHP's problem, but I wasn't sure if we could do anything here. I figured we could keep a list of functions to keep the parameters blacklisted from, but I'm afraid it would be a game of whack-a-mole.
As a quick thought on this: we could potentially mask *all* params in the stack traces (rather than playing whack-a-mole with potentially unsafe entries), and hide them behind another setting ($wgShowStacktraceDetails) with a very clear comment that it should almost never be enabled on a production site?
+1 to Chad. As an aside, it looks like vbulletin's backtrace stuff
somehow overrides installation path information in non-debug output
with [path]. So obfuscation is possible, not sure *how* just yet, or
how robust it is.
I don't understand why this is a security issue. What sort of system administrator would have access to the logs but not to any of the other ways to get a user's password?
I was probably just being paranoid. We can move this out of security and into the general bug pool.
(In reply to comment #6)
I was probably just being paranoid. We can move this out of security and into
the general bug pool.
Moving
Related URL: https://gerrit.wikimedia.org/r/64450 (Gerrit Change I0a9e92448f8d9009dd594cb4d7f5dc6fdd4bcc86)
Related URL: https://gerrit.wikimedia.org/r/64590 (Gerrit Change I76c3bc41cda004f00d12d46015455cd9ea79578b)
Change 64590 merged by jenkins-bot:
Redact certain function parameters from exception stack traces
Change 64450 merged by jenkins-bot:
Add a way to redact certain function parameters from exception stack traces
Not quite it seems:
#1 /var/www/MediaWiki/Git/core/includes/specials/SpecialChangePassword.php(84): SpecialChangePassword->attemptReset('REDACTED', 'My password was here')
Just need to modify the line in DefaultSettings.php to include that second parameter.
Change 85437 had a related patch set uploaded by Alex Monk:
Redact second parameter of SpecialChangePassword::attemptReset by default in stack traces
Change 92472 had a related patch set uploaded by Chad:
Revert "Add a way to redact certain function parameters from exception stack traces"
Change 92472 abandoned by Hashar:
Revert "Add a way to redact certain function parameters from exception stack traces"
Reason:
Follow up in https://gerrit.wikimedia.org/r/#/c/92334/
Change 85437 abandoned by Alex Monk:
Redact second parameter of SpecialChangePassword::attemptReset by default in stack traces
Reason:
It was decided that this system probably isn't a good idea