Extremely large passwords as DoS
Closed, ResolvedPublic

Description

Noticed this old post about django, http://threatpost.com/patches-for-django-framework-fix-dos-vulnerability/102323, and we should probably account for it before we have pbkdf2 out there. With the old md5 hashing, this poc took just a few hundred ms. With pbdkf2, I hit max execution time.

perl -e "print 'lgpassword=' . 'A'x1024x1024" > pw.txt
curl -b './pw.cookie' -c './pw.cookie' -d action=login -d lgname=NormalUser -d lgtoken=df405afa01896b61b40229c86baef771 --data @pw.txt 'http://localhost/wiki/api.php?format=json'

It's a quick fix to limit the password input to 4096 (or even 10,000 if we wanted). Just something less the max post size.


Version: unspecified
Severity: normal

Patch:

  • 1.24:

Affected Versions: Since 95a8974c6bda2c6353612c40b01b9c78527b8956 (1.24)
Type: DoS
CVE: CVE-2015-2936

Details

Reference
bz62685
bzimport set Reference to bz62685.
bzimport changed Security from none to Software security bug.
Restricted Application changed the visibility from "Public (No Login Required)" to "Security (Project)". · View Herald TranscriptNov 22 2014, 2:53 AM
Restricted Application changed the edit policy from "All Users" to "Security (Project)". · View Herald Transcript
csteipp created this task.Mar 15 2014, 1:25 AM

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.

Restricted Application changed the visibility from "Security (Project)" to "Custom Policy". · View Herald TranscriptNov 24 2014, 9:27 PM
Restricted Application changed the edit policy from "Security (Project)" to "Custom Policy". · View Herald Transcript

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.)

Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptNov 25 2014, 7:45 AM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript

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.

Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptDec 15 2014, 9:06 PM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript

Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptDec 26 2014, 4:55 PM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript
Anomie added a subscriber: Anomie.Dec 29 2014, 8:05 PM

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.

Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptDec 29 2014, 8:05 PM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript

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.

Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptDec 29 2014, 9:08 PM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript
Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptDec 29 2014, 9:09 PM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript

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.

Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptDec 29 2014, 9:20 PM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript

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.

Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptDec 29 2014, 9:26 PM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript

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.

Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptJan 5 2015, 8:43 PM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript

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?

Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptJan 13 2015, 7:54 PM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript

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

Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptJan 13 2015, 9:18 PM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript
  • removed the @todo
  • updated SpecialUserLogin to call checkPasswordValidity, so it can handle the message for long passwords

did these.

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

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

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

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

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

csteipp closed this task as "Resolved".Jan 23 2015, 8:10 PM
csteipp edited the task description. (Show Details)Mar 18 2015, 12:39 AM
csteipp edited the task description. (Show Details)Mar 24 2015, 10:11 PM
csteipp changed the visibility from "Custom Policy" to "Custom Policy".Mar 31 2015, 12:38 PM
csteipp changed the visibility from "Custom Policy" to "Public (No Login Required)".Mar 31 2015, 9:16 PM
csteipp changed the edit policy from "Custom Policy" to "All Users".
csteipp changed Security from Software security bug to None.

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

csteipp edited the task description. (Show Details)Apr 9 2015, 10:53 PM