392575 - Not restricting session to single IP address fails with IPv6

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Magnus Holmgren]

User-Agent:       Opera/9.22 (X11; Linux i686; U; en)
Build Identifier: 3.0

The solution to bug 20122 is inadequate in an IPv6 world. The problem is that Bugzilla::Utils::get_netaddr() barfs on non-IPv4 addresses, returning undef, which leads to an ugly error.

Reproducible: Always

Steps to Reproduce:
0. Install Bugzilla on a server with an IPv6 interface.
1. Set the loginnetmask parameter to < 32, enabling the feature. Make sure that the rememberlogin parameter is not set to "off".
2. Access Bugzilla from a computer with an IPv6 interface.
3. On the login page, check "Remember my Login" (if rememberlogin is set to "defaultoff") and uncheck "Restrict this session to this IP address".
4. Enter any valid username and password and try to login.
Actual Results:  
Undef to trick_taint at Bugzilla/Util.pm line 67
	Bugzilla::Util::trick_taint('undef') called at Bugzilla/Auth/Persist/Cookie.pm line 61
	Bugzilla::Auth::Persist::Cookie::persist_login('Bugzilla::Auth::Persist::Cookie=ARRAY(0x1910030)', 'Bugzilla::User=HASH(0x1a28490)') called at Bugzilla/Auth.pm line 147
	Bugzilla::Auth::_handle_login_result('Bugzilla::Auth=ARRAY(0x18d9970)', 'HASH(0x1a280e0)', 2) called at Bugzilla/Auth.pm line 92
	Bugzilla::Auth::login('Bugzilla::Auth=ARRAY(0x18d9970)', 2) called at Bugzilla.pm line 218
	Bugzilla::login('Bugzilla', 2) called at /srv/bugzilla/editparams.cgi line 38

Expected Results:  
Bugzilla should do something useful. Preferably there should be a separate loginnetmask6 applied to IPv6 addresses. As an alternative, a formula such as loginnetmask*2+16 could be used (a fixed netmask for all users is a very coarse-grained instrument anyway), but in the long term IPv6 will be used exclusively, and then the netmask must be in relation to IPv6 addresses. As a temporary solution, Bugzilla can let the login cookie be valid for the entire IPv6 address space, but at the very least it should not crash.

Comment 1

[Magnus Holmgren]

Here is a conceptual reimplementation of Bugzilla::Utils::get_netaddr() using NetAddr::IP ("use" statement sold separately). I'd prefered not adding another dependency, but IPv6 addresses are complex enough that I think it's best to outsource their handling to an expert module.

sub get_netaddr {
    my $ipaddr = new NetAddr::IP(shift);
    return unless defined $ipaddr;

    my $maskbits = Bugzilla->params->{'loginnetmask'};

    if ($ipaddr->version == 6) {
        $maskbits = 2 * $maskbits + 16;
    }

    return lc new NetAddr::IP($ipaddr->addr, $maskbits)->network->short;
}

Comment 2

[Frédéric Buclin]

Dave, do you know something about IPv6? :)

Comment 3

[Max Kanat-Alexander]

I think timeless already filed/fixed this.

Comment 4

[Marc Blanchet]

I have the exact same issue: login does not work over IPv6. either it just comes back to the login page (when accessing from the home page) or it shoots the following error message (when accessing from the /?GoAheadAndLogIn=1):

Undef to trick_taint at Bugzilla/Util.pm line 67
	Bugzilla::Util::trick_taint('undef') called at Bugzilla/Auth/Persist/Cookie.pm line 61
	Bugzilla::Auth::Persist::Cookie::persist_login('Bugzilla::Auth::Persist::Cookie=ARRAY(0x99cae9c)', 'Bugzilla::User=HASH(0x9a7bbd0)') called at Bugzilla/Auth.pm line 147
	Bugzilla::Auth::_handle_login_result('Bugzilla::Auth=ARRAY(0x99a2f70)', 'HASH(0x9a7b900)', 2) called at Bugzilla/Auth.pm line 92
	Bugzilla::Auth::login('Bugzilla::Auth=ARRAY(0x99a2f70)', 2) called at Bugzilla.pm line 218
	Bugzilla::login('Bugzilla', 0) called at /usr/share/bugzilla/index.cgi line 40

bugzilla 3.0


this makes bugzilla 3.0 not working over IPv6... really sad.

Comment 5

[A. Shimono [:himorin]]

(In reply to comment #3)
> I think timeless already filed/fixed this.

We ignore IPv6 netaddress when we applying loginnetmask. (return undef for them)
Bugzilla::Auth::Login::Cookie module does well about this, but i think Bugzilla::Auth::Persist::Cookie doesn't.

I think this is A BUG.

Comment 6

[last1]

Attachment: Spam

Comment 7

[Dave Miller [:justdave]]

The content of attachment 328579 [details] has been deleted by
    Dave Miller [:justdave] <[e-mail]>
who provided the following reason:

drug spam

The token used to delete this attachment was generated at 2008-07-08 18:12:46 PDT.

Comment 9

[Gervase Markham [:gerv]]

I've just hit this on Ubuntu 9.04. It's a bug, and it's not fixed. get_netaddr() in Util.pm copes badly with what my system returns for $cgi->remote_addr (passed by persist_login in Cookie.pm), which is the string "::1". This breaks logging in from the header and footer, although not for some reason from GoAheadAndLogIn=1. (Is that symptomatic of a bug that they are not doing the same thing with IP restrictions?)

Gerv

Comment 10

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Here's a patch using the code from comment #1, which solves the problem for me. Of course, it adds a new dependency. I guess it could be rewritten to make it optional and use the old code if not present, but that sounds like a recipe for admins being surprised by broken systems...

Gerv

Comment 11

[Frédéric Buclin]

Isn't bug 399073 going to fix this bug automatically, which means we won't need a new dependency?

Comment 12

[Gervase Markham [:gerv]]

I guess it would, although in the last two years there doesn't seem to have been much activity in fixing that bug :-)

Why did we decide it always had to be 0? Is it too risky to set it to a higher value?

Gerv

Comment 13

[Frédéric Buclin]

(In reply to comment #12)
> I guess it would, although in the last two years there doesn't seem to have
> been much activity in fixing that bug :-)

Because it's not a top priority bug. But if you want to fix the problem with IPv6, you should rather focus on bug 399073 rather than this one.


> Why did we decide it always had to be 0? Is it too risky to set it to a higher
> value?

The reason to remove this param and set it to 0 in the code was discussed at our meeting on September 4, 2007:

http://bugzilla.glob.com.au/irc/?c=bugzilla-meeting&a=date&s=4+September+2007&e=4+September+2007&h=loginnetmask

(look for loginnetmask).

Since Bugzilla 2.22, we use random tokens for logincookies rather than autoincremented integers, see bug 119524. This makes it impossible for an attacker to guess what your logincookie is and so restricting you access from a given IP or pool of IPs is no longer required. This means you can keep your logincookie valid even when your IP address changed.

Comment 14

[Frédéric Buclin]

Comment on attachment 402312 [details] [diff] [review]
Patch v.1

Per my previous comment, removing the loginnetmask parameter will fix this problem.

Comment 15

[Frédéric Buclin]

Fixed in Bugzilla 3.5.1.

Comment 17

[Max Kanat-Alexander]

FWIW, if somebody wants to fix this in 3.4, I'd take a patch.

Comment 18

[Gervase Markham [:gerv]]

Max: what sort of solution would you take? One which removes loginnetmask? Or one which adds an extra dependency on IP::Addr or a similar module? Or some other sort of solution?

Gerv

Comment 19

[Max Kanat-Alexander]

  Here's how I think a branch fix would work:

  If there's a loginnetmask set in such a way that this bug would be triggered, then any IPv6 address that we receive should cause the cookie to be explicitly locked down to just that address, skipping any netmask checking.