The logincookies table has a hostname field. This is obtained from the webserver via REMOTE_HOST env variable. The webserver gets this by doing a reverse DNS lookup. Therefore an attacker which can provide reverse dns for their IP can bypass the IP address checks by providing the hostname of the user they want to spoof. Hostname lookups are disabled by default in apache, so this won't affect most people, which is why if you look at your logincookies table you'll see ip addresses. Its still an attack, though, and should be fixed for 2.16. I've been looking for an excuse to get bug 20122 in for 2.16, and since fixing this bug is about 2/3 of the work I'll use this. ;) An open question which I was thinking about before I realised the security issues: REMOTE_ADDR can be ipv4 or ipv6. If its ipv4, then the field is set to varchar(15) NOT NULL (or int4 NOT NULL) and we're done. If its ipv6, the field has to be larger. (I don't plan on supporting the netmask stuff for ipv6). If we want to validate that the string given by the server is actually an IP, then we have to drag in some more modules. Thoughts?
I think this is needed for 2.16 - retarget if you disagree.
Created attachment 73333 [details] [diff] [review] patch DELETE FROM logincookies (or TRUNCATE TABLE logincookies) seems to reset the primary key count, and then the next entry inserts at 0. Does this matter? DELETE FROM logincookies where cookie>0 keeps it as is. Anyone know why that happens?
r=gerv if Dave agrees we want to do this. Gerv
This was way easier than I thought, and the ipv4 vs 6 issues don't come into this anyway. (Whoever thought of using base85 (no, thats not a typo) to represent ip addresses...) I don't think there is risk to it, and it is a security bug.
Comment on attachment 73333 [details] [diff] [review] patch >Index: checksetup.pl >- hostname varchar(128), >+ ip varchar(40) NOT NULL, Nit: this would be more accurate as "ip_address" or "ipaddr". >@@ -2688,6 +2687,21 @@ >+ # We've changed what we match against, so all entries are now invalid >+ # Note that in most cases, REMOTE_HOST won't be set by apache since >+ # reverse lookups are disabled by default. However, only deleting >+ # non-ipv4 formatted strings won't work if a host could resolve to a >+ # numeric IP. (Can they?) Aren't numeric IPs the only thing to which hosts resolve? Looks good, works in tests. r=myk
I plan to make it an ip-network address (when a valid netmask is given in prefs). The comment should read ".. if an ip could resolve to".... I'm concerned about reverse resolutions here, really.
Comment on attachment 73333 [details] [diff] [review] patch <justdave> heck yes, we want that marking r=gerv from his comment I'll take better suggestions for a field name, though. ip implies address in this context, and acceptable_source isn't really what I want. Any name needs to allow for the network mask I want (else we'll just have to have this discussion again later), so src_addr is out.
Created attachment 74062 [details] [diff] [review] v2 change to using ipaddr, and removed the confusing comment
Comment on attachment 74062 [details] [diff] [review] v2 r=gerv. Don't forget to fix the date in checksetup.pl. Gerv
Comment on attachment 74062 [details] [diff] [review] v2 r=justdave. Don't forget to fix the date in checksetup.pl.
Checked in, with today's PST date (2002-03-15)
Created attachment 83021 [details] [diff] [review] Backported patch for BUGZILLA-2_14_1-BRANCH Interesting thing to note about this patch: the logincookies table still contains the cryptedpassword field.
The reason I said 'no schema change' was because this is technically a stable release. Just call it .hostname, and comment that 2.16 changes the column name to be more correct. What do others think?
Comment on attachment 83021 [details] [diff] [review] Backported patch for BUGZILLA-2_14_1-BRANCH bbaetz is right - there's no real need to change the schema. However, if we don't, then we don't know when to do: $dbh->do("DELETE FROM logincookies"); So the only option would be to execute this every time checksetup runs, which would suck. So, unless anyone can think of a better idea, I think we need to change the schema. Gerv
So, the problem is we don't want to make the schema change, but we still want to make the bug update by storing IPs instead of hostnames, but we have to delete all the logincookies which contain host names at least *once* during the upgrade, right? I assume checksetup.pl can query the database while it's running, yes? We could put an SQL check in there to check for entries in the logincookies table that contain any text in the "hostname" column; if there's any text (i.e. a hostname), then run the DELETE to flush everything out; if there's only IPs, then don't. Does that sound like it would that give us the DELETE we need during the install of the patch, but not require a DELET everytime checksetup is run?
Sounds like it would do the trick. Gerv
Eww. The delete was only really needed to tidy things up. If a system has HostnameLookups enabled in their apache config, then those entries will be invalid, and they'll be cleaned out in three day's time anyway. The only possible security hole after applying this patch would be if this option was enabled, AND the reverse lookup for a real IP was another IP. In that case, if you also had their cookie, you could pretend to be from that IP. Note that its teh real IP which has to have that funny resolution is the person you're trying to impersonate, not you, so the chances of this are about zero. Without this patch, it can be you playing with the reverse lookup, which is why this is a security hole. Lets forget the deletion, I think.
bbaetz's analysis seems to be correct, but my gut tells me to just invalidate the entries. This is a security patch, and while the analysis seems valid, the problem with security patches is that sometimes there are a lot of unknowns. For instance, vendors don't always follow the RFCs (a situation could arise where one IP reverses to a string that has the format of another IP, even though it's probably against the DNS RFC) and other such craziness exists. All in all, I think it's better to be safe than get a report a couple weeks later that someone's BZ installation got hacked because they're using a funky version of WinNT and they had some weird option setup and we missed something. I'll attach a new version of the 2_14_1-BRANCH patch for comment/review.
Created attachment 83230 [details] [diff] [review] Backported patch, v.2 --Removes the schema change; documents why --Invalidates all entries only if ONE of them is not an IP address
Can't you do the matching as part of the select? That will take _ages_ to run on somewhere like bmo. What about deleting if there is _anything_ which doesn't start with [1-9]? The case of all reverse IPs starting with numbers is really really inprobable, although I suppose its possible if an installation was local to an organisation. Can we just use the regexp in the search? Do an EXPLAIN on it, and see how long it takes, maybe? Maybe we should just do the schema change anyway?
Comment on attachment 83230 [details] [diff] [review] Backported patch, v.2 I'd go for that. Gerv
Comment on attachment 83230 [details] [diff] [review] Backported patch, v.2 Changed my mind. Bbaetz is right. We need to either do something faster, or just make the schema change anyway. Gerv
Created attachment 83254 [details] [diff] [review] Backported patch, v3 --just like v2, except it puts the regex query in the SQL; we should probably test this SQL if we choose this patch; I ran it on a mysql server I'm running (to check against IP addresses, not hostnames) and it worked, but that doesn't prove it's right (just proves it's not wrong).
I really have no preference on which patch goes in... I don't see that much risk in changing the schema (as long as we're sure that the code that does that isn't buggy), but I can see how some people might be, so the other version that checks the entries in the table for hostnames should work too. My vote goes for the backported v3 patch, but as I said I don't really care; you can now choose between the v3 patch and the v1 patch depending on what you decide; let me know, and I'll check either in.
As much as I hate to make a schema change on a "stable" branch, NOT doing so means they're going to lose all their stored cookies again if they upgrade to 2.16 later. I'd just as soon go ahead and make the schema change. The necessity of it for the security fix is a good enough excuse.
Adding representatives of the packagers to bugs that are going into the Bugzilla 2.14.2 security update
Mmm... good point. I've obsoleted the patch(es) which didn't contain the schema change, which leaves the first version of the patch; reviews? A 2.14.2 -> 2.16 upgrade is going to incur other schema changes (dropping cryptedpassword in logincookies, for one) anyway, but I guess that's to be expected. We actually could get around the issue by putting a rename column statement in the 2.16 checksetup, but that could get confusing and messy... it's probably easier to bite the bullet and just update the schema on the stable branch.
Comment on attachment 83021 [details] [diff] [review] Backported patch for BUGZILLA-2_14_1-BRANCH r=justdave Looks good to me.
Comment on attachment 83021 [details] [diff] [review] Backported patch for BUGZILLA-2_14_1-BRANCH Fix both xx's in the dates before checking in. On the trunk and 2.16 branch, add the backport comment.
Created attachment 83340 [details] [diff] [review] Backported patch, v4 This patch differs only from the first patch (attachment 83021 [details] [diff] [review]), which has r=/2r= by the comments, at bbaetz's request. This patch will go into the tree.
v4 patch checked in.
moving secure bugzilla/webtools bugs from mozilla security group to the new bugzilla security group.
Comment on attachment 83340 [details] [diff] [review] Backported patch, v4 adding review flags for reviews that were stated in the comments
2.14.2 is out, removing security group.