Page MenuHomePhabricator

User::matchEditToken should use constant-time string comparison
Closed, ResolvedPublic

Description

User::matchEditToken ends with

if ( $val != $sessionToken ) {
	wfDebug( "User::matchEditToken: broken session data\n" );
}

return hash_equals( $sessionToken, $val );

Any benefits from using constant time comparison are lost if there is a non-constant-time comparison in the same function.

Not sure if this is really a security issue (seems impossible to exploit since you cannot time CSRF requests) but erring on the side of safety.


Patch:

  • 1.23 - included in
  • 1.24 - included in
  • 1.25 - included in
  • 1.26 - included in

type: CWE-208
CVE:

  • CVE-2015-8623 - Use hash_equals for result of User::matchEditToken (fixed in https://gerrit.wikimedia.org/r/#/c/156336, backported in security release)
  • CVE-2015-8624 - Use hash_equals for determining if we debug message should be logged

Event Timeline

Maniphest changed the visibility from "Public (No Login Required)" to "Custom Policy".Nov 21 2015, 8:12 PM
Maniphest changed the edit policy from "All Users" to "Custom Policy".
Tgr updated the task description. (Show Details)
Tgr changed Security from None to Software security bug.
Tgr edited subscribers, added: Tgr; removed: Aklapper.

Patch looks good to me too.

If anyone can get this deployed by tomorrow, we can probably get it into the next security release.

csteipp claimed this task.

23:40 csteipp: deployed patch for T119309

Patch applies cleanly except 1.23, here's a patch for that:

The backport patches for this to MW 1.23 and 1.24 should also use hash_equals in the check in the return of the method as well, i.e. here: https://github.com/wikimedia/mediawiki/blob/REL1_24/includes/User.php#L3939 and https://github.com/wikimedia/mediawiki/blob/REL1_23/includes/User.php#L3814. And they should both also probably have the same done for User::matchEditTokenNoSuffix.

demon changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 18 2015, 12:39 AM
demon changed the edit policy from "Custom Policy" to "All Users".
demon changed Security from Software security bug to None.

Change 259947 had a related patch set uploaded (by Chad):
Use hash_equals in User::matchEditToken

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

Change 259912 merged by Chad:
Use hash_equals in User::matchEditToken

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

Change 259901 merged by Reedy:
Use hash_equals in User::matchEditToken

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

Change 259936 merged by Chad:
Use hash_equals in User::matchEditToken

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

Change 259925 merged by Reedy:
Use hash_equals in User::matchEditToken

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

Change 259947 merged by jenkins-bot:
Use hash_equals in User::matchEditToken

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