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

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).

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!

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.

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.

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.

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".

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

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?

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

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

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.

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.

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

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).

See comment 14 for details. Taking bug.

(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...

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 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?

>>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 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 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 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.

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...

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

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.

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)

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

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

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."

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).