Passwords generated by User::randomPassword() may be shorter than $wgMinimalPasswordLength
Closed, ResolvedPublic

Description

Some quick testing of the output of User::randomPassword() shows that by default output is generated in the range 0 to 7vvvvvvvvv. With a default configuration all passwords should be 10 characters; instead passwords of 9 or fewer characters are easily generated every few thousand invocations of the method -- in very rare cases, the method might generate a password just one single character long.

Ideally, this method should always return $wgMinimalPasswordLength characters (10 by default) -- in the range 0000000000 to vvvvvvvvvv


patches:

  • master -
  • 1.23 - 1.26 -

CVE: CVE-2015-8626

Maniphest changed the visibility from "Public (No Login Required)" to "Custom Policy".Oct 14 2015, 9:16 PM
Maniphest changed the edit policy from "All Users" to "Custom Policy".
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 14 2015, 9:16 PM
Frankrfarmer updated the task description. (Show Details)
Frankrfarmer changed Security from None to Software security bug.
Frankrfarmer edited subscribers, added: Frankrfarmer; removed: Aklapper.
Anomie added a subscriber: Anomie.EditedOct 14 2015, 9:46 PM

Patches for security bugs should not be posted publicly!

An easier fix is to just pass the padding length into wfBaseConvert, as shown here.

While this won't apply to previous versions of MediaWiki, the same change can easily be applied to User.php:

Sorry, let me upload the patch directly here, and remove it from github:

Of course your version is more than sufficient as well

Frankrfarmer updated the task description. (Show Details)Oct 15 2015, 7:22 PM
csteipp triaged this task as Low priority.Oct 20 2015, 12:26 AM
csteipp added a subscriber: csteipp.

Patch looks good to me. We'll get this deployed.

csteipp closed this task as Resolved.
csteipp claimed this task.

18:53 csteipp: deployed patches for T97897 & T115522

demon added a subscriber: demon.Dec 15 2015, 8:08 PM

While this won't apply to previous versions of MediaWiki, the same change can easily be applied to User.php:

This doesn't apply to 1.26, 1.25 or 1.24 as this was already looking ok in those branches afaict. I think this is a regression only affecting 1.23 and master.

Looks like it applies to 1.24, 1.25, and 1.26 cleanly, and is needed.

While this won't apply to previous versions of MediaWiki, the same change can easily be applied to User.php:

This doesn't apply to 1.26, 1.25 or 1.24

It does when I try it (with git am). I'm currently seeing head for 1.26 is 79902bb6, 1.25 is 1c4d1ee5, and 1.24 is f8d608e6.

as this was already looking ok in those branches afaict. I think this is a regression only affecting 1.23 and master.

All versions since r114233 appear to have been affected.

demon added a comment.Dec 15 2015, 8:46 PM

Bleh this was user error, ignore me

demon added a subscriber: Grunny.Dec 17 2015, 1:19 AM
demon changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 18 2015, 12:40 AM
demon changed the edit policy from "Custom Policy" to "All Users".
demon changed Security from Software security bug to None.

Change 259949 had a related patch set uploaded (by Chad):
[SECURITY] 0-pad to length in random string generation

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

Change 259914 merged by Chad:
[SECURITY] 0-pad to length in random string generation

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

Change 259903 merged by Reedy:
[SECURITY] 0-pad to length in random string generation

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

Change 259938 merged by Chad:
[SECURITY] 0-pad to length in random string generation

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

Change 259927 merged by Reedy:
[SECURITY] 0-pad to length in random string generation

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

Change 259949 merged by jenkins-bot:
[SECURITY] 0-pad to length in random string generation

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

csteipp updated the task description. (Show Details)Dec 24 2015, 12:09 AM
csteipp added a project: Vuln-Authn/Session.
Restricted Application added a subscriber: Luke081515. · View Herald TranscriptDec 24 2015, 12:09 AM