Closed Bug 906745 Opened 11 years ago Closed 11 years ago

In MySQL, tokens are not case-sensitive, reducing total entropy and allowing easier brute force

Categories

(Bugzilla :: Bugzilla-General, defect)

4.2.6
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: anandtiwarics, Assigned: dkl)

Details

(Keywords: sec-low, wsec-authentication, wsec-crypto, Whiteboard: [site:bugzilla.mozilla.org][reporter-external])

Attachments

(2 files, 2 obsolete files)

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.
I can confirm this, at least on bugzilla.mozilla.org. Will test normal Bugzilla next.
Assignee: nobody → user-accounts
Group: websites-security → bugzilla-security
Status: UNCONFIRMED → NEW
Component: Other → User Accounts
Ever confirmed: true
Product: Websites → Bugzilla
QA Contact: default-qa
Summary: Found Logical Bug in bugzilla.mozilla.org → Tokens are not case-sensitive, reducing total entropy and allowing easier brute force
Version: unspecified → 4.2.6
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
Flags: sec-bounty?
Whiteboard: [site:bugzilla.mozilla.org][reporter-external]
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);
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.
or possibly utf8_bin collation
reed - I'd go with utf8_bin.
Attached patch 906745_1.patch (obsolete) — Splinter Review
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
Attachment #792435 - Flags: review?(glob)
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.
(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 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.
Attachment #792435 - Flags: review?(glob) → review-
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().
Attachment #792435 - Flags: review-
This problem affects all places where tokens are in use, not only user accounts.
Assignee: user-accounts → general
Severity: normal → major
Component: User Accounts → Bugzilla-General
Flags: blocking4.4.1+
Flags: blocking4.2.7+
Flags: blocking4.0.11+
Target Milestone: --- → Bugzilla 4.0
(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.
Summary: 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 easier brute force
(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.
Attachment #792435 - Flags: review-
(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.
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
Attached patch 906745_2.patch (obsolete) — Splinter Review
This takes LpSolit's idea of just doing the comparison in Perl which negates the need to do anything special per database.
Assignee: general → dkl
Attachment #792435 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #792975 - Flags: review?(LpSolit)
(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.
Severity: major → normal
Keywords: sec-critical
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.
(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
(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.
(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...
i am with reed on htis one.  I see this as at least a moderate risk.
Keywords: sec-moderate
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.
Attachment #792975 - Flags: review?(LpSolit) → review-
(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!
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.
(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.
(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.
Attachment #792975 - Attachment is obsolete: true
Attachment #793061 - Flags: review?(LpSolit)
Comment on attachment 793061 [details] [diff] [review]
patch for 4.4 and trunk

Works fine and looks good. r=LpSolit
Attachment #793061 - Flags: review?(LpSolit) → review+
Flags: approval?
Flags: approval4.4?
Flags: approval4.2?
Flags: approval4.0?
The patch doesn't apply cleanly on the 4.2 and 4.0 branches. Backports are needed.
Flags: approval4.2?
Flags: approval4.0?
Attachment #793061 - Attachment description: 906745_3.patch → 906745_3.patch (4.4/trunk)
token.cgi needed changes due to not using Bugzilla::Token::GetTokenData the same way as 4.4/trunk.
Attachment #793137 - Flags: review?(LpSolit)
Attachment #793061 - Attachment description: 906745_3.patch (4.4/trunk) → patch for 4.4 and trunk
Attachment #793137 - Attachment description: 906745_40_42_1.patch (4.0/4.2) → patch for 4.0 and 4.2
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.
Keywords: sec-moderatesec-low
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.
Attachment #793137 - Flags: review?(LpSolit) → review+
Flags: approval4.2?
Flags: approval4.0?
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval4.2?
Flags: approval4.2+
Flags: approval4.0?
Flags: approval4.0+
Flags: approval+
Does not qualify for a web security bounty
Flags: sec-bounty? → sec-bounty-
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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
Security advisory sent.
Group: bugzilla-security
You need to log in before you can comment on or make changes to this bug.