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
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".
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 21 2015, 8:12 PM
Tgr created this task.Nov 21 2015, 8:12 PM
Tgr updated the task description. (Show Details)
Tgr changed Security from None to Software security bug.
Tgr edited subscribers, added: Tgr; removed: Aklapper.
Tgr updated the task description. (Show Details)Nov 21 2015, 8:24 PM

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 triaged this task as Low priority.Nov 24 2015, 9:35 PM
csteipp closed this task as Resolved.Nov 24 2015, 11:41 PM
csteipp claimed this task.

23:40 csteipp: deployed patch for T119309

demon added a subscriber: demon.Dec 15 2015, 7:55 PM

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

demon added a subscriber: Grunny.Dec 17 2015, 1:18 AM

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

csteipp updated the task description. (Show Details)Dec 23 2015, 11:52 PM
Restricted Application added a subscriber: Luke081515. · View Herald TranscriptDec 23 2015, 11:52 PM