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

I don't think that's going to work after gerrit 117635, since it will still attempt to check the password even when it's not valid. But I'm going to try and get that merged first, then I'll work on getting that draft into shape.

Trying to think of a solution, what if we do this:

set_time_limit( $wgPasswordHashTimeout );
if ( !$this->mPassword->equals( $plaintext ) ) {
.......
}
set_time_limit( (int)ini_get( 'max_execution_time' ) - $wgRequestTime / 1000000 );

This sucks a bit since if it exceeds the timeout the script will just terminate, but it does prevent the DoS attack, and we can set it to a value reasonable above the average hashing time. (Also, it is a bit more general protection than simply having a large password limit.)

Wordpress decided to make 4096 the maximum password length to prevent this. I'd be ok with that solution.

https://core.trac.wordpress.org/changeset/30467/branches/4.0

We should fix this now that we have pbkdf2 deployed.

In T64685#944470, @Parent5446 wrote:

0001-SECURITY-Set-maximal-password-length-for-DoS.patch8 KBDownload

Re the @todo added at User.php line 778: We don't want the Status class calling $user->checkPasswordValidity(), that makes no sense. Deprecating it would be fine, IMO.

I don't see that Status needs the Status-related part of this method either, as it's much like $status->getStatusArray( false ) except even more broken since it throws away the message parameters. For that matter, it looks like the semantics of the 'isValidPassword' hook are broken and it currently only works by accident via User::getPasswordValidity(), and every caller of User::getPasswordValidity() is hacking around its brokenness (or is itself broken) and won't work right anymore with your patch.

To fix that mess, we really need to:

• Deprecate the 'isValidPassword' hook entirely. Replace it (if necessary) with a hook that gets passed $status directly. Fortunately, the only user in git is Extension:SecurePasswords which is already broken with MW 1.24+.
• When calling the 'isValidPassword' hook for BC and it returns false, we need to return a good status for $result === true and translate $result === false into some generic error message.
• All the callers of User::getPasswordValidity() (none in extensions in git, thankfully) should be fixed to just use User::checkPasswordValidity().
• Maybe fix User::getPasswordValidity() to just return the first error/warning as an array key + parameters, instead of an array of message keys without any parameters, since that's what all the callers seem to be assuming it does.

Other than that, the code seems ok. Haven't tested.

In T64685#947592, @Anomie wrote:

To fix that mess, we really need to:

• Deprecate the 'isValidPassword' hook entirely. Replace it (if necessary) with a hook that gets passed $status directly. Fortunately, the only user in git is Extension:SecurePasswords which is already broken with MW 1.24+.
• When calling the 'isValidPassword' hook for BC and it returns false, we need to return a good status for $result === true and translate $result === false into some generic error message.
• All the callers of User::getPasswordValidity() (none in extensions in git, thankfully) should be fixed to just use User::checkPasswordValidity().
• Maybe fix User::getPasswordValidity() to just return the first error/warning as an array key + parameters, instead of an array of message keys without any parameters, since that's what all the callers seem to be assuming it does.

Other than that, the code seems ok. Haven't tested.

Yeah I completely agree. I was hesitant to do it because I wanted to keep the patch as thin as possible, but I have no problem going ahead and deprecating that nonsense.

I wouldn't say to do it if it weren't already very broken as it is. As it is, failing this check is going to output an error along the lines of "Passwords cannot be longer than 1 character" (or whatever $wgMinimalPasswordLength is set to).

But we could probably get away with fixing the crap in a public patch, and then implementing only the new security check privately.

The only thing I'm wondering is what the logistics would be. What MediaWiki versions does this vulnerability affect? Does it need to be backported? And if so, can we really backport a deprecation.

This issue was also identified by iSec as iSEC-WMF1214-1, so we should fix this before we make the report public.

This only needs to be backported to when we set pbkdf2 as default, so 1.24 I think. I'm definitely not a fan of deprecating in security patches, if we can avoid it.

In the interest of getting this deployed and released, I'd deploy @Parent5446's patch if it:

• removed the @todo
• updated SpecialUserLogin to call checkPasswordValidity, so it can handle the message for long passwords

Then publicly we deprecate getPasswordValidity, since it has almost no usage that I can find.

@Anomie / @Parent5446, does that sound good to you?

You'll also want to fix the usage of getPasswordValidity in User::setPassword(), for the same reason.

In T64685#974376, @csteipp wrote:

• removed the @todo
• updated SpecialUserLogin to call checkPasswordValidity, so it can handle the message for long passwords

In T64685#974571, @Anomie wrote:

You'll also want to fix the usage of getPasswordValidity in User::setPassword(), for the same reason.

@Anomie, I think I'm missing where this is

In T64685#989326, @csteipp wrote:

In T64685#974571, @Anomie wrote:

You'll also want to fix the usage of getPasswordValidity in User::setPassword(), for the same reason.

@Anomie, I think I'm missing where this is

In T64685#991145, @Anomie wrote:

In T64685#989326, @csteipp wrote:

In T64685#974571, @Anomie wrote:

You'll also want to fix the usage of getPasswordValidity in User::setPassword(), for the same reason.

@Anomie, I think I'm missing where this is

t64685b.patch9 KBDownload

does that one too. It's on line 2315 in current master (which is at 3f4156be), @@ -2310,17 +2323,9 @@ in the patch.

Brad and I talked through it, and Brad +1'ed my version of Tyler's patch, so it's ready to deploy.

I deployed it
20:09 csteipp: deployed patch for T64685

Change 201014 had a related patch set uploaded (by CSteipp):
SECURITY: Set maximal password length for DoS

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

Change 201014 merged by jenkins-bot:
SECURITY: Set maximal password length for DoS

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

Change 201220 had a related patch set uploaded (by CSteipp):
SECURITY: Set maximal password length for DoS

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