129466 - logincookies IP check can be bypassed

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect, P1)

Description

[Bradley Baetz (:bbaetz)]

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?

Comment 1

[Bradley Baetz (:bbaetz)]

I think this is needed for 2.16 - retarget if you disagree.

Comment 2

[Bradley Baetz (:bbaetz)]

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

Comment 3

[Gervase Markham [:gerv]]

r=gerv if Dave agrees we want to do this.

Gerv

Comment 4

[Bradley Baetz (:bbaetz)]

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 5

[Myk Melez [:myk] [@mykmelez]]

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

Comment 6

[Bradley Baetz (:bbaetz)]

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 7

[Bradley Baetz (:bbaetz)]

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.

Comment 8

[Bradley Baetz (:bbaetz)]

Attachment: v2

change to using ipaddr, and removed the confusing comment

Comment 9

[Gervase Markham [:gerv]]

Comment on attachment 74062 [details] [diff] [review]
v2

r=gerv. Don't forget to fix the date in checksetup.pl.

Gerv

Comment 10

[Dave Miller [:justdave]]

Comment on attachment 74062 [details] [diff] [review]
v2

r=justdave. Don't forget to fix the date in checksetup.pl.

Comment 11

[Bradley Baetz (:bbaetz)]

Checked in, with today's PST date (2002-03-15)

Comment 12

[Dave Miller [:justdave]]

munging ccs

Comment 13

[J. Paul Reed [:preed]]

Attachment: Backported patch for BUGZILLA-2_14_1-BRANCH

Interesting thing to note about this patch: the logincookies table still
contains the cryptedpassword field.

Comment 14

[Bradley Baetz (:bbaetz)]

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 15

[Gervase Markham [:gerv]]

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

Comment 16

[J. Paul Reed [:preed]]

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?

Comment 17

[Gervase Markham [:gerv]]

Sounds like it would do the trick.

Gerv

Comment 18

[Bradley Baetz (:bbaetz)]

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.

Comment 19

[J. Paul Reed [:preed]]

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.

Comment 20

[J. Paul Reed [:preed]]

Attachment: Backported patch, v.2

--Removes the schema change; documents why
--Invalidates all entries only if ONE of them is not an IP address

Comment 21

[Bradley Baetz (:bbaetz)]

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 22

[Gervase Markham [:gerv]]

Comment on attachment 83230 [details] [diff] [review]
Backported patch, v.2

I'd go for that.

Gerv

Comment 23

[Gervase Markham [: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

Comment 24

[J. Paul Reed [:preed]]

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

Comment 25

[J. Paul Reed [:preed]]

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.

Comment 26

[Dave Miller [:justdave]]

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.

Comment 27

[Dave Miller [:justdave]]

Adding representatives of the packagers to bugs that are going into the
Bugzilla 2.14.2 security update

Comment 28

[J. Paul Reed [:preed]]

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 29

[Dave Miller [:justdave]]

Comment on attachment 83021 [details] [diff] [review]
Backported patch for BUGZILLA-2_14_1-BRANCH

r=justdave
Looks good to me.

Comment 30

[Bradley Baetz (:bbaetz)]

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.

Comment 31

[J. Paul Reed [:preed]]

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

Comment 32

[J. Paul Reed [:preed]]

v4 patch checked in.

Comment 33

[Bradley Baetz (:bbaetz)]

moving secure bugzilla/webtools bugs from mozilla security group to the new
bugzilla security group.

Comment 34

[Dave Miller [:justdave]]

Comment on attachment 83340 [details] [diff] [review]
Backported patch, v4

adding review flags for reviews that were stated in the comments

Comment 35

[Dave Miller [:justdave]]

2.14.2 is out, removing security group.