119524 - SECURITY: predictable sessionid (Use a token instead of logincookie)

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect, P2)

Description

[peterw]

We're using bugzilla at work for some internal development projects; thank you
for your continued work on this project.

Today I noticed a security problem with the session management/authentication
system. Bugzilla requires two cookies: one with the userid, the other with a
session number. The session value is sequential, and therefore (relatively)
easily guessed. An attacker could theoretically assume someone else's identity
by noting their email address (which is in all the tickets they touch) and
trying different session numbers til one works. E.G., log in at the beginning of
the day, note the session number, wait for the victim to post a change to a
ticket, log in again, check the session number, then try connecting with the
victim's email address as Bugzilla_login, trying all session numbers between the
later number and the earlier number as Bugzilla_logincookie, until one works.

On systems with fewer users (including high-profile projects like OpenSSH
[bugzilla.mindrot.org]), the attack would be easier to accomplish.

Suggested improvements:

Option One (easiest to retrofit): shared secret. 

Add another configuration variable (populated by a random value when Bugzilla is
installed) for a site-wide shared secret. Set another cookie, Bugzilla_token,
which is something like the hexadecimal MD5 or SHA hash of
"${sessionid}-${secret}-${logintime}". (Might also want to add the User-Agent
request header value sent by the browser used at login time. The login time is
critical -- without login time, a user could brute-force the secret by looking
at their own cookies, making the attack process trivially more complex [the
attacker would only have to calculate the hash for each sessionid to be
attempted].) The access control code would use Bugzilla_logincookie to find the
user's time of login, to calculate the expected Bugzilla_token value. No match
== security error message. Disadvantage: someone with read access (no write
privileges needed) to the bugzilla server config could obtain the shared secret
and use it to attack sessions

Option Two (preferred, requires database change): per-session random tokens

Add a third cookie, Bugzilla_token. Add a column to the session table,
tokenhash. When a session is initiated, obtain/generate some random data. Store
a seeded MD5  or SHA hash of that data to the session table, and set the cookie
to contain the hexadecimal representation of the data itself. The access control
code would use Bugzilla_logincookie to find the salt for the stored hash, use
the provided Bugzilla_token data to calculate the hash of the client-provided
data with that salt, and see if the result matches the stored salted hash. No
match == security error message. Advantage: an attacker would have to be able to
control/predict the random number generation to hijack a session. Disadvantage:
would require modifications to database schema, making this improvment a more
difficult upgrade for running sites to use.

Option Two Minus (secure but ugly hack): abusing database definitions

You could probably wedge the per-session salted hash described in Option Two
inside some other column in the session table, e.g. in the userid/email column
store "${saltedhash}\t${email}" and then parse them as needed. No database
changes (== easy upgrades for running sites), but possibly gobs of code changes,
and a really awful abuse of the database design. 

Recommendation: Option One now, Option Two later

While not as strong as Option Two, Option One should dramatically improve the
security of the cookie authentication mechanism, and would be an easy
patch/upgrade for current users. Option Two would be preferable in the long
term, though.

Thanks!

-Peter

Comment 1

[Jacob Steenhagen]

The Bugzilla_logincookie isn't really as insecure as it appears.  It is tied
directly (in the backend database) to the IP address it was issued to.  This
means that even if I knew your login session id was 45865 (randomly typed
number) it wouldn't do me any good unless I was at the same computer as you (or
behind the same firewall/proxy/nat box).

Comment 2

[peterw]

That's good to know, but it's still not as secure as it could be, right?

And anyone whose IP address changes (for instance, my home ISP uses a
transparent Web proxy, but only intermittently, so I have actually seen my
apparent IP address change when refreshing a Web page!) may face needless problems.

Thanks!

Comment 3

[Jacob Steenhagen]

Peter, yes that's true.  The IP changing problem is bug 20122.  I believe the
real problem is striking a balance. Some people, such as myself, are lucky
enough to have fairly static IP's and thus just log in one and have it work for
months.  Others, such as yourself, get a new IP address every 10 minutes.  Would
a "secure" hash of some sort that never expires be considered secure if it
weren't tied to the IP address?

Fortunately, Bugzilla is built to be useful even without cookies enabled, so
while it is a pain to log in all the time when the IP changes, it doesn't make
it impossible.

Comment 4

[Enrico Scholz]

I think, that a not to an IP tied but random authtoken is more secure and
functional than the current solution.

Any IP-based authentication over HTTP can not work because of the design of
HTTP; you can neither say that

* a people/session comes from exactly one IP, nor that
* a IP belongs to exactly one people/session

Reasons for case #1 are load-balancing proxies or dynamic IPs. They are subject
of bug 20122, about which I am cursing everytime I visit a bugzilla-system.

In case #2 you can have thousands of people sharing a proxy server and coming
from its IP. At my (small) university are 7000 more or less skilled students;
proxies of big ISPs may be used by even more people (perhaps less skilled but
more criminal instead ;) ). Possible attacks can be imagined easily.



Systems like bugzilla can be designed to work in any environment and not only in
a "balanced" one where everybody has exactly one IP only used by him.

The suggested solutions for unpredictable authtokens (without IP-tieding) would
not destroy the current functionality (even when cookies are disabled), but
would increase security and usability. Therefore, I do not see a reason why not
to use them.

Comment 5

[Matthew Tuck [:CodeMachine]]

The IP solution (once loosened a bit), is still improved security, and can
operate along side whatever happens here.  Hence it's off topic for this bug.

Comment 6

[Enrico Scholz]

Cutting the power-cable would improve security also. Alas, it removes
functionality (like the IP-based authentication).

It is on topic because the weak auth-tokens are justified with the non-working
and insecure IP "solution".

Comment 7

[Bradley Baetz (:bbaetz)]

The fix is to move the logincookies table to use the tokens table instead.

Comment 8

[Joel Peshkin]

Proposal:

table logincookies goes away
Bugzilla_logincookie cookies cease to be numbers and become a string
The string is stored in the tokens table as a "login" token.
The string is created by taking the crypt of some random-ish thing.
And the IP address is no longer used.

Comments?

Comment 9

[Gervase Markham [:gerv]]

Joel: sounds good, although we should rename the cookie to make migration
easier. Everyone will have to log in again after the switch; that's not a disaster.

We should store the IP address in the "eventdata" field of the tokens table, for
auditing/tracking purposes.

Gerv

Comment 10

[Dave Miller [:justdave]]

All 2.18 bugs that haven't been touched in over 60 days and aren't flagged as
blockers are getting pushed out to 2.20

Comment 11

[Max Kanat-Alexander]

I'm just re-assigning this to joel since he expressed a plan for how to do it
(which sounds fully sensible to me).

I kind of want to move this to an enhancement, because that's what it's become.
I'll leave it as "major" for now, though.

Comment 12

[Stephen Lee]

Really easy fix:

Change the PRIMARY KEY 'cookie' column from the 'logincookies' table, so that 
instead of an autoincrementing mediumint, it is something bigger.

Allow legacy login cookies (converted from an autoincrementing number) to 
remain, but generate new ones as a hash of something random.

checksetup.pl
[approx line 1810:]
-   'cookie mediumint not null auto_increment primary key,
+   'cookie varchar(64) not null primary key,
[above the "*** A B O V E *** this comment" line:]
+ ChangeFieldType("logincookies", "cookie", "varchar(64) not null");

Also the cookie generation and validation code - I would expect to find the 
place this accesses the logincookies table from this search:
http://lxr.mozilla.org/bugzilla/search?string=logincookie
... but it does not seem to be there for some reason :-(
Does this mean lxr is broken, or am I looking in the wrong place?

Anyway, it is simple enough to hash something, and use this as the cookie id 
instead of a sequential number.


This would be an even lower impact change equivalent in security to the "option 
one". (Advantage: users aren't necessarily inconvenienced by having to log in 
again after the change, since it converts existing data at no additional effort)


Slight variation of this, which I think would be equivalent in security to 
option two:
Instead of cookie, store hashcookie as the primary key of the logincookies 
table. Use hashfunction(Bugzilla_logincookie) to look up the correct row in the 
logincookies table (rather than Bugzilla_logincookie directly). Again, this 
could be made to work seamlessly with existing cookies if this was felt 
desirable simply by hashing the existing cookie column into the new hashcookie 
column.

Comment 13

[Dave Miller [:justdave]]

*** Bug 301058 has been marked as a duplicate of this bug. ***

Comment 14

[Olav Vitters]

I'm going to attach a work-in-progress patch that implements the "Really easy
fix" from comment 12. The patch will give some harmless offset errors (due to
local unrelated changes), but should otherwise be ok (you can login + existing
logins will still work).

Note: Seems that changing a field from primarykey to non-primarykey does not
work with MySQL. Should be fixed elsewhere, so work around that if you look at
this patch (alter table logincookies drop primary key).

What I've done:
- Changed checksetup.pl and Schema.pm to change 'cookie' in logincookies table
from a auto_increment to a non-primarykey varchar(16).
- Changed Bugzilla/Auth/Login/WWW/CGI.pm to set the 'cookie' using
Bugzilla::Token::GenerateUniqueToken()
- Changed Bugzilla/Auth/Login/WWW/CGI/Cookie.pm to update logincookies.lastused
using cookie and userid (as cookie is no longer unique).

Ideally I'd like cookie to be unique only for a userid. That way nobody can
login milions of times to reduce the available token space for other users.

It is work-in-progress because I'm not sure how to do the token stuff.
GenerateUniqueToken in Token.pm ensures the token isn't used within the tokens
table. This is not good enough as the token in used in the logincookies table
(field: cookie), so the unique check doesn't work. I could easily either change
GenerateUniqueToken to accept table/field or perhaps make a new function. This
also makes sense as GenerateUniqueToken uses the standard password length. As
the cookie contents will not be seen by a user (normally), I'd like to use 16
chars for extra security. However, the password length could also be a new
option passed to GenerateUniqueToken.

Note: A new GenerateUniqueToken-like function would allow the token to be unique
only for the userid (without making GenerateUniqueToken look like a total hack).

Comment 15

[Olav Vitters]

Attachment: Work in progress patch

See comment 14 for details. Taking bug.

Comment 16

[Stephen Lee]

(In reply to comment #14)
> Ideally I'd like cookie to be unique only for a userid. That way nobody can
> login milions of times to reduce the available token space for other users.

Hopefully the token space can be made big enough that this isn't an issue...
It certainly doesn't hurt to allow for collisions in tokenspace though.

Perhaps the table could have a multi-column PRIMARY KEY(userid,cookie) to
explicitly state this in the schema.

> It is work-in-progress because I'm not sure how to do the token stuff.

GenerateUniqueToken just calls GenerateRandomPassword with an additional check
for uniqueness. You may as well call GenerateRandomPassword directly, and not
worry about collision detection because only per-user collisions matter, and it
would take an implausible number of concurrent sessions to make a per-user
collision even remotely likely.


I was about to suggest some further "simple" changes to make the more secure
version, but I think that it involves enough additional thought to have its own
bug, which I will check if it exists, and otherwise file...

Comment 17

[Olav Vitters]

Attachment: Adds suggestions from comment 16

Features:
 * Keeps existing logincookies
 * New logins will have the more secure cookies
 * Has been tested

See previous comment for more details. I have added a new function called
_generate_login_token to Bugzilla/Auth/Login/WWW/CGI.pm as Token.pm is only
usable for the token table (it can't be reused).

Triggers the following bugs:
 * MySQL bug for the 'SELECT 1 FROM logincookies WHERE cookie IS NULL' check
done in bz_alter_column (Bugzilla/DB.pm)
   See http://bugs.mysql.com/?id=13535
 * The get_alter_column_ddl (Bugzilla/DB/Schema/Mysql.pm) not dropping the
primary key. Will file a bug for this.

Warning:
1. If you get the error: "You cannot alter the logincookies.cookie column to be
NOT NULL without specifying a default...". This is caused by the MySQL bug.
I'll file a Bugzilla bug to workaround this as this happens in the current
MySQL stable (4.1.14).
2. Bugzilla will not drop the primary key. Can cause errors. I'll file another
bug for this. When testing now, do a 'ALTER TABLE logincookies DROP PRIMARY
KEY' after running ./checksetup.pl.

Comment 18

[Max Kanat-Alexander]

Comment on attachment 197620 [details] [diff] [review]
Adds suggestions from comment 16

>Index: Bugzilla/Auth/Login/WWW/CGI.pm
>+sub _generate_login_token {

  Instead of this function... there must already be a function to generate a
token in token.cgi or Token.pm, right? Use that one. All token functions should
be using the same generation code, and all tokens should be the same length. If
the function that we have doesn't do what you need, then it should be modified
appropriately.

>-        $dbh->do("UPDATE logincookies SET lastused=NOW() WHERE cookie=?",
>+        $dbh->do("UPDATE logincookies " .
>+                 "SET lastused=NOW() " .

  Nit: There's no reason to separate the UPDATE and SET on these lines.

>Index: Bugzilla/DB/Schema.pm
>     logincookies => {
>         FIELDS => [
>-            cookie   => {TYPE => 'MEDIUMSERIAL', NOTNULL => 1,
>-                         PRIMARYKEY => 1},
>+            cookie   => {TYPE => 'varchar(16)', NOTNULL => 1},
>             userid   => {TYPE => 'INT3', NOTNULL => 1},
>             ipaddr   => {TYPE => 'varchar(40)', NOTNULL => 1},
>             lastused => {TYPE => 'DATETIME', NOTNULL => 1},
>         ],
>         INDEXES => [
>             logincookies_lastused_idx => ['lastused'],
>+            logincookies_userid_idx => {FIELDS => [qw(userid cookie)],
>+                                        TYPE => 'UNIQUE'},

  I don't see why the token shouldn't be globally unique, as opposed to just
unique per-user. All our other tokens are globally unique... aren't they?

Comment 19

[Olav Vitters]

Attachment: V3: Fix comments from review (Bugzilla HEAD 27 Sep 2005)

>>Index: Bugzilla/Auth/Login/WWW/CGI.pm
>>+sub _generate_login_token {
>
>  Instead of this function... there must already be a function to generate a
>token in token.cgi or Token.pm, right? Use that one. All token functions
should
>be using the same generation code, and all tokens should be the same length.
If
>the function that we have doesn't do what you need, then it should be modified

>appropriately.

Didn't want to do that as I wanted to make a token userid-specific + longer
'password'. But as I have changed the patch to not be userid-specific, I've
modified Bugzilla::Token::GenerateUniqueToken to suit my needs (being able to
specify table + column the function looks at).

>>-	   $dbh->do("UPDATE logincookies SET lastused=NOW() WHERE cookie=?",
>>+	   $dbh->do("UPDATE logincookies " .
>>+		    "SET lastused=NOW() " .
>
>  Nit: There's no reason to separate the UPDATE and SET on these lines.

This change was required because the token was userid-specific. As only the
token is now unique again, this change isn't needed.

>>Index: Bugzilla/DB/Schema.pm
>>     logincookies => {
>>	   FIELDS => [
>>-	       cookie	=> {TYPE => 'MEDIUMSERIAL', NOTNULL => 1,
>>-			    PRIMARYKEY => 1},
>>+	       cookie	=> {TYPE => 'varchar(16)', NOTNULL => 1},
>>	       userid	=> {TYPE => 'INT3', NOTNULL => 1},
>>	       ipaddr	=> {TYPE => 'varchar(40)', NOTNULL => 1},
>>	       lastused => {TYPE => 'DATETIME', NOTNULL => 1},
>>	   ],
>>	   INDEXES => [
>>	       logincookies_lastused_idx => ['lastused'],
>>+	       logincookies_userid_idx => {FIELDS => [qw(userid cookie)],
>>+					   TYPE => 'UNIQUE'},
>
>  I don't see why the token shouldn't be globally unique, as opposed to just
>unique per-user. All our other tokens are globally unique... aren't they?

I wanted to avoid someone being able to limit the token-space by running a
script that would login 500 times/sec. But, even with a 10 char token this
would still take a long time. In this patch I have kept the cookie globally
unique (for the logincookies table.. a token could exists in the logincookies
tables and in the tokens table).

Note that it isn't possible to put the logincookies in the tokens table, data
doesn't match + records in the tokens table is deleted after 3 days. Having it
in logincookies is clean.

Comment 20

[Olav Vitters]

Comment on attachment 197724 [details] [diff] [review]
V3: Fix comments from review (Bugzilla HEAD 27 Sep 2005)

From IRC:

<bkor> mkanat: do you have time to review my patches (change of logincookies +
support changing primary key in MYSQL + MySQL bug workaround)? I see I could
ask Tomas for the MySQL bug workaround, but the rest touches Schema.. I need
this in 2.20 and want to ensure 2.22 will have the same changes

<mkanat> bkor: I don't have time, actually. You could ask Tomas for the whole
thing.

This contains Schema changes, but as you can see mkanat said I could ask you.
Requires patches to workaround a MySQL bug (bug 310325) and a patch to make
primary key changes work on MySQL (bug 310231). The generic (well.. that
currently means Postgresql) one works fine. I think I explained everything in
the comments of every bug. In this bug, skip everything till comment 12 or so.

Comment 21

[Max Kanat-Alexander]

Comment on attachment 197724 [details] [diff] [review]
V3: Fix comments from review (Bugzilla HEAD 27 Sep 2005)

This looks great to me, design-wise! I don't know enough about the login system to test it (I don't know all the areas to cover) but kiko's our Auth guy. :-)

Comment 22

[Max Kanat-Alexander]

Comment on attachment 197724 [details] [diff] [review]
V3: Fix comments from review (Bugzilla HEAD 27 Sep 2005)

OK, basic testing from me, on landfill, shows that this works. I also examined the code in various places, and we don't detaint_natural the logincookie anywhere that I can see, so it should be fine for it to suddenly be a varchar.

We should relnote that admins who are paranoid about security should clear their logincookies table immediately after they upgrade.

Comment 23

[Dave Miller [:justdave]]

Being that this is sort of a security issue (although not a very big one) I'd be willing to take backports of this on the branches if they aren't too invasive.  The patch on the trunk isn't that bad, but I can imagine this being a bigger problem to fix with some of the older code...

Comment 24

[Frédéric Buclin]

The patch for tip has a+ on it, there is no reason to hold the checkin any longer. Leaving the bug open due to comment 23.

tip:

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.464; previous revision: 1.463
done
Checking in Bugzilla/Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.39; previous revision: 1.38
done
Checking in Bugzilla/Auth/Login/WWW/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/WWW/CGI.pm,v  <--  CGI.pm
new revision: 1.14; previous revision: 1.13
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.43; previous revision: 1.42
done

Comment 25

[Olav Vitters]

Attachment: 2.20 backport + fix for bug 322244 (2.20 Branch 2006-01-07)

Due to the landing of bug 300473 the patch on this bug didn't apply anymore to the 2.20 branch. This patch is a rediff + also includes the fix for bug 322244 (reviewed + fixed on HEAD).

Honestly do not care if this goes into 2.20 (I patched it locally). Important for me is that it is in Bugzilla CVS HEAD / 2.22. Attaching just in the case that it is wanted for 2.20 the patch will actually apply.

Comment 26

[Olav Vitters]

2.20 backport would need:
 * review from patch in bug 310325
 * review from 2.20 backport in this bug
 *patch in bug 310231 (still applies cleanly to 2.20)

Comment 27

[Max Kanat-Alexander]

Thanks for all your work on the patch, but I have decided, and justdave has approved, that we don't need this backported.

Comment 28

[Max Kanat-Alexander]

Added to the Bugzilla 2.22 Release Notes in bug 322960, including information in comment 22.

Comment 29

[Robert Chapin]

There is a problem with this bug in 
https://bugzilla.mozilla.org/page.cgi?id=upgrade-2006-12-26.html

It is described as "Login cookies now use a randomized token instead of sequential session IDs, greatly lessening the need to tie a cookie to an IP address to keep your login secure."

That description is not accurate because it sounds like you guys are trading one guessable cookie for another, a bad solution.

Having read this thread, I think the bug description needs to say, "Login cookies now use a randomized token attached to the user name."

Comment 30

[Olav Vitters]

Even with the old way you had to know someones userid and their cookie. So the part about 'attached to the user name' should not be mentioned (it was never not 'attached'). However, we make no effort to hide someones userid (do not know where it is actually visible.. but it is not considered security sensitive).