Page MenuHomePhabricator

Timing attack on Token
Closed, ResolvedPublic

Description

Aaron pointed out that when checking the Token cookie, we use a string comparison, which could allow someone to brute-force the correct token faster than brute-forcing the entire key space.

$passwordCorrect = ( strlen( $token ) && $token === $request->getCookie( 'Token' ) );

This should use a constant time comparison (xor and check if the result > 1)


Version: unspecified
Severity: normal

Details

Reference
bz61346

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 2:52 AM
bzimport set Reference to bz61346.
bzimport added a subscriber: Unknown Object (MLST).
csteipp created this task.Feb 13 2014, 11:01 PM

Created attachment 14585
Check xor of tokens

attachment bug61346.patch ignored as obsolete

Looks fine, but that comparison should go in it's own protected method.

Created attachment 14631
Check xor of tokens

Attached:

aaron added a comment.Feb 20 2014, 9:05 PM

Looks mergable

Deployed today, and we'll add it to the next release in a couple of weeks.

For reference, see I2a9e89120f7092015495e638c6fa9f67adc9b84f (Looks like gerrit bot can't edit security bugs)

I guess there are more routines and many more _Extensions_ which should start using the constant-time compareSecrets function.

Chris, next time pls. inform me directly.

Not sure, if the following lines in your patch are correct as they make the function return quickly if the lenghts are unequal -> timing attack made easy

		if ( strlen( $answer ) !== strlen( $test ) ) {

+ $passwordCorrect = false;
+ } else {

(In reply to T. Gries from comment #9)

Not sure, if the following lines in your patch are correct as they make the
function return quickly if the lenghts are unequal -> timing attack made easy

		if ( strlen( $answer ) !== strlen( $test ) ) {

+ $passwordCorrect = false;
+ } else {

We could move this check out of the function, so the function is always constant time. But in this context, MediaWiki user tokens have been 32 characters since 2004, so knowing that the token is 32 characters doesn't give an attacker any extra information.

This was assigned CVE-2014-2243. http://www.openwall.com/lists/oss-security/2014/03/01/2

I'm closing this bug since the token comparison in MediaWiki core is fixed. For this vulnerability in other extensions, let's open a separate bug for each, so we can track those separately.

let's open a separate bug for each, so we can track those separately.

Yes! Thx. Don't forget to mail me directly in case you detect further issues.