Last Comment Bug 906745 - In MySQL, tokens are not case-sensitive, reducing total entropy and allowing easier brute force
: In MySQL, tokens are not case-sensitive, reducing total entropy and allowing ...
Status: RESOLVED FIXED
[site:bugzilla.mozilla.org][reporter-...
: sec-low, wsec-authentication, wsec-crypto
Product: Bugzilla
Classification: Server Software
Component: Bugzilla-General (show other bugs)
: 4.2.6
: All All
: -- normal (vote)
: Bugzilla 4.0
Assigned To: David Lawrence [:dkl]
: default-qa
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-19 09:29 PDT by Anand Sundar Tiwari
Modified: 2014-07-16 12:04 PDT (History)
8 users (show)
glob: approval+
glob: approval4.4+
LpSolit: blocking4.4.1+
glob: approval4.2+
LpSolit: blocking4.2.7+
glob: approval4.0+
LpSolit: blocking4.0.11+
dveditz: sec‑bounty-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
906745_1.patch (2.00 KB, patch)
2013-08-19 14:38 PDT, David Lawrence [:dkl]
glob: review-
Details | Diff | Splinter Review
906745_2.patch (1.86 KB, patch)
2013-08-20 11:38 PDT, David Lawrence [:dkl]
LpSolit: review-
Details | Diff | Splinter Review
patch for 4.4 and trunk (2.41 KB, patch)
2013-08-20 13:16 PDT, David Lawrence [:dkl]
LpSolit: review+
Details | Diff | Splinter Review
patch for 4.0 and 4.2 (3.23 KB, patch)
2013-08-20 15:12 PDT, David Lawrence [:dkl]
LpSolit: review+
Details | Diff | Splinter Review

Description Anand Sundar Tiwari 2013-08-19 09:29:57 PDT
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.95 Safari/537.36

Steps to reproduce:

I found Logical bug in you site bugzilla.mozilla.org.
in this site when user request for password resetting token and they receive token i.e 
https://bugzilla.mozilla.org/token.cgi?t=(token1)6n&a=(token2)

Value1 is look like t= DLYtLvqT6n
Value2 is look like a= cfmpw

Value1 is the main token for password reset.
and Value2 is used for all account.

so, Value1 (t= DLYtLvqT6n) is look like case sensitive but when you use this value1 in small letter (dlytlvqt6n) then its work properly and not verified by server side for case sensitive.

original token : https://bugzilla.mozilla.org/token.cgi?t=DLYtLvqT6n&a=cfmpw
after change : https://bugzilla.mozilla.org/token.cgi?t=dlytlvqt6n&a=cfmpw

So this token is only 10 alphanumeric value and look like case sensitive.

But actual value1 is not case sensitive as capital letters not validated to check weather its capital or small even if we convert all string into small letter.

So please check it out this bug.


Actual results:

Attacker able to guess value1 which is used for password reset token.


Expected results:

User account compromised by attacker if password  reset token is not properly validated and case sensitive.
Comment 1 Reed Loden [:reed] (use needinfo?) 2013-08-19 09:40:18 PDT
I can confirm this, at least on bugzilla.mozilla.org. Will test normal Bugzilla next.
Comment 2 Reed Loden [:reed] (use needinfo?) 2013-08-19 10:01:14 PDT
This is due to MySQL's case insensitivity to search when using latin1 or latin1_general_cs.

https://dev.mysql.com/doc/refman/5.0/en/case-sensitivity.html
Comment 3 Reed Loden [:reed] (use needinfo?) 2013-08-19 10:08:32 PDT
Except the tokens table is utf8 on prod bmo. Ugh. Back to checking... but it looks to me like the GetTokenData() code just does this:

    return $dbh->selectrow_array(
        "SELECT userid, " . $dbh->sql_date_format('issuedate') . ", eventdata, tokentype
         FROM   tokens
         WHERE  token = ?", undef, $token);
Comment 4 Reed Loden [:reed] (use needinfo?) 2013-08-19 10:17:16 PDT
Ah, but according to Sheeri, we are using the utf8_general_ci collation, which is case-insensitive. So, that's why this works. We need to explicitly either use the utf8_general_cs collation when doing case-sensitive queries or do the checking in some other fashion.
Comment 5 Reed Loden [:reed] (use needinfo?) 2013-08-19 10:23:26 PDT
or possibly utf8_bin collation
Comment 6 Sheeri Cabral [:sheeri] 2013-08-19 10:34:22 PDT
reed - I'd go with utf8_bin.
Comment 7 David Lawrence [:dkl] 2013-08-19 14:38:04 PDT
Created attachment 792435 [details] [diff] [review]
906745_1.patch

This patch fixes the issue for MySQL. I will need to make sure nothing breaks with Oracle as well as PostgreSQL. I can test with Oracle if someone can verify on PostgreSQL.

dkl
Comment 8 Reed Loden [:reed] (use needinfo?) 2013-08-19 15:01:42 PDT
Comment on attachment 792435 [details] [diff] [review]
906745_1.patch

Review of attachment 792435 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/DB/Mysql.pm
@@ +185,5 @@
>  
> +sub sql_csstrcmp {
> +    my ($self, $left, $right, $op) = @_;
> +    $op ||= "=";
> +    return "$left COLLATE utf8_bin $op $right COLLATE utf8_bin";

What happens if utf8 support is off?

::: Bugzilla/Token.pm
@@ +329,4 @@
>      return $dbh->selectrow_array(
>          "SELECT userid, " . $dbh->sql_date_format('issuedate') . ", eventdata, tokentype
>           FROM   tokens 
> +         WHERE  " . $dbh->sql_csstrcmp("token", "?", "="), undef, $token);

Are we sure this the only place where we need to do this? Seems like we should be using the case-sensitive match by default and only use case-insensitive when we specific need it.
Comment 9 David Lawrence [:dkl] 2013-08-19 15:32:19 PDT
(In reply to Reed Loden [:reed] from comment #8)
> Comment on attachment 792435 [details] [diff] [review]
> 906745_1.patch
> 
> Review of attachment 792435 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: Bugzilla/DB/Mysql.pm
> @@ +185,5 @@
> >  
> > +sub sql_csstrcmp {
> > +    my ($self, $left, $right, $op) = @_;
> > +    $op ||= "=";
> > +    return "$left COLLATE utf8_bin $op $right COLLATE utf8_bin";
> 
> What happens if utf8 support is off?

good point. I can create a new patch that checks for $self->bz_db_is_utf8, otherwise use COLLATE latin1_bin.

> ::: Bugzilla/Token.pm
> @@ +329,4 @@
> >      return $dbh->selectrow_array(
> >          "SELECT userid, " . $dbh->sql_date_format('issuedate') . ", eventdata, tokentype
> >           FROM   tokens 
> > +         WHERE  " . $dbh->sql_csstrcmp("token", "?", "="), undef, $token);
> 
> Are we sure this the only place where we need to do this? Seems like we
> should be using the case-sensitive match by default and only use
> case-insensitive when we specific need it.

I would think we would fix this on a case-by-case basis such as any place that is a security risk first. AFAIK we have been case-insensitive by default with MySQL for a long time and switching everything the opposite way would not be trivial.

dkl
Comment 10 Byron Jones ‹:glob› [PTO until 2017-01-09] 2013-08-19 22:13:35 PDT
Comment on attachment 792435 [details] [diff] [review]
906745_1.patch

Review of attachment 792435 [details] [diff] [review]:
-----------------------------------------------------------------

as per reed, you need to support when utf8 is off.  it may be easier to do a BINARY comparison instead.
this will fail on non-mysql databases, as their DB modules won't have sql_csstrcmp defined.
Comment 11 Frédéric Buclin 2013-08-20 02:38:03 PDT
Comment on attachment 792435 [details] [diff] [review]
906745_1.patch

>=== modified file 'Bugzilla/Token.pm'

>     return $dbh->selectrow_array(
>         "SELECT userid, " . $dbh->sql_date_format('issuedate') . ", eventdata, tokentype
>          FROM   tokens 
>-         WHERE  token = ?", undef, $token);
>+         WHERE  " . $dbh->sql_csstrcmp("token", "?", "="), undef, $token);

This change will break _create_token() as it uses GetTokenData() to know if a token already exists, and if not, insert that token into the tokens table. As MySQL is case-insensitive by default, trying to insert a token which only differs by its case from an existing token will make MySQL fail.

Another way to fix this problem is to check the DB token in Perl itself, i.e. something like:

  if ($token eq $db_token)


The 'eq' operator is case-sensitive, and so we don't care if the DB uses UTF8 or not. And this lets callers ignore this test if they want a case-insensitive comparison, such as _create_token().
Comment 12 Frédéric Buclin 2013-08-20 02:44:14 PDT
This problem affects all places where tokens are in use, not only user accounts.
Comment 13 Frédéric Buclin 2013-08-20 03:00:58 PDT
(In reply to Byron Jones ‹:glob› from comment #10)
> this will fail on non-mysql databases, as their DB modules won't have
> sql_csstrcmp defined.

They inherit the methods defined in DB.pm, so that's fine.


This issue is MySQL-specific as PostgreSQL, Oracle and SQLite are all case-sensitive and so are not affected by this problem.
Comment 14 Frédéric Buclin 2013-08-20 03:06:16 PDT
(In reply to Frédéric Buclin from comment #11)
> This change will break _create_token() as it uses GetTokenData() to know if
> a token already exists, and if not, insert that token into the tokens table.

I take that back, _create_token() calls GenerateUniqueToken() which does its own check, and so is not affected by your code. So this is fine, you can make it case-sensitive always.
Comment 15 Byron Jones ‹:glob› [PTO until 2017-01-09] 2013-08-20 07:08:05 PDT
(In reply to Frédéric Buclin from comment #13)
> This issue is MySQL-specific as PostgreSQL, Oracle and SQLite are all
> case-sensitive and so are not affected by this problem.

those databases are case sensitive _by default_.

our sqlite database uses a custom collation which is case-insensitive.
i haven't had time to test the other two databases.
Comment 16 Gervase Markham [:gerv] 2013-08-20 09:35:46 PDT
Number of possible chars, case-sensitive: 26 + 26 + 10 = 62
Token possibilities = 62^10 = 839,299,365,868,340,224
Number of possible chars, case-insensitive: 26 + 10 = 36
Token possibilities = 36^10 =   3,656,158,440,062,976

So this bug makes the tokens approximately 229 times easier to crack, but one would still need to make 1.5 quadrillion requests, on average, to guess it. So unless there's also a flaw in our random number generation, I don't think this rates as a security flaw. Although we are pleased it was reported :-)

Gerv
Comment 17 David Lawrence [:dkl] 2013-08-20 11:38:49 PDT
Created attachment 792975 [details] [diff] [review]
906745_2.patch

This takes LpSolit's idea of just doing the comparison in Perl which negates the need to do anything special per database.
Comment 18 Frédéric Buclin 2013-08-20 11:41:33 PDT
(In reply to Gervase Markham [:gerv] from comment #16)
> So this bug makes the tokens approximately 229 times easier to crack

Yes, good point. I still think it should be fixed to make our system 229 times more secure. :) But this makes it more a security enhancement than a security bug, I agree.
Comment 19 Reed Loden [:reed] (use needinfo?) 2013-08-20 11:48:01 PDT
Comment on attachment 792975 [details] [diff] [review]
906745_2.patch

What happens if there are multiple valid tokens that just have case differences? What will this code do? Please show valid testing of this.
Comment 20 Gervase Markham [:gerv] 2013-08-20 11:52:34 PDT
(In reply to Reed Loden [:reed] from comment #19)
> What happens if there are multiple valid tokens that just have case
> differences? 

I refer the honourable gentleman to the probability figures I gave some moments ago. 

> What will this code do? Please show valid testing of this.

(The above is not to argue against testing of patches, of course.)

Gerv
Comment 21 Frédéric Buclin 2013-08-20 11:54:32 PDT
(In reply to Reed Loden [:reed] from comment #19)
> What happens if there are multiple valid tokens that just have case
> differences?

This cannot happen on DBs which are case-insensitive, so this is not an issue.
Comment 22 Reed Loden [:reed] (use needinfo?) 2013-08-20 11:55:05 PDT
(In reply to Gervase Markham [:gerv] from comment #20)
> (In reply to Reed Loden [:reed] from comment #19)
> > What happens if there are multiple valid tokens that just have case
> > differences? 
> 
> I refer the honourable gentleman to the probability figures I gave some
> moments ago.

No, look at it from the perspective of what happens if a different type of token is returned from the SELECT vs. the type of token the user is actually presenting...
Comment 23 Raymond Forbes[:rforbes] 2013-08-20 12:00:45 PDT
i am with reed on htis one.  I see this as at least a moderate risk.
Comment 24 Frédéric Buclin 2013-08-20 12:05:46 PDT
Comment on attachment 792975 [details] [diff] [review]
906745_2.patch

>+    ($db_token eq $token) || ThrowUserError("token_does_not_exist");

If there is no such token in the DB, then you would get a "Use of uninitialized value" warning. But Bugzilla::Token::Cancel() is only called from token.cgi or Token.pm themselves *after* the token has been validated and we made sure it exists. So you should never reach the ThrowUserError() code here. That's why I would prefer a ThrowCodeError() as it would be a code error rather than a user error. There is no need to make sure that $db_token is defined because this cannot happen unless there is a code error.


>+    my ($db_token, $user_id, $issue_date, $event_data, $token_type) = $dbh->selectrow_array(

It would be easier and cleaner to simply write my @token_data = $dbh->... and then look at $token_data[0] ne $token.


>+    return undef if $db_token ne $token;

You must first make sure that $db_token is defined.
Comment 25 Frédéric Buclin 2013-08-20 12:07:44 PDT
(In reply to Reed Loden [:reed] from comment #22)
> No, look at it from the perspective of what happens if a different type of
> token is returned from the SELECT vs. the type of token the user is actually
> presenting...

This cannot happen, see comment 21.


(In reply to Raymond Forbes[:rforbes] from comment #23)
> i am with reed on htis one.

reed is wrong!
Comment 26 Anand Sundar Tiwari 2013-08-20 12:18:37 PDT
I don't think it was normal bug.
i always like to make more secure mozilla and I am happy that I reported this types of bug.
Please don't take this bug in normal way. (In reply to Raymond Forbes[:rforbes] from comment #23)
> i am with reed on htis one.  I see this as at least a moderate risk.
Comment 27 Reed Loden [:reed] (use needinfo?) 2013-08-20 12:35:06 PDT
(In reply to Frédéric Buclin from comment #25)
> (In reply to Raymond Forbes[:rforbes] from comment #23)
> > i am with reed on htis one.
> 
> reed is wrong!

Fairly sure he's talking about my stance that this should be treated as a security *bug*, not a security *enhancement*. This is still a dramatic reduction in total keyspace for tokens, and even with comment #16, we shouldn't treat this issue lightly. As I mentioned on IRC, gerv's explanation is based on how the PRNG is *supposed* to work, but can we prove it actually does that, though? Seems like we're putting too much faith in statistics and not enough in ensuring that the PRNG will actually do what it's supposed to do. We've already been bit before by PRNG issues (remember bug 619594?), so I just want to lean conservative and ensure we treat this as a valid security bug.
Comment 28 Frédéric Buclin 2013-08-20 12:39:05 PDT
(In reply to Reed Loden [:reed] from comment #27)
> Fairly sure he's talking about my stance that this should be treated as a
> security *bug*, not a security *enhancement*.

No, I was talking about your comment 22. This situation cannot happen.
Comment 29 David Lawrence [:dkl] 2013-08-20 13:16:50 PDT
Created attachment 793061 [details] [diff] [review]
patch for 4.4 and trunk
Comment 30 Frédéric Buclin 2013-08-20 14:19:58 PDT
Comment on attachment 793061 [details] [diff] [review]
patch for 4.4 and trunk

Works fine and looks good. r=LpSolit
Comment 31 Frédéric Buclin 2013-08-20 14:24:45 PDT
The patch doesn't apply cleanly on the 4.2 and 4.0 branches. Backports are needed.
Comment 32 David Lawrence [:dkl] 2013-08-20 15:12:45 PDT
Created attachment 793137 [details] [diff] [review]
patch for 4.0 and 4.2

token.cgi needed changes due to not using Bugzilla::Token::GetTokenData the same way as 4.4/trunk.
Comment 33 Daniel Veditz [:dveditz] 2013-08-26 08:55:02 PDT
There's no risk of collision and long odds on pulling off a brute force. If we were really worried about brute-force attacks on the token we'd make it longer whether or not we fix the case-sensitivity. I'm calling this sec-low, not moderate. There's really no plausible real-world attack here.
Comment 34 Frédéric Buclin 2013-08-26 13:24:51 PDT
Comment on attachment 793137 [details] [diff] [review]
patch for 4.0 and 4.2

>=== modified file 'token.cgi'

>+  (defined $db_token && $db_token eq $token && $tokentype)
>+    || ThrowUserError("token_does_not_exist");

Please remove && $tokentype from the check. If the token exists, then $tokentype is always defined.


r=LpSolit with the comment fixed on checkin.
Comment 35 Daniel Veditz [:dveditz] 2013-09-23 15:39:09 PDT
Does not qualify for a web security bounty
Comment 36 David Lawrence [:dkl] 2013-10-16 09:11:10 PDT
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/4.0
modified token.cgi
modified Bugzilla/Token.pm
modified template/en/default/global/code-error.html.tmpl
Committed revision 7757.

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/4.2
modified token.cgi
modified Bugzilla/Token.pm
modified template/en/default/global/code-error.html.tmpl
Committed revision 8229.

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/4.4
modified Bugzilla/Token.pm
modified template/en/default/global/code-error.html.tmpl
Committed revision 8621

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified Bugzilla/Token.pm
modified template/en/default/global/code-error.html.tmpl
Committed revision 8775.
Comment 37 David Lawrence [:dkl] 2013-10-16 09:28:59 PDT
Forgot the 'fix on checkin' change needed.

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/4.0
modified token.cgi
Committed revision 7759.

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/4.2
modified token.cgi
Committed revision 8231.

dkl
Comment 38 Frédéric Buclin 2013-10-17 07:58:49 PDT
Security advisory sent.

Note You need to log in before you can comment on or make changes to this bug.