Last Comment Bug 728639 - (CVE-2012-0465) [SECURITY] User lockout policy can be bypassed by altering the X-FORWARDED-FOR header
(CVE-2012-0465)
: [SECURITY] User lockout policy can be bypassed by altering the X-FORWARDED-FO...
Status: RESOLVED FIXED
[infrasec:auth][ws:moderate]
:
Product: Bugzilla
Classification: Server Software
Component: User Accounts (show other bugs)
: 3.5.3
: All All
: -- major (vote)
: Bugzilla 3.6
Assigned To: Frédéric Buclin
: default-qa
:
Mentors:
Depends on:
Blocks: 835424 729001 741079
  Show dependency treegraph
 
Reported: 2012-02-18 19:27 PST by Soroush Dalili
Modified: 2014-06-27 14:09 PDT (History)
11 users (show)
LpSolit: approval+
LpSolit: approval4.2+
LpSolit: blocking4.2.1+
LpSolit: approval4.0+
LpSolit: blocking4.0.6+
LpSolit: approval3.6+
LpSolit: blocking3.6.9+
rforbes: sec‑bounty+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (1.89 KB, patch)
2012-02-19 05:01 PST, Frédéric Buclin
glob: review-
Details | Diff | Splinter Review
patch for 4.0, v2 (6.86 KB, patch)
2012-03-26 09:34 PDT, Frédéric Buclin
glob: review+
Details | Diff | Splinter Review
patch for 3.6, v1 (6.13 KB, patch)
2012-04-12 11:53 PDT, Frédéric Buclin
LpSolit: review+
Details | Diff | Splinter Review
patch for 4.2 + trunk, v2.1 (6.85 KB, patch)
2012-04-18 09:50 PDT, Frédéric Buclin
LpSolit: review+
Details | Diff | Splinter Review

Description Soroush Dalili 2012-02-18 19:27:43 PST
User Agent: Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; WOW64; Trident/5.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E)

Steps to reproduce:

It is possible to bypass the user lockout policy by using the "X-FORWARDED-FOR" in the header of the request. 
The problem is because of the "remote_ip" function in "Bugzilla/Util.pm" which adds this header value to the user's IP address (there is also no validation on this field). However, this "X-FORWARDED-FOR" can be controlled by an attacker to brute force the other people username/password. The "remote_ip" function also has been used in lockout modules ("Bugzilla/User.pm: account_ip_login_failures()") which is the reason of having this issue.

Please let me know if it can be eligible for the bug bounty too.


Actual results:

It is possible to brute force a password without being locked by setting the "X-FORWARDED-FOR" value in the header of the authentication request to one of these:
1- A complete random IP or String
2- A string with the length of more than 26 characters. Ex.: "123456789012345678901234567"


Expected results:

It should prevent from brute force attack.
My recommendation is:
1- First we need to have a complete validation (characters, length, format) for $ENV{'HTTP_X_FORWARDED_FOR'} in the "remote_ip" function.
2- This value should not be used in the user's lockout process as it can be controlled by the user. Only the main IP address should be fine for this action ($ENV{'REMOTE_ADDR'}) although it can be a proxy IP address.
Comment 1 Reed Loden [:reed] (use needinfo?) 2012-02-18 21:46:12 PST
https://github.com/jsocol/commonware/blob/master/commonware/request/middleware.py has a fairly good Python-based way of doing validation.
Comment 2 Frédéric Buclin 2012-02-19 01:59:12 PST
remote_ip() has been added in Bugzilla 3.5.3, see bug 527586. Note that "X-FORWARDED-FOR" is only taken into account if the 'inbound_proxies' parameter is set.

(In reply to Reed Loden [:reed] (very busy) from comment #1)
> middleware.py has a fairly good Python-based way of doing validation.

But this validation won't help if the attacker is free to set the IP address he wants, right? He just has to pass a valid IP address to defeat this check. The equivalent of socket.inet_aton(ip) in Python is Socket::getaddrinfo() in Perl, but it doesn't check that this is really the IP address the request is coming from.
Comment 3 Frédéric Buclin 2012-02-19 04:16:10 PST
If I understand correctly how the X-Forwarded-For header is built, a trusted proxy will append the real remote IP address to the existing X-Forwarded-For header, and so this header could look like so (in the example below, there are 3 trusted proxies in the chain):

  fake_or_real1, fake_or_real2, real_remote_ip, trusted_proxy1, trusted_proxy2

So the first addresses when starting from the left cannot be trusted, because they are easily faked by the client. The last addresses on the right are our trusted proxies and so we are confident about them. So if we take the last IP address not matching one of our trusted proxies (the one I named real_remote_ip), we get the real address of the remote machine, because this one has been inserted by the first trusted proxy in the chain, right? At this point, we don't care if this IP address is the one of the client machine itself or some intermediate proxy. In both cases is this the IP we want to block.
Comment 4 Frédéric Buclin 2012-02-19 05:01:20 PST
Created attachment 598653 [details] [diff] [review]
patch, v1
Comment 5 Soroush Dalili 2012-02-19 05:51:37 PST
I still think we need to have validation before Splitting the X-Forwarded-For header by the comma "," character, because:
1- It can lead to denial of service if someone manage to send a lot of "," characters in X-Forwarded-For header in many requests (because of the for loop).
2- Is there any reason that you want to cover all the characters' sets in X-Forwarded-For header? As far as I know, it should only contain 0-9\,\ \. and its length should be limited to the number of possible proxies*17; unless it can be set to anything by the trusted proxies (it is better to validate the format really - just be careful about the reDoS issues)

I should also say, as long as we do not have any CAPTCHA in here, this lockout policy can lead to a permanent DoS for the targeted users.
Comment 6 Frédéric Buclin 2012-02-19 05:57:47 PST
(In reply to Soroush Dalili from comment #5)
> 1- It can lead to denial of service if someone manage to send a lot of ","
> characters in X-Forwarded-For header in many requests (because of the for
> loop).

As we start from the end of the list, we will stop at the first external IP and ignore everything else. Your DoS won't work here. There is nothing bad you can do here which you couldn't do elsewhere.


> 2- Is there any reason that you want to cover all the characters' sets in
> X-Forwarded-For header?

We trust what our trusted proxy returns. We will never look at what external proxies or machines sent to us, so that's fine.


> I should also say, as long as we do not have any CAPTCHA in here, this
> lockout policy can lead to a permanent DoS for the targeted users.

As we lock accounts on a per IP address basis, only your IP will be block.
Comment 7 Reed Loden [:reed] (use needinfo?) 2012-02-19 08:44:22 PST
(In reply to Frédéric Buclin from comment #2)
> (In reply to Reed Loden [:reed] (very busy) from comment #1)
> > middleware.py has a fairly good Python-based way of doing validation.
> 
> But this validation won't help if the attacker is free to set the IP address
> he wants, right? He just has to pass a valid IP address to defeat this
> check. The equivalent of socket.inet_aton(ip) in Python is
> Socket::getaddrinfo() in Perl, but it doesn't check that this is really the
> IP address the request is coming from.

By "validation", I meant the whole process including the X-Forwarded-For header, not just checking the IP.

However, on that note, we should be validating the IP using something like Data::Validation::IP::is_ipv4() or Socket::getaddrinfo(). If anything in $ENV{'HTTP_X_FORWARDED_FOR'} is invalid, we should revert to using $ENV{'REMOTE_ADDR'}. Just because we trust our proxy servers doesn't mean we should trust them to always return correct data and do the right thing with validation. We should still validate ourselves ("Trust, but verify" policy).

Also, are we doing any type of validation on what goes into Bugzilla->params->{'inbound_proxies'}? If not, we probably should. A typo could be pretty bad.
Comment 8 Byron Jones ‹:glob› [PTO until 2017-01-09] 2012-02-19 21:48:17 PST
Comment on attachment 598653 [details] [diff] [review]
patch, v1

i agree with reed -- we should be validating the IPs in x-forwarded-for and inbound_proxies.

putting a dependency on Data::Validation::IP is an overkill, we should inline the is_ipv4 method from that module instead:

sub is_ipv4 {
    my $value = shift;
    return unless defined($value);
    my (@octets) = $value =~ /^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/;
    return unless @octets == 4;
    foreach my $part (@octets) {
        return unless $part >= 0 && $part <= 255 && $part !~ /^0\d{1,2}$/;
    }
    return 1;
}

aside from the lack of validation, your patch is fine.
Comment 9 Frédéric Buclin 2012-02-20 01:25:51 PST
I disagree. This would break IPv6 IP addresses.
Comment 10 Byron Jones ‹:glob› [PTO until 2017-01-09] 2012-02-20 01:33:21 PST
(In reply to Frédéric Buclin from comment #9)
> I disagree. This would break IPv6 IP addresses.

Data::Validation::IP has is_ipv6() which we could inline too.
Comment 11 Reed Loden [:reed] (use needinfo?) 2012-02-20 01:36:10 PST
(In reply to Frédéric Buclin from comment #9)
> I disagree. This would break IPv6 IP addresses.

This code already doesn't support IPv6 correctly (see the hardcoded "127.0.0.1"). However, it's just as simple to copy Data::Validation::IP::is_ipv6() as well.
Comment 12 Dave Miller [:justdave] (justdave@bugzilla.org) 2012-02-20 04:17:52 PST
Did someone recently change the account lockout code to how the X-Forwarded-For path?  This might be a b.m.o hack.  I know until recently it was listing the proxy server because we were getting lockouts from bzAPI people listing the bzAPI server as the IP.  The lockout notices I got for Soroush's attempts to test this listed the X-Forwarded-For path in the lockout notice (which is not good - should have only been listing the "real_remote_ip" as defined in comment 3.

FWIW, I almost locked out Soroush's account permanently (at least until he would get in contact with us to validate he was in control of his own account) based on these lockouts because a) it was obvious someone was trying to brute force his account, and b) he has filed several security bugs.  But then I saw this bug and realized he was just testing this bug. :)
Comment 13 Dave Miller [:justdave] (justdave@bugzilla.org) 2012-02-20 06:55:19 PST
OK, from discussion on IRC it looks like the (correct) code that was locally hacked on b.m.o in version 3.4 got incorrectly imported upstream in 3.6 (bug 527586), and has been broken since.  Not quite sure why we're only seeing this on b.m.o now when we've been on 4.0 for a while now.
Comment 14 Frédéric Buclin 2012-02-20 08:37:50 PST
(In reply to Reed Loden [:reed] (very busy) from comment #11)
> (In reply to Frédéric Buclin from comment #9)
> > I disagree. This would break IPv6 IP addresses.
> 
> This code already doesn't support IPv6 correctly (see the hardcoded
> "127.0.0.1").

The hardcoded 127.0.0.1 is fine, as we only use it if we got no remote IP address at all. Else we would use the IPv6 one set into $ENV{'REMOTE_ADDR'}.


(In reply to Dave Miller [:justdave] from comment #13)
> Not quite sure why we're only seeing this on b.m.o now when we've been on 4.0 for a while now.

Probably because there aren't so many attackers around trying to brute force passwords in b.m.o.
Comment 15 Reed Loden [:reed] (use needinfo?) 2012-02-20 09:56:24 PST
(In reply to Frédéric Buclin from comment #14)
> (In reply to Reed Loden [:reed] (very busy) from comment #11)
> > (In reply to Frédéric Buclin from comment #9)
> > > I disagree. This would break IPv6 IP addresses.
> > 
> > This code already doesn't support IPv6 correctly (see the hardcoded
> > "127.0.0.1").
> 
> The hardcoded 127.0.0.1 is fine, as we only use it if we got no remote IP
> address at all. Else we would use the IPv6 one set into $ENV{'REMOTE_ADDR'}.

... which would still be invalid for an IPv6-only box, as it should be ::1 then. ;)

> (In reply to Dave Miller [:justdave] from comment #13)
> > Not quite sure why we're only seeing this on b.m.o now when we've been on 4.0 for a while now.
> 
> Probably because there aren't so many attackers around trying to brute force
> passwords in b.m.o.

Keep in mind that until fairly recently, admin notifications for account lockouts were broken (bug 707594).
Comment 16 Byron Jones ‹:glob› [PTO until 2017-01-09] 2012-02-20 19:58:17 PST
(In reply to Reed Loden [:reed] (very busy) from comment #15)
> > The hardcoded 127.0.0.1 is fine, as we only use it if we got no remote IP
> > address at all. Else we would use the IPv6 one set into $ENV{'REMOTE_ADDR'}.
> 
> ... which would still be invalid for an IPv6-only box, as it should be ::1
> then. ;)

this should always work (however i don't have an ipv6-only box to test on):

> inet_ntoa(inet_aton("localhost"))
Comment 17 Dave Miller [:justdave] (justdave@bugzilla.org) 2012-02-20 22:34:14 PST
(In reply to Byron Jones ‹:glob› from comment #16)
> this should always work (however i don't have an ipv6-only box to test on):
> 
> > inet_ntoa(inet_aton("localhost"))

Not necessarily.  Most ipv6-capable boxes I've seen map ::1 to "localhost6" in the hosts file, and "localhost" still goes to 127.0.0.1.
Comment 18 Frédéric Buclin 2012-02-21 03:50:24 PST
(In reply to Dave Miller [:justdave] from comment #17)
> Not necessarily.  Most ipv6-capable boxes I've seen map ::1 to "localhost6"
> in the hosts file, and "localhost" still goes to 127.0.0.1.

We don't care about 127.0.0.1 vs ::1. In both cases, we didn't get the remote IP address and so we fall back to some default. We could fall back to "unknown" that this wouldn't change the story.
Comment 19 Frédéric Buclin 2012-03-26 09:34:55 PDT
Created attachment 609352 [details] [diff] [review]
patch for 4.0, v2

Now with validators. Note that I have no proxy to play with.
Comment 20 Byron Jones ‹:glob› [PTO until 2017-01-09] 2012-03-27 08:27:37 PDT
Comment on attachment 609352 [details] [diff] [review]
patch for 4.0, v2

r=glob

this looks good to me.
Comment 21 Frédéric Buclin 2012-03-27 08:33:05 PDT
Thanks! Let's take it for the next releases.
Comment 22 Soroush Dalili 2012-03-27 10:43:42 PDT
Thanks for the fix. Could you also please let me know if it can be eligible for the bug bounty?
Comment 23 Michael Coates [:mcoates] (acct no longer active) 2012-04-02 15:52:37 PDT
(In reply to Soroush Dalili from comment #22)
> Thanks for the fix. Could you also please let me know if it can be eligible
> for the bug bounty?

We'll review this during our next bounty review sessions and provide feedback.

Thanks.
Comment 26 Daniel Veditz [:dveditz] 2012-04-09 15:35:23 PDT
(In reply to Soroush Dalili from comment #22)
> Thanks for the fix. Could you also please let me know if it can be eligible
> for the bug bounty?

We've decided this is eligible for a bounty of $1000. You should be hearing from Chris (or one of us) soon.
Comment 27 Frédéric Buclin 2012-04-12 11:53:30 PDT
Created attachment 614477 [details] [diff] [review]
patch for 3.6, v1

This is the same patch as for other branches. I simply had to fix bitrot in the EXPORT list of Bugzilla::Util. So it doesn't need another review.
Comment 28 Frédéric Buclin 2012-04-18 09:38:24 PDT
It's release time.
Comment 29 Frédéric Buclin 2012-04-18 09:50:04 PDT
Created attachment 616179 [details] [diff] [review]
patch for 4.2 + trunk, v2.1

Fix a slight bitrot in the EXPORT list of Bugzilla::Util due to another patch which has been checked in meanwhile. This patch is for 4.2 + trunk.
Comment 30 Frédéric Buclin 2012-04-18 09:55:05 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Util.pm
modified Bugzilla/Config/Advanced.pm
modified Bugzilla/Config/Common.pm
Committed revision 8206.


Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.2/
modified Bugzilla/Util.pm
modified Bugzilla/Config/Advanced.pm
modified Bugzilla/Config/Common.pm
Committed revision 8080.


Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/Util.pm
modified Bugzilla/Config/Advanced.pm
modified Bugzilla/Config/Common.pm
Committed revision 7705.


Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.6/
modified Bugzilla/Util.pm
modified Bugzilla/Config/Advanced.pm
modified Bugzilla/Config/Common.pm
Committed revision 7285.
Comment 31 Frédéric Buclin 2012-04-18 15:40:07 PDT
Security advisory sent.
Comment 32 Soroush Dalili 2012-04-18 15:45:52 PDT
Thank you very much for the fix and the advisory.

Note You need to log in before you can comment on or make changes to this bug.