20122 - Bugzilla requires new login if IP changes

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bugzilla requires that I login again every time my hostname changes, even if I'm
on the same computer.  This is very annoying when on a dialup (and people on a
LAN don't notice the problem).

Could this be changed?  The cause seems to be in quietly_check_login in
http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/CGI.pl , where SendSQL
is called.

Comment 1

[Terry Weissman]

It's a security tradeoff.  If I don't do this, then it's much easier to spoof
someone's login.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK.  I suspected that was the reason...

Comment 3

[Hixie (not reading bugmail)]

FWIW, I have the same problem because my local www cache is actually two
machines, and so I keep switching which machine bugzilla thinks I am logged in.
Of course, the other problem is that anyone can spoof a cookie with my username
in it and send that to bugzilla, and if they are lucky (that is, if they get
the same proxy server as I had last time I logged in) then they will be able to
do one thing as if they were me.

David: see bug 3641. If you know of a better way of doing things, please tell
me, as I have the same problem with my own authenticating scripts.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ian - wrong bug#.  Also, if you want to make it a *lot* harder to spoof a
cookie, have 2 cookies:
 * your username
 * a GPG (or PGP) signature on your username

I think that's what you were asking.

Comment 5

[Hixie (not reading bugmail)]

Oops, bug 3614! Sorry.

So the second cookie would include a PGP-signed version of the username, signed
by the server? So my server would need to have a private key that it could
use to sign the username. Hmm. Unfortunately, my server is not (read) secure so
I can't do that. If my script can read the private key, then anyone can read
the private key.

Interesting idea though. If checking a PGP-signature is quick, then it might
work. (Don't forget this signature would need to be checked everytime a page
is accessed!)

Comment 6

[Terry Weissman]

*** Bug 18720 has been marked as a duplicate of this bug. ***

Comment 7

[Ben Bucksch (:BenB)]

I have the same problem with my autodialup/-down set at 1 min.

Comment 8

[Gervase Markham [:gerv]]

*** Bug 33232 has been marked as a duplicate of this bug. ***

Comment 9

[Ben Bucksch (:BenB)]

Sourceforge (after a similar bug) checks only for a B class match (i.e. the
first 2 bytes have to be the same). That's less secure, but will fix this bug.

If anybody can catch a cookie, can't he/she also catch the password?

Comment 10

[Terry Weissman]

[e-mail] is the new owner of Bugzilla and Bonsai.  (For details,
see my posting in netscape.public.mozilla.webtools,
news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)

Comment 11

[Ben Bucksch (:BenB)]

I'm really tired of this bug.

Comment 12

[Gervase Markham [:gerv]]

[e-mail], and anyone else with this problem - go to 
http://www.typingmaster.com and download a free utility called QuickPhrase. 
Then, bind e.g. F12 to e.g.
{TAB}{TAB}{TAB}{TAB}[e-mail]{TAB}password{TAB}

Problem nearly solved :-) This is what I used for ages.

Gerv

Comment 13

[Ben Bucksch (:BenB)]

> QuickPhrase

I'm using Unix.

> Problem nearly solved

Yes, but still
- one extra page load
- one extra button press and one extra mouse click
- some fuctions won't work, e.g. attaching or changing multiple bugs at once.

All this for nearly *each and every change* in bugzilla.

Comment 14

[Not Gerv's Account - use [e-mail]]

Are there no text insertion utils for Unix? I thought it was big on that sort of
stuff.

FWIW, I solved this problem for myself (caused by the UK JANET National Web
Cache using a round robin of machines) by getting our site admins to put
bugzilla.mozilla.org on the exceptions list. Would that work for you? You didn't
mention what part of your setup causes your IP to change so often, but I assume
it's a web cache.

Would it also be possible to enable bugzilla access on port 81 of
bugzilla.mozilla.org as well as port 80? That might help people.

Gerv

Comment 15

[Ben Bucksch (:BenB)]

> Are there no text insertion utils for Unix?

If really tried to (i.e. spend a day look for apps, Xresoucrs etc.), I'd
propably find a solution. It wouldn't solve the problems I mentioned above.

But I don't want a workaround on the client side. There're so many people having
this problem, that it should be fixed on bugzilla's side. Soon.

There is an easy workaround on Bugzilla's side (check only for B class match).

> You didn't mention what part of your setup causes your IP to change so often

> ------- Additional Comments From Ben Bucksch 2000-02-18 12:55 -------
> I have the same problem with my autodialup/-down set at 1 min.

I'm on a dialup line.

Comment 16

[Stephan Niemz]

    Why don't you just use the HTTP Authentication mechanism? Is there some
reason I don't see yet?

    That way the browser would pop up *once* with a window asking for a
username and password, but would submit it each time the server asks for
it, without any further user interaction. The browser remembers that info
until I close it, no matter what IP I have or what web cache I use. Many
sites work this way, and I think that's much more convenient.

    By the way, I just wanted to vote for this bug, but Bugzilla didn't let
me, saying "wrong login", although I was logged in and made sure not to
hang up the line inbetween. But that's probably a different bug.

    By the way again, I think the platform and OS field is wrong and should
be all/all, but I'm obviously not allowed to change it.

Comment 17

[Stephan Niemz]

I have a workaround for this bug: bookmark the following URL

http://bugzilla.mozilla.org/showvotes.cgi?Bugzilla_login=YOUR@EMAIL&Bugzilla_password=YOURPASSWORD

    and replace YOUR@EMAIL and YOURPASSWORD with the appropriate values.
Every time you connect to your ISP, load that URL in a new browser window.
As soon as some page content starts appearing, you can press "commit" in
the other window where you want to make some changes.

    You could also use query.cgi instead of showvotes, but this conserves a
lot more bandwidth.

    Of course this is insecure, since your password will be stored in
plaintext on your hard drive. Maybe you should consider this before you do
it on a machine where others have root access. :-)

    And of course it doesn't solve this bug at all. I'd still be really
glad if someone would fix it!

Comment 18

[Dave Miller [:justdave]]

The problem with using HTTP authentication is that it requires the cooperation of 
the web server on the machine that is hosting Bugzilla.  You will need to teach 
the web server how to read your bugzilla user database.  This is not always easy, 
and would be a major setup chore for anyone installing Bugzilla, and would 
probably also limit which web servers you could use it with.

Comment 19

[Hixie (not reading bugmail)]

Stephan: It's insecure anyway, as your password is sent in plaintext across
the internet every time you log on...

Dave: Couldn't the script take care of the authentication part itself? Why
require the webserver to do any work?

Comment 20

[Dave Miller [:justdave]]

>Couldn't the script take care of the authentication part itself? Why
>require the webserver to do any work?

That's the problem, the script *is* doing the authentication part NOW.  People 
don't like that because it requires cookies.  The CGI protocol only passes the 
authenticated username to the script, it doesn't pass along their password.  So 
the script has no idea what password they used, if they use the standard HTTP 
authentication.  The suggestion was to use the standard HTTP authentication.  If 
the server is doing this authentication, the script can easily tell who's logged 
in.

Although the script could certainly issue the authentication challenge, the 
response from the user's browser would get intercepted by the server on the way 
back, and the script would never see it.

I do have an idea though...  instead of teaching the web server how to read 
bugzilla's user database, how about teaching bugzilla to maintain standard 
.htaccess and htpasswd files as well as its own database?

That, again, will have additional compatibility problems, as not all webservers 
use .htaccess for authentication information.

Comment 21

[Ben Bucksch (:BenB)]

Assuming it would work with Apache, I'd value the user's problems over the
choices of the admin (but I am biased :) ).

Comment 22

[Dave Miller [:justdave]]

Yeah, it would work for Apache....

speaking of Apache...  I seem to recall mentioning above about teaching a web 
server how to read Bugzilla's user database...  well, someone already did it.  
Not to work with Bugzilla specifically, but with mySQL in general, and you can 
give it whatever database, table, and column you want for the usernames and 
passwords, so you could easily point it at the profiles table in the bugs 
database. :)

Look here: http://bourbon.netvision.net.il/mysql/mod_auth_mysql/

Comment 23

[Hixie (not reading bugmail)]

It turns out Apache doesn't steal the "Authorization:" line and prevent the
script from knowing it was given some credentials -- it puts the relevant
line into the environment variable HTTP_AUTHORIZATION.

So what Bugzilla needs to do instead of sending the login page with a form is
reply with

    HTTP/1.1 401 Unauthorized
    WWW-Authenticate: Basic realm="Bugzilla"

...and follow that with a page providing a button to send/create a password.

The browser with respond to this and the following code parses the reply:

   use MIME::Base64;
   my($user, $pass);

   # Did client provide credentials?
   if ($ENV{'HTTP_AUTHORIZATION'} =~ /^Basic (.*)$/os) {
       ($user, $pass) = split(/:/, decode_base64($1), 2);
   }

   # Can user log in?
   # The "authenticate" function should return true if $user has password
   # $password and false otherwise. It should return false if $user is
   # blank, too.
   if (&authenticate($user, $password)) {
       print "HTTP/1.1 200 OK\n";
   } else {
       print "HTTP/1.1 401 Unauthorized\n";
       print "WWW-Authenticate: Basic realm=\"Bugzilla\"\n";
   }

That should work.

Comment 24

[rbs]

I suspect I have troubles attaching files to bug reports because of this bug...
When I try attaching a file to a bug report, I am prompted for my password over
and over again. Each time, 'create attachment' fails with the error message
that no attachment was supplied.

Comment 25

[Ben Bucksch (:BenB)]

rbs, right. You must not change your hostname between loading the attachment
page and commiting/uploading. You're behind a proxy? Out of luck :(. (I don't
know any workaround. Somebody else?)

BTW: I like Ian's proposal. Can somebody get this to a point where it is ready
to be checked into the tree and activated on mozilla.org, please?
BTW2: I happen to have a new ISP plan, which makes this bug much less bad for
me.

Comment 26

[Gervase Markham [:gerv]]

*** Bug 38831 has been marked as a duplicate of this bug. ***

Comment 27

[Dave Miller [:justdave]]

I spent a couple hours today experimenting with Ian's suggestion that the script 
should still get the HTTP_AUTHORIZATION environment variable.  I now have a copy 
of CGI.pl which would theoretically handles all of its authentication this way.

After my experiments, I'll stand on my original statement that the information 
never gets passed to the CGI script.  Apache 1.3.5 eats all the authorization 
stuff, and it never makes it anywhere the script can see it.  I can successfully 
return 401 Unauthorized headers that trick the browser into authenticating, and 
by logging headers on the server, I can see that the browser is indeed sending an 
Authentication: header, but when logging the environment variables avaialble to 
the script, there's nothing that even remotely sounds like it showing up in %ENV.

At least, this is the case with Apache 1.3.5 on Linux...

Comment 28

[Andreas Franke (gone)]

Quoted from http://hoohoo.ncsa.uiuc.edu/cgi/env.html:
[...] In addition to these, the header lines received from the client, if any,
are placed into the environment with the prefix HTTP_ followed by the header
name. [...] The server may exclude any headers which it has already processed,
such as Authorization [...].

Comment 29

[Hixie (not reading bugmail)]

Hmm. It works for me!

My proof of concept script is available online:
Source: 
   http://www.bath.ac.uk/%7Epy8ieh/cgi-source/development/tests/nph-auth_basic
Executable:
   http://www.bath.ac.uk/%7Epy8ieh/cgi/development/tests/nph-auth_basic

Is there some way to tell Apache to not bother authenticating? (There must be,
I assume...)

Comment 30

[Dave Miller [:justdave]]

It must be the 'nph-*' that lets it through.  In order for that to work, we'd 
have to rename every script in here to nph-....  that'd be a pain in the neck.

I spent most of a day last week searching Apache's docs for a way to get it to 
not bother authenticating, and I couldn't find a way to do it without disabling 
it site-wide, and I have other parts of my site that use it the real way.

Sounds like the nph- trick would do it.  We could get around the filenaming thing 
as far as the users are concerned by using RewriteRule lines in the .htaccess 
file, assuming people have mod_rewrite installed.  Note that if we get into nph- 
files, then all of the CGI scripts will be required to emit full headers, and not 
just the Content-Type header...

Sounds like almost as much of a nightmare (though not quite as bad) as using 
session keys...

Comment 31

[Dave Miller [:justdave]]

More thoughts....   I did find an Anonymous_UserID option for Apache that will 
allow you to let people into an authenticated area by just hitting the OK button 
on the password dialog without entering a UserID or a password.  However, if they 
do supply a username and password, it authenticates it to make sure it's them.  
Since the script could easily tell this by whether REMOTE_USER was blank or not, 
perhaps this would be the way to go.  This would necessitate placing the 
index.html file in a different directory, and explaining on the index page that 
you can just hit the OK button when prompted for a password when you go into the 
other pages (although I suppose you could mask the index.html file out of the 
authenticated zone with creative use of the .htaccess file)

Combine this with mod_auth_mysql in order to yank the passwords out of the 
existing bugzilla user database, and we have a working system.

Comment 32

[Hixie (not reading bugmail)]

The 'nph' prefix is because we use CGIWrap and is just because that is what
our RewriteRule for CGIWrap expects. It could be anything.

Comment 33

[Hixie (not reading bugmail)]

*** Bug 3614 has been marked as a duplicate of this bug. ***

Comment 34

[Hixie (not reading bugmail)]

*** Bug 31427 has been marked as a duplicate of this bug. ***

Comment 35

[Dave Miller [:justdave]]

ok, how do you go about changing what prefix it uses?  Or make it use a 
siffix instead?  And can you do that only in the bugzilla directory, or would it 
have to be done for the whole server?  (i.e. if we told it that any script in 
the bugzilla directory should be treated as an nph script or soemthing)

Comment 36

[Hixie (not reading bugmail)]

Unless you use CGIwrap, then there should be nothing to worry about.

Comment 37

[Stephan Niemz]

    This is still bugging me heavily. See my latest comments to bug 20122
and bug 36358 for an example. Are there any chances that this is going to
be fixed some day?

Comment 38

[Stephan Niemz]

There is another thing I'm wondering about ... as I always log in from different
IP numbers, the bugzilla database must have tons of them stored for my user ID.
And if I click on "log out", I seem to only log out the current IP number. So
anybody who is using the same dialup ISP and who could grab my user ID and
password could make changes to Bugzilla on my behalf. This doesn't give me a
feeling of great security!

And it is still very annoying that I always have to load the voting page in
a different window for a workaround as I wrote in my 2000-06-15 comment.

Could someone *pleeeeeease* fix this? There have been solutions suggested here.

Comment 39

[Ben Bucksch (:BenB)]

I don't think, bugzilla stores the IP-numbers. They are stored in client cookies
and there is only one (i.e. all previous ones are forgotten).

Comment 40

[Dave Miller [:justdave]]

The IP number is not stored in the cookie.  If it were, it would be real easy to 
edit it. :)   The IP number *is* stored on the server for the IP that it sent 
each cookie to.  It only stores one IP per login ID.  As soon as you log in at a 
new IP, it forgets any other IPs you've logged in from.

For someone else to spoof you, three things have to happen:

1) They have to sniff your cookie and steal it.
2) They have to get the same IP address you were previously using
3) You would have to have not chosen the "Log Out" link in Bugzilla before
   logging out of that IP address.

Comment 41

[Hixie (not reading bugmail)]

or
4) they can just sniff your network for your password when you next log in (since
   the password is sent in clear text).

Comment 42

[Dave Miller [:justdave]]

*** Bug 69955 has been marked as a duplicate of this bug. ***

Comment 43

[Dave Miller [:justdave]]

updating summary to reflect where the discussion on this bug was headed, and make 
it easier for people to find it.

Comment 44

[Stephan Niemz]

Adding default QA contact to all open Webtools/Bugzilla bugs lacking one.
Sorry for the spam.

Comment 45

[Jacob Steenhagen]

*** Bug 80963 has been marked as a duplicate of this bug. ***

Comment 46

[John E. Jasen]

I did not notice this problem with redhat's bugzilla 2.8. My guess would be
either something introduced since mozilla's and redhat's bugzilla diverged, or
RH found a way around it.

is there any hope of a resolution?

Comment 47

[Jacob Steenhagen]

2.8 was Released 11/19/99 and this bug was opened 11/26/99 so I'd say it's a
safe bet that this was an issue when RedHat forked :) My best guess is that
RedHat went for the security tradeoff that makes it easier to spoof somebody's
login.

Comment 48

[John E. Jasen]

Then perhaps it could be made a configuration option in the next release or so
of bugzilla, where the default is what it is now, but it can be made more lax,
so that people can make their own decisions as to which way to go on this?

Comment 49

[Dave Miller [:justdave]]

since there's people that have this problem on bugzilla.mozilla.org, why not make 
it an individual use preference?  Since people are responsible for their own 
accounts, if they want to take that risk, why stop them?

With the looser checks, you could still valid the IP address, but maybe just 
validate it against the subnet instead of the full IP.  I'd assume that the 
round-robin proxies are usually on the same subnet with each other, aren't they?

Comment 50

[Jacob Steenhagen]

-> New Bugzilla Product

Comment 51

[Dave Miller [:justdave]]

*** Bug 104520 has been marked as a duplicate of this bug. ***

Comment 52

[Gervase Markham [:gerv]]

I don't think we need this for 2.16. Anyone care enough to write a patch?

Gerv

Comment 53

[Matthew Tuck [:CodeMachine]]

Absolutely.  To me, this is our most needed feature.

Comment 54

[Matthew Tuck [:CodeMachine]]

Taking this.  I have a patch that allows you to specify an IP mask, but it
requires installation of the Net:IPv4Addr module and I'm still waiting for it to
be installed on landfill.

Comment 55

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

http://www.stonehenge.com/merlyn/WebTechniques/col61.html

Comment 56

[Dave Miller [:justdave]]

There's nothing in the works on this yet, and it'll likely require more review
than we have time for between now and 2.16, so I'm pushing to 2.18

Comment 57

[Matthew Tuck [:CodeMachine]]

Attachment: Rough, likely bitrotten patch.

bbaetz wanted this attached.

In particular, he indicated this stuff should perhaps be fixed to always use
IPs, which I thought it always did, but apparently not, so this patch might
break then.

Also it doesn't support IPv6 at all.  The current model of specifying 24 bits,
ie a mask of 255.255.255.0 would not work for IPv6.  Perhaps it should be
changed to specify the 8 bits that are a part of the network.

Comment 58

[Bradley Baetz (:bbaetz)]

Comment on attachment 72962 [details] [diff] [review]
Rough, likely bitrotten patch.

The check in globals.pl won't match if there is more than one entry for the
user in teh logincookies table, since you have removed teh where. I was just
going to store the network address, and do an update in userprefs to keep the
current login session working.

We can also probably avoid the dependance on another module unless we want to
handle ipv6

Comment 59

[Bradley Baetz (:bbaetz)]

I'm going to attach a patch which works. It requires Net::IPv4 - if thats likely
to be a problem I can probably redo the required function myself without too
much trouble.

I'm not very happy with the way the userprefs screen looks. Matty's patch moved
it to a separate screen, but I don't think a screen with one option is worth it.
I'll take any improvments on this (which is why this patch is v0.9, not v1.0)

Resetting to 2.16 for consideration, since it is a Frequently Requested Feature.
It is very late for this though, so feel free to bump it.

Comment 60

[Bradley Baetz (:bbaetz)]

Attachment: patch v0.9

Comment 61

[Bradley Baetz (:bbaetz)]

And my comment, which seems to have been misplaced:

So, here we go. We now store the network address in the logincookies table.
Since there isn't an sql function for doing this, this means that my query will
return only one value. I had to split up the query a bit to do this, since I
need the profiles table before I check the logincookies table.

Theres a param for the minimum value for the netmask, defaulting to 24. Setting
to 32 hides this option. Setting this value updates the profiles table with the
new info - should it instead keep it until the user changes it, and then CGI.pl
would just take the min of the param and the value from the db?

Also, I had to comment out the eval in get_netaddr, else I got syntax errors.
Any ideas? It is needed for dealing with addresses Net::IPv4 can't parse.

Comment 62

[Dave Miller [:justdave]]

Comment on attachment 74508 [details] [diff] [review]
patch v0.9

>Index: CGI.pl

>+sub get_netaddr($$) {
>+    my ($addr, $mask) = (@_);
>+
>+    use Net::IPv4Addr qw(ipv4_network);
>+    
>+    my $ipaddr;
>+    #eval {
>+        ($ipaddr) = ipv4_network($addr, $mask);
>+    #}
>+    # Some addresses (ie ipv6) won't work. Thats ok - we'll
>+    # just ignore the user's netmask in that case and match on
>+    # the entire string
>+    # Warn to help track down issues with this
>+    if ($@) {
>+        warn $@;
>+        $ipaddr = $addr if $@;
>+    }
>+    return $ipaddr;
>+}

Just to fill me in here...  we have the eval commented out....
does ipv4_network() set $@ when it fails anyway?  If so, this is going to
confuse people (like me) to see that partially commented out.  Let's just
get rid of it if that's the case.

Comment 63

[Bradley Baetz (:bbaetz)]

BEcause it didn't compile - see my comments.

Hixie pointed out that I was missing a ; at the end of that block. Adding that
makes it work correctly. Without the eval, it will throw an exception, and die,
on invalid input.

Comment 64

[Hixie (not reading bugmail)]

I also reminded bbaetz about my suggestion earlier in this bug of using HTTP
auth. That is, after all, the whole point of HTTP auth.

Comment 65

[Dave Miller [:justdave]]

Making it use HTTP authentication will be a humungous amount of work (about on
order with the Templatization patches with how much it'll change the face of the
cvs repository).  See comment #30.

Comment 66

[Bradley Baetz (:bbaetz)]

And I reminded hixie that while you can use a modrewrite rule to get the
password sent anyway (which he pointed out to me - really cool; I didn't know
you could do that), that won't work for IIS. And you'd be sending the password
in plain text on every page load. Although we do that currently anyway, so...

Comment 67

[Dave Miller [:justdave]]

except we're not sending it in cleartext on every page load.  We're sending a
session cookie with every page load that is tied to the IP address (or a subnet
with this most recent patch).  The password is only sent cleartext when you
actually log in.

Comment 68

[Hixie (not reading bugmail)]

And I pointed out that this just gave us a false sense of security. There is
nothing secure about our current system, as we all know. Given that it is
completely insecure, I see no reason to not use another completely insecure but
at least more correct solution.

And if anyone develops a well-supported and secure alternative to HTTP Basic or
HTTP Digest, then we can use it.

Comment 69

[Matthew Tuck [:CodeMachine]]

Saying "the fact that the password only appears 1/20th as much gives us a false
sense of security so we shouldn't do this" is kind of like saying "the fact that
a password makes it harder to login gives us a false sense of security because a
password can be guessed so we shouldn't use them".

Comment 70

[Hixie (not reading bugmail)]

Passwords increase the security by factors of millions.

Comment 71

[Matthew Tuck [:CodeMachine]]

The factor isn't the issue here.  The point is that security is achieved through
multiple layers, each decreasing the likelihood of a break in.  Whether they
decrease it a little or a lot is really not relevant.  The only thing that
should prevent something that increases security being done is if it prevents
something else that increases it more.

I've heard this "false sense of security" argument before and it can be used to
nix just about anything if you want it to.

Comment 72

[Hogarth]

Ok. This is the most relevant place that I have found for this so here goes...

PLEASE. For the love of god. Fix this. If there's no patch that people want to
use then can we at least have bugzilla.mozilla.org respond on a port other then
80 (like 81) aswell as port 80 so that those of us stuck behind a rotating proxy
can use it without the frustration of having to log in each and every time we
make some kind of change to a bug.

And yes I do appreciate the security behind this. I just want to be able to use
bugzilla in a happy mood rather then in a frustrated mood that makes me want to
use it as little as possible. :/ Imagine having to go through unconfirms or
adding your expiriences to bugs talked about on #mozillazine and having to log
in each time you add a comment, cast a vote, add yourself to the CC list or
whatever. It can get nasty.

I just had to login again to cast my vote. Now I'll be doing it one more time to
add this comment.

So PLEASE PLEASE PLEASE can someone with the power to do something do something?
Please? :/

Comment 73

[Bradley Baetz (:bbaetz)]

Attachment: v1

Adds the missing ;

Comment 74

[Dave Miller [:justdave]]

*** Bug 132914 has been marked as a duplicate of this bug. ***

Comment 75

[Bradley Baetz (:bbaetz)]

I'll take this, because I wrote the current patch.

If this doesn't get reviewed RSN, then it should be pushed off.

Comment 76

[Gervase Markham [:gerv]]

Comment on attachment 76929 [details] [diff] [review]
v1

>+    use Net::IPv4Addr qw(ipv4_network);

<sigh> If we really must.

>+    # Warn to help track down issues with this
>+    if ($@) {
>+        warn $@;
>+        $ipaddr = $addr if $@;

Use ThrowCodeError() instead of warn.

>+       SendSQL("SELECT netmask FROM profiles WHERE login_name = " . 
>+               SqlQuote($enteredlogin));

This is _so_ geeky. Does the netmask have to be configurable
per-user? Can't it just be an admin option? I can see that
everyone having the same value might be less secure, but I
still think this is not something we should be exposing.

If it has to be configurable per user, could we just have a 
checkbox - "allow login from your entire network",
which sets it to /24 or something? 

And really, we don't need to have a param to enable or disable this - 
admins can just remove it from the template. (Yes, clever people
who read the source could still hack the HTML, but is that really
something to care about?)

Gerv

Comment 77

[Bradley Baetz (:bbaetz)]

>(From update of attachment 76929 [details] [diff] [review])
>>+    use Net::IPv4Addr qw(ipv4_network);

><sigh> If we really must.

We don't have to, since I think that the server will always give us
dotted quad form, Its not required to by the cgi spec, though.

>+    # Warn to help track down issues with this
>+    if ($@) {
>+        warn $@;
>+        $ipaddr = $addr if $@;

Use ThrowCodeError() instead of warn.

Its not an unsolvable eror though - we can ignore it and things will still work.
It also makes the netmask stuff easier.

But yeah, you're propbably right.

>>+       SendSQL("SELECT netmask FROM profiles WHERE login_name = " . 
>>+               SqlQuote($enteredlogin));

>This is _so_ geeky. Does the netmask have to be configurable
>per-user? Can't it just be an admin option? I can see that
>everyone having the same value might be less secure, but I
>still think this is not something we should be exposing.
>
>If it has to be configurable per user, could we just have a 
>checkbox - "allow login from your entire network",
>which sets it to /24 or something? 

Thats one option. It has to be per user though - most won't want it/need it, and
there is a security penalty for enabling it.

But yes, I see your point.

>And really, we don't need to have a param to enable or disable this - 
>admins can just remove it from the template. (Yes, clever people
>who read the source could still hack the HTML, but is that really
>something to care about?)

YES! Of course we care about people sending data to override admin specified
security settings!!!

Comment 78

[Gervase Markham [:gerv]]

OK, fair enough on that last point :-) 

But can we do the checkbox thing? It's so much more user-friendly. Also, you
mentioned the possibility of rewriting the IPv4::Foo code to avoid Yet Another
Module Dependency. Are either of that, or the "just trust the server" option,
possible?

Gerv

Comment 79

[Bradley Baetz (:bbaetz)]

Well, I can make sure that we work with apache in that case.... I mean, I could
rewrite those functions, but since they already exist for us, I don't see the
point. And we will need an external module for ipv6, unless you want to rewrite
Math::Base85 (No, I'm not joking, unofrtunately)

Comment 80

[Gervase Markham [:gerv]]

OK, forget the rewriting. But do the checkbox :-) (The param to turn this on can
be the number the checkbox reduces the mask to; i.e. 32 turns the thing off
completely, and any lower number allows that much loosening.)

Gerv

Comment 81

[Bradley Baetz (:bbaetz)]

This has missed 2.16

->2.18, will tidy up sop that it can be applied after we release

Comment 82

[Gervase Markham [:gerv]]

BTW, if you go to:
http://www.blackcatnetworks.co.uk/cgi-bin/sqwebmail
you'll see that the developers of SQWebmail obviously hit the same problem, and
got around it with a checkbox at login. 

We could do that - instead of this being a per-account pref, have a checkbox at
login, and store whether we were being "loose" or not in the logincookies table.
This way, someone can be secure from work, where they have a fixed IP, but be
loose at home, because their home ISP has a round-robin proxy.

I suspect this would actually simplify the code, as well.

Gerv

Comment 83

[Bradley Baetz (:bbaetz)]

I have this sort of vague idea to move the login cookies into the tokens table
at some point, just to remove redundancy.

Anyway, I'm unlikley to get to this withint the next couple of weeks, due to
other  commitments. If someone else wants to do this, be my guest.

Comment 84

[Dave Miller [:justdave]]

Hmm, Brad, that's not a bad idea at all (moving it into the tokens table).  That
would satisfy the complains about the sequential session tokens, too, since we
could just use the "generatetoken" routine to create them.

Comment 85

[Bradley Baetz (:bbaetz)]

Well, I can just try to match against either the IP _or_ the net addr when
logging in. Actually, thats simpler, I think, unless we want to encode what type
of login we used, and use an extra bit on the login cookies table.

SQmail appears to be an all or nothing affair. I'd like to have a checkbox for
the same, but still have the 'what netmask to use' thing be a param.

Or maybe a bit on the logincookies table would be the better way, anyway. Hmm -
I'll have to think about that.

Comment 86

[Bradley Baetz (:bbaetz)]

Attachment: v2

OK, I prefer gerv's solution

This now doesn't have a schema change at all, and so the code is nicer, too.
The lack of flexability is probably irrelevent.

Comment 87

[Ross Gardler]

The V2 patch provided is not against the latest build, I have just done the work
necesaary to make it work against 2.16rc1. If anyone wnats it please mail me (I
can't attach it because I suffer from this bug! (It's too long to put in comments)

Comment 88

[Bradley Baetz (:bbaetz)]

Attachment: v2, take 2

The version I _made_ was agianst cvs. Unfortunately, that wasn't the version I
attached. Try this, instead, which has the different approach Gerv mentioned.

Comment 89

[Malcolm Scott]

As a temporary fix to this bug (for those behind proxies with changing IP
numbers) I've investigated the possibility of using HTTP_X_FORWARDED_FOR rather
than REMOTE_HOST if it is available. This seems to work OK, and fixes a few
problems I had behind a proxy. If anyone's interested in a patch, tell me.

Although, of course, this isn't a final solution - it doesn't solve the problem
of people who have *real* rotating IP numbers (e.g. auto-dialup/down). It just
makes things work a better for some people until a proper fix is applied.

Comment 90

[Bradley Baetz (:bbaetz)]

Yeah, but thats really really trivially forgable. I think that the attached
patch should just be reviewed, hint, hint...

justdave? matty?

Comment 91

[Dave Miller [:justdave]]

*** Bug 154644 has been marked as a duplicate of this bug. ***

Comment 92

[Gervase Markham [:gerv]]

Comment on attachment 83243 [details] [diff] [review]
v2, take 2

>+    # Some addresses (ie ipv6) won't work. Thats ok - we'll
>+    # just ignore the user's netmask in that case and match on
>+    # the entire string
>+    # Warn to help track down issues with this
>+    if ($@) {
>+        warn $@;
>+        $ipaddr = $addr if $@;
>+    }
>+    return $ipaddr;

We need a log, and a logging API.

Educate me: why is the seconf "if $@" necessary?

> sub CheckEmailSyntax {
>     my ($addr) = (@_);
>     my $match = Param('emailregexp');
>-    if ($addr !~ /$match/ || $addr =~ /[\\\(\)<>&,;:"\[\] \t\r\n]/) {
>+    if ($addr !~ /$match/ || $addr =~ /[\\\(\)<>&,;:\"\[\] \t\r\n]/) {

Is this a separate bug you are fixing?

>@@ -826,6 +853,18 @@
> <tr>
> <td align=right><b>Password:</b></td>
> <td><input type=password size=35 name=Bugzilla_password></td>";
>+        }
>+        if (Param("loginnetmask") < 32) {
>+            print "
>+</tr>
>+<tr>
>+<td align=\"right\">
>+  <b>Use this session<br>from different IPs:</b>
>+</td>
>+<td>
>+  <input type=\"checkbox\" name=\"Bugzilla_netlogin\">
>+  (Using this option decreases security)
>+</td>";
>         }
>         print "
> </tr>

Surely this is now templatised, right?

Also, "Using this option makes it easier for someone to impersonate you."

>-    "File::Spec"   => "0.82"
>+    "File::Spec"   => "0.82",
>+    "Net::IPv4Addr" => "0.10"

<sigh> Does it ship with Perl normally?

>+        return "an IPv4 netmask must be between 0 and 32";

I thought a netmask was specified as e.g. 255.255.255.0 . Why aren't we using
that form?

>+DefParam("loginnetmask",
>+         "The netmask used if a user chooses to allow a login to be valid
>+          from more than a single IP. Setting this to 32 disables this
>+          feature.",
>+         "t",
>+         '32',
>+         \&check_netmask);

I think allowing the admin to set any value just adds complexity. It's also
quite hard to explain exactly what it does, and "32 to disable" isn't 
particularly intuitive. Do we envisage anyone using any values other than 24 
and 32? 

Gerv

Comment 93

[Bradley Baetz (:bbaetz)]

> We need a log, and a logging API.

That may be true, but thats well outside the scope of this bug. I'll just remove
the |warn| if you want to insist on this...

> Educate me: why is the seconf "if $@" necessary?

Its not - remnants of before the eval was added.

> Is this a separate bug you are fixing?

Oops, yeah - xemacs stuffs up the colors. I should file a separate bug on that..

> Surely this is now templatised, right?

Nope, we never templatised the login screen...

No, Net::IPv4Addr doesn't ship normally. I can remove that dependency if we
assume that Ips are always in dotted waud form, which probably isn't that
unreasonable.

> I thought a netmask was specified as e.g. 255.255.255.0 . Why aren't we using
> that form?

Its more complicated. Teh number of one bits is all you require, anyway

> Do we envisage anyone using any values other than 24 and 32? 

Not often, no, but its possible that an admin may use something more restrictive.

Comment 94

[Bradley Baetz (:bbaetz)]

Attachment: v3

OK, now try this one. I've removed the dependency on Net::IPv4Addr, mainly
because I discovered that it only handled dotted-quad addresses anyway.

There isn't any schema change for this. Should I have one? There can't be any
false matches, because anyone whose 'real' IP was equal to the masked IP would
have a match on the masked entry anyway, so there isn't a security issue there.


Thoughts?

Comment 95

[Bradley Baetz (:bbaetz)]

Attachment: v4

OK, I've inverted the login for the checkbox, to make the words slightly less
geeky. I could s/IP/computer, but thats not accurate, because of
NAT/proxies/etc.

Comment 96

[Joel Peshkin]

yikes

Enabling this really leads to crummy security.   Could this at least use
something less predictable than the existing cookie?

Comment 97

[Bradley Baetz (:bbaetz)]

Well, you could argue that it provides greater security, by sending the password
arround in plain text less often

That said, this is optional, so people who have their own permenant IP wouldn't
enable this, and so wouldn't lose anything, and those who share via
NAT/transparent proxies/etc already appear to have the same IP as other users
anyway.

That said, there is a separate bug on moving the logincookies stuff to the token
table.

Comment 98

[Joel Peshkin]

Comment on attachment 101087 [details] [diff] [review]
v4

r=joel if you update the text at the param to indicate that numbers less than
32 can make it easier for someone else to impersonate a logged-in user.

Comment 99

[Bradley Baetz (:bbaetz)]

OK, checked in. This bug didn't deal with cookies, so I removed that bit from
the title. If someone wants to have bz deal with that, please file a separate bug.

Comment 100

[Thomas Bertels]

This doesn't seem to work here. Either with the checkbox unchecked or checked,
I'm  still asked to relogin on IP change.
To reproduce :
- login to bugzilla
- let the browser open and disconnect+reconnect (by browsing to the router page)
- return to bugzilla
Cookies are of course enabled (there is no problem if I close the browser
provided that my IP doesn't change).