The default bug view has changed. See this FAQ.

logincookies IP check can be bypassed

RESOLVED FIXED in Bugzilla 2.16

Status

()

Bugzilla
User Accounts
P1
blocker
RESOLVED FIXED
15 years ago
4 years ago

People

(Reporter: bbaetz, Assigned: bbaetz)

Tracking

2.15
Bugzilla 2.16
x86
Linux

Details

(Whiteboard: applied to 2.14.2)

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

15 years ago
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?
(Assignee)

Comment 1

15 years ago
I think this is needed for 2.16 - retarget if you disagree.
Assignee: myk → bbaetz
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
(Assignee)

Comment 2

15 years ago
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
Keywords: patch, review
(Assignee)

Comment 4

15 years ago
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
Attachment #73333 - Flags: review+
(Assignee)

Comment 6

15 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 7

15 years ago
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.
Attachment #73333 - Flags: review+
(Assignee)

Comment 8

15 years ago
Created attachment 74062 [details] [diff] [review]
v2

change to using ipaddr, and removed the confusing comment
Attachment #73333 - Attachment is obsolete: true
Comment on attachment 74062 [details] [diff] [review]
v2

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

Gerv
Attachment #74062 - Flags: review+
Comment on attachment 74062 [details] [diff] [review]
v2

r=justdave. Don't forget to fix the date in checksetup.pl.
Attachment #74062 - Flags: review+
(Assignee)

Comment 11

15 years ago
Checked in, with today's PST date (2002-03-15)
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
munging ccs
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.
(Assignee)

Comment 14

15 years ago
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
(Assignee)

Comment 18

15 years ago
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
Attachment #83021 - Attachment is obsolete: true
(Assignee)

Comment 21

15 years ago
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
Attachment #83230 - Flags: review+
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
Attachment #83230 - Flags: review+
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).
Attachment #83230 - Attachment is obsolete: true
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

Updated

15 years ago
Attachment #83254 - Attachment is obsolete: true

Updated

15 years ago
Attachment #83021 - Attachment is obsolete: false
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.
Attachment #83021 - Flags: review+
(Assignee)

Comment 30

15 years ago
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.
Attachment #83021 - Flags: review+
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.
Attachment #83021 - Attachment is obsolete: true
v4 patch checked in.
(Assignee)

Comment 33

15 years ago
moving secure bugzilla/webtools bugs from mozilla security group to the new
bugzilla security group.
Group: security? → webtools-security?
Comment on attachment 83340 [details] [diff] [review]
Backported patch, v4

adding review flags for reviews that were stated in the comments
Attachment #83340 - Flags: review+
Whiteboard: applied to 2.14.2
2.14.2 is out, removing security group.
Group: webtools-security?
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.