Pbkdf2Password does not check if hash_pbkdf2() succeeded
Closed, ResolvedPublic

Description

I don't know how likely this is in practice, though just in case, reporting it as a security issue anyway:

hash_pbkdf2() can return false, for example if the specified algorithm is unknown, which allows a simple misconfiguration, like this:

$wgPasswordConfig['pbkdf2']['algo'] = 'sha-512'; // note the hyphen

to cause any newly generated password hash (e.g. :pbkdf2:sha-512:10000:128:jyK0+sEmJ+CMnpucRLANmg==:) to match any password. The same could also happen when comparing against a password hash generated using an algorithm supported only in a newer version of PHP.

hash_pbkdf2() generates a PHP warning; however, if display_errors is off (as it should be in production), they would just go into a log file that the server administrator may or may not check. The function then returns false; however, base64_encode() treats its false return value as the empty string.

It would be entirely reasonable to add an is_string() check to prevent this. In fact, BcryptPassword already has one. It may also make sense to add sanity checks elsewhere, such as in toString() and LayeredParameterizedPassword, to protect against similar issues.

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 19 2016, 9:35 AM

Good catch. I agree, we should def check that.

Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptFeb 19 2016, 2:16 PM
Restricted Application changed the edit policy from "All Users" to "Custom Policy". · View Herald Transcript
csteipp added a subscriber: csteipp.EditedFeb 22 2016, 8:53 PM

csteipp triaged this task as "Low" priority.Feb 22 2016, 8:54 PM
csteipp added a project: Patch-For-Review.
csteipp claimed this task.Feb 23 2016, 10:42 PM

This looks good to me.

New version using is_string. Also added the test in the legacy hash formats, just in case md5() ever gives back something other than a valid hash.

New version using is_string. Also added the test in the legacy hash formats, just in case md5() ever gives back something other than a valid hash.

These changes look sane to me.

csteipp closed this task as "Resolved".Mar 29 2016, 10:29 PM

Deployed in production

22:21 csteipp: Deployed patch for T127420 to wmf18 and wmf19

demon changed the visibility from "Custom Policy" to "Public (No Login Required)".May 20 2016, 5:24 PM
demon changed the edit policy from "Custom Policy" to "All Users".
demon changed Security from Software security bug to None.
Restricted Application added a subscriber: Malyacko. · View Herald TranscriptMay 20 2016, 5:24 PM