Closed Bug 728639 (CVE-2012-0465) Opened 12 years ago Closed 12 years ago

[SECURITY] User lockout policy can be bypassed by altering the X-FORWARDED-FOR header

Categories

(Bugzilla :: User Accounts, defect)

3.5.3
defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 3.6

People

(Reporter: soroush.dalili, Assigned: LpSolit)

References

Details

(Whiteboard: [infrasec:auth][ws:moderate])

Attachments

(3 files, 1 obsolete file)

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.
Assignee: general → user-accounts
Component: Bugzilla-General → User Accounts
https://github.com/jsocol/commonware/blob/master/commonware/request/middleware.py has a fairly good Python-based way of doing validation.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Target Milestone: --- → Bugzilla 3.6
Version: 4.0.4 → 3.5.3
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.
Attached patch patch, v1 (obsolete) — Splinter Review
Assignee: user-accounts → LpSolit
Status: NEW → ASSIGNED
Attachment #598653 - Flags: review?(glob)
Attachment #598653 - Flags: review?(dkl)
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.
(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.
(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.
Severity: normal → major
Summary: User lockout policy can be bypassed by using X-FORWARDED-FOR header → [SECURITY] User lockout policy can be bypassed by using X-FORWARDED-FOR header
Whiteboard: [infrasec:auth][ws:moderate]
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.
Attachment #598653 - Flags: review?(glob) → review-
I disagree. This would break IPv6 IP addresses.
(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.
(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.
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. :)
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.
(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.
(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).
(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"))
(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.
(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.
Blocks: 729001
Now with validators. Note that I have no proxy to play with.
Attachment #598653 - Attachment is obsolete: true
Attachment #609352 - Flags: review?(glob)
Attachment #609352 - Flags: review?(dkl)
Attachment #598653 - Flags: review?(dkl)
Comment on attachment 609352 [details] [diff] [review]
patch for 4.0, v2

r=glob

this looks good to me.
Attachment #609352 - Flags: review?(glob) → review+
Flags: approval?
Thanks! Let's take it for the next releases.
Flags: blocking4.2.1+
Flags: blocking4.0.6+
Flags: blocking3.6.9+
Flags: approval4.2?
Flags: approval4.0?
Thanks for the fix. Could you also please let me know if it can be eligible for the bug bounty?
Attachment #609352 - Flags: review?(dkl)
Blocks: 741079
(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.
(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.
Alias: CVE-2012-0465
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.
Attachment #614477 - Flags: review+
Flags: approval3.6?
It's release time.
Flags: approval?
Flags: approval4.2?
Flags: approval4.2+
Flags: approval4.0?
Flags: approval4.0+
Flags: approval3.6?
Flags: approval3.6+
Flags: approval+
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.
Attachment #616179 - Flags: review+
Attachment #609352 - Attachment description: patch for 4.0 - 4.3, v2 → patch for 4.0, v2
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Summary: [SECURITY] User lockout policy can be bypassed by using X-FORWARDED-FOR header → [SECURITY] User lockout policy can be bypassed by altering the X-FORWARDED-FOR header
Security advisory sent.
Group: bugzilla-security
Thank you very much for the fix and the advisory.
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.