Page MenuHomePhabricator

audit password policy check for constant time string comparisons
Open, Needs TriagePublic

Description

per title. in particular:

$policyVal && $contLang->lc( $password ) === $contLang->lc( $username )

Should use hash_equals() instead of === in the file /includes/password/PasswordPolicyChecks.php.

Details

Event Timeline

Bawolff created this task.Oct 23 2018, 6:52 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 23 2018, 6:52 PM
Aklapper renamed this task from audit password policy check for constant time string comparisions to audit password policy check for constant time string comparisons.Oct 23 2018, 7:29 PM

Which code repository(es) does this task refer to?

@Aklapper - mw core: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/refs/heads/master/includes/password/PasswordPolicyChecks.php

@Bawolff - Could just do a hash_equals here like you did in T207778, though $username would typically be fairly public, no? Not sure what an attacker might gain from a timing attack in this instance, as compared to the example in T207778.

@Aklapper - mw core: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/refs/heads/master/includes/password/PasswordPolicyChecks.php
@Bawolff - Could just do a hash_equals here like you did in T207778, though $username would typically be fairly public, no? Not sure what an attacker might gain from a timing attack in this instance, as compared to the example in T207778.

Yeah, the username is public. I think the password should be considered sensitive at this point, and anything doing comparisions on it should use hash_equals() instead of ===. I think its a very low priority issue, just something to do when stuff calms down.

If we're just talking about three instances here (lines 89, 112, 117) in PasswordPolicyChecks.php (all I see atm), could this be a Google-Code-in-2018 task? I feel like, given the severity, this could potentially be done publicly in gerrit.

Sounds good to me. Even as far as timing attacks this is extremely minor (read impossible) as its only comparing the entered password not actual. But i think its good to use hash_equals for any comparison involving a password just in case

Bawolff changed the visibility from "Custom Policy" to "Public (No Login Required)".Nov 19 2018, 9:29 PM

Agreed. I'm also happy to mentor anyone for this task, as far as working on mw code, CR in gerrit, etc.

Aklapper updated the task description. (Show Details)Nov 25 2018, 11:35 PM

Change 477007 had a related patch set uploaded (by Mogmog123; owner: Mogmog123):
[mediawiki/core@master] Changing "===" on secrets to hash_equals to protect from timing attacks.

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

Change 477007 merged by jenkins-bot:
[mediawiki/core@master] Changing "===" on secrets to hash_equals to protect from timing attacks.

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