90468 - Implement optional session logout modes for Bugzilla

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement, P3)

Description

[Angelika Frey]

If you don't log out explicitly, Bugzilla access is granted in every new Browser
session. As we want to uses Buzilla as a ticket tracking sysstem for a variety
of customers, we consider this a security hazard.

Is there any trick to change that?

Comment 1

[Jacob Steenhagen]

I theory, you can go to line 804 of CGI.pl (in version 2.12) and remove 
the "expires" part of the cookie (on both lines 804 and 805).  This will make 
the cookies session cookies rather than stored cookies (the browser will delete 
them as soon as it is closed).

Comment 2

[Andreas Franke (gone)]

-> Bugzilla product, User Accounts component, reassigning.

Comment 3

[Toms Baugis]

Attachment: patch for users option whether expire the cookie or not

This patch needs some changes in database - add to table PROFILES column
paranoia (tinyint(1)).

Patch makes possible for the user to choose whether or not to expire the cookie
(forget login on browser close).

Comment 4

[Toms Baugis]

Attachment: patch v1.1

1. execute this line @ mysql:
   alter table profiles add column remember_login tinyint(1)

2. Append patch. There where some syntax errors which forced POST_DATA for no
reason. FIXED in this patch

Comment 5

[Bradley Baetz (:bbaetz)]

Comment on attachment 86228 [details] [diff] [review]
patch v1.1

Maybe this should be a param instead of a per-user option?

Alternatly, just use the checkbox and have an admin modify the template to
change the default. That wouldn't stop someone logging in for multiple sessons
even if the admin doesn't want them to (by manually changing the url)

Comment 6

[Dave Miller [:justdave]]

What would anyone think of doing it this way:

Have the checkbox, instead of in the user's preferences, on the login screen. 
The checkbox says "Remember my login between browser sessions".  The admin sets
a default with a param.  Default as shipped will be to have the box checked to
preserve existing behavior.

Not sure if that's desired behavior or not but I've seen it done that way a lot
of places...

Comment 7

[Bradley Baetz (:bbaetz)]

That sounds fine. The current patch does it as a per-user default though - I
don't know if we want that.

Comment 8

[Joel Peshkin]

Comment on attachment 86228 [details] [diff] [review]
patch v1.1



If this does go into the database, then checksetup.pl needs to be updated to
take care of the change.

This is a handy idea.  In fact, it may pay to make the length of time that the
server keeps the token around configurable.

Comment 9

[Toms Baugis]

due to my overloaded schedule i'm afraid that i'll not be able to fix this in
next weeks (or even months). could someone take this bug on himeself, please?

Comment 10

[Vijayan Reddy]

Attachment: Patch for checksetup.pl to add the field to DB

After adding the main patch,

add this change and run checksetup.pl before using bugzilla.

Comment 11

[Toms Baugis]

Attachment: Patch against HEAD, with solution from comment #6

Someone, please - check and correct my english - it would just make too much
time for me to figure out the right texts.

Comment 12

[Dave Miller [:justdave]]

Comment on attachment 135918 [details] [diff] [review]
Patch against HEAD, with solution from comment #6

since you reviewed this before...  (many moons ago :)

Comment 13

[Joel Peshkin]

Comment on attachment 135918 [details] [diff] [review]
Patch against HEAD, with solution from comment #6


I might have missed it, but where does Bugzilla_remember get set if
rememberlogin is 'on'??

Also, the desciption is a bit odd.  I think you want to descibe this as a
control overe the persistance of the login after the browser is closed rather
than pretending to store the ID on the computer.

How about...
"Remain logged in on this computer" or some similar  language??

We really should add a mechanism to auto-logout sessions that go idle for more
than a specified time and have this handled on the server.  This doesn't help
anyone whose cookies are kidnapped.  But, that would be a seperate bug.

Comment 14

[Christian Reis]

Comment on attachment 135918 [details] [diff] [review]
Patch against HEAD, with solution from comment #6

AFAICS, Bugzilla_remember will be correctly picked up by
Bugzilla::Auth::CGI->login upon form submission (if you look at the code in
login you'll see it pulls stuff directly from the CGI object. So it does seem
to work correctly.

>Index: login.html.tmpl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/account/auth/login.html.tmpl,v
>+    [% IF Param('rememberlogin') != 'on' && Param('rememberlogin') != 'off' %]
>+      <tr>
>+        <td colspan="2">
>+          <input type="checkbox" name="Bugzilla_remember" 
>+                 [% "checked=checked" IF Param('rememberlogin') == "on" 
>+                                      || Param('rememberlogin') == "defaulton" %]>

However, I'm denying review because the rememberlogin == "on" here seems
redundant; we're already saying the checkbox won't be displayed when
rememberlogin is on by default. What you probably want is to display an input
type=hidden here in the case of "on" or "off", and the normal checkbox for the
cases "defaulton"/"defaultoff".

>+          Remember my ID on this computer
>+        </td>
>+      </tr>
>+    [% END %]

The better wording is probably "Remember my Login" since it's used on other
sites (I've seen it at SF.net, and many others) and the term Login is what's
currently in use in login.html.tmpl.

Comment 15

[Christian Reis]

I forgot to say, but it would be great if we could see the checksetup patch
integrated with this one so we have a single patch to review and checkin!

Reassigning to master Toms, patch author extraordinaire

Comment 16

[Bradley Baetz (:bbaetz)]

You also misspelt remembers:

 +           '<li>on - just remebers session info</li>' .

I don't think we want a schema change for this, either - we deliberately didn't
have one for the netmask stuff which was added.

Comment 17

[Toms Baugis]

Attachment: patch v1.1 against HEAD, with solution from comm #6

Thanx for input! In this patch i fixed things mentioned in #14 and #16.
Also somehow tried to reformat parameter text in defparams (please read it and
correct). 
comment #15 - this solution doesn't need to change checksetup since there is no
new field.

Comment 18

[Toms Baugis]

Comment on attachment 136100 [details] [diff] [review]
patch v1.1 against HEAD, with solution from comm #6

train has stopped, again. please review!

Comment 19

[Christian Reis]

Comment on attachment 136100 [details] [diff] [review]
patch v1.1 against HEAD, with solution from comm #6

>Index: CGI.pm
>+        if($cgi->param("Bugzilla_remember") eq 'on') {

You missed a space before the parenthesis here.

>+           $cookieexpires = 'Sun, 30-Jun-2029 00:00:00 GMT';

Remind me why we're changing the date on the expires? It used to be:

>-                          -expires => 'Fri, 01-Jan-2038 00:00:00 GMT');

-- any particular reason?

Have you made sure this patch passes the template tests (i.e. runtests.sh runs
fine)? 

Oh, and:

>cvs -q diff -u defparams.pl (in directory G:\bugzilla_test\)

-- you're using the wrong operating system :-P

Comment 20

[Joel Peshkin]

Comment on attachment 136100 [details] [diff] [review]
patch v1.1 against HEAD, with solution from comm #6

This needs to have Kiko's comments addressed. 

Also, this is going to cause the logincookies table to accumulate a large
number of still-valid cookie values that could be hijacked.

I think that, when a user logs in on a new session, we should invalidate his
old cookies.   I'd like to get another opinion on that point, though.

Comment 21

[Dave Miller [:justdave]]

That would break my usage if we did that.  My office has two computers next to
each other (different things open on each, usually, and one is typically behind
AOL's firewall via VPN and the other isn't).  I quite often visit Bugzilla from
both, and would be finding myself having to log in again any time I switched
which computer I was looking at, even though that computer hadn't logged out.

Comment 22

[Joel Peshkin]

I'd like to see some way to keep from having zombie valid tokens. Perhaps each
PC should wind up with a cookie that DOES persist in addition to the one that
doesn't so that the system can tell when the same machine re-visits without the
login token and knows to clean up its old token.

We need a good solution here.

Comment 23

[Christian Reis]

Well, I don't think the patch on this bug makes this problem much worse than it 
already is (in my logincookies table I have a number of repeat entries for the 
same userid and ip address), so I'd suggest opening a follow-up bug to this one.

Perhaps we could restrict ourself to invalidating all cookies from a specific 
IP address once the user has logged in -- that way you can have at most one 
entry per IP address. Beyond that you can have a process that reaps stale 
logincookies once they haven't been touched in X minutes or so. I still defend 
this should be part of another bug now what we have a patch that's at the end 
of the reviw process, though.

Comment 24

[Christian Reis]

If Toms answers the questions in comment 19, the spacing can be fixed before 
checking this patch in, since it's just a style nit.

Comment 25

[Joel Peshkin]

I agree. (On this patch)

As a seperate bug, we need to do a better job of login cookie housekeeping.  My
own preference is we seperate the invalidation of the record in the DB from the
deletion of the record.  It is bad security karma to be unable to tell a user
where his lasty login was from.  Personally, I'd opt for the approach in #20
with a user-pref to keep from breaking justdave.

Comment 26

[Toms Baugis]

In reply to comment 19 from Christian Reis:

> (From update of attachment 136100 [details] [diff] [review])
> >Index: CGI.pm
> >+        if($cgi->param("Bugzilla_remember") eq 'on') {
>
> You missed a space before the parenthesis here.
that is - after "if" or before both parenthesis ? (sorry, no expert in perl)

>
> >+           $cookieexpires = 'Sun, 30-Jun-2029 00:00:00 GMT';
> Remind me why we're changing the date on the expires? It used to be:
> >-                          -expires => 'Fri, 01-Jan-2038 00:00:00 GMT');
> -- any particular reason?

no reason, just copy-paste form some another place. i will fix that together
with other things

> Have you made sure this patch passes the template tests (i.e. runtests.sh runs
> fine)? 

>[root@cvs bugzilla_test]# perl runtests.pl
t/001compile.......ok
t/002goodperl......ok
t/003safesys.......ok
t/004template......NOK 24#     Failed test (t/004template.t at line 75)
t/004template......ok 26/312Confused test output: test 26 answered after test 26
t/004template......ok 27/312Confused test output: test 27 answered after test 27
..and so on

i guess i am doing something wrong

Comment 27

[Christian Reis]

I meant after the if -- the style (and Bugzilla in general I risk) that file
follows is

if (xxx) {
}

with a space after the if.

As to the tests, that's definitely weird. Seems like an internal error in the
test suite, but I couldn't reproduce it here (I tried with your patch applied
locally), so that's fine. (Are you running the tests on a box with all the
required Perl packages?)

Comment 28

[Toms Baugis]

regarding spacing - understood
regarding tests - well, i guess it doesn't matter currently, because everything
worked fine for you (so the patch is fine for tests)

ummm... anything else?

Comment 29

[Christian Reis]

Not AFAICS.

Comment 30

[Toms Baugis]

Attachment: patch v1.2 against HEAD

fixing nits of comment #19

Comment 31

[Toms Baugis]

Comment on attachment 138622 [details] [diff] [review]
patch v1.2 against HEAD

asking for review from kiko.

Comment 32

[Christian Reis]

Comment on attachment 138622 [details] [diff] [review]
patch v1.2 against HEAD

Toms, does this work for the case where the checkbox is used? ISTM you're only
comparing Bugzilla_remember to the string 'on', and I don't see how a
'checked=checked' attribute can equate to 'on' -- can you explain?

Comment 33

[Toms Baugis]

Attachment: clarify the checked value

uh, well, i can not explain it, but it worked for me :)
anyway - this patch makes it more understandable (checked=checked replaced by
value="on" [% "checked" if blahblah %]

Comment 34

[Christian Reis]

Comment on attachment 138926 [details] [diff] [review]
clarify the checked value

I just realized this presents a minor security issue. If the administrator has
set the rememberlogin Param to 'off', the user can still work around it by
forging a Bugzilla_remember CGI form variable. 

To avoid this, I'd suggest changing:

>Index: Bugzilla/Auth/CGI.pm
>+        my $cookieexpires = '';
>+        if ($cgi->param("Bugzilla_remember") eq 'on') {
>+           $cookieexpires = 'Fri, 01-Jan-2038 00:00:00 GMT';
>+        }

Instead of checking the $cgi->param alone here, you'd do:

  if (Param('rememberlogin') eq 'on') || 
	(Param('rememberlogin') neq 'off') &&
	 $cgi->param('Bugzilla_remember') eq 'on')) {

    # ...

  }

This way, you'll make sure the CGI variable is only checked if we're using one
of the default* variants. Then:

>Index: template/en/default/account/auth/login.html.tmpl
>+    [% IF Param('rememberlogin') == 'on' || Param('rememberlogin') == 'off'%]
>+      <input type="hidden" name="Bugzilla_remember" value="[% Param('rememberlogin') %]">
>+    [% ELSE %]
>+      <tr>
>+        <td colspan="2">
>+          <input type="checkbox" name="Bugzilla_remember" value="on"
>+                 [% "checked" IF Param('rememberlogin') == "defaulton" %]>
>+          Remember my Login
>+        </td>
>+      </tr>
>+    [% END %]


-- you can avoid the hidden variable here altogether and just present the
checkbox if rememberlogin != on and != off. This makes for proper separation of
concerns (the Params are verified at the lowest level) and fixes this (arguably
trivial) security issue.

Sorry for not noticing this earlier (and perhaps being a bit misleading in my
initial comments) -- I sure didn't want to put you through an extra hoop! I'll
be happy to r+ your fixed patch, of course :-)

Comment 35

[Toms Baugis]

Attachment: fixes vulnerability and adds real cookie expiration at session end

uh, you are right. i hope this time i made it right.
also - i found out, that expires '' doesn't work - it expired after each page
(it didn't store the cookie at all). setting expires to ' ' fixes this.

Comment 36

[Christian Reis]

Comment on attachment 138948 [details] [diff] [review]
fixes vulnerability and adds real cookie expiration at session end

Two questions:

a) Isn't the approach I suggested a bit cleaner? I outlined an approach in
comment 34 that would have worked, I think, and it also avoids the need for the
hidden (and makes the whole thing clearer). Do you object to using the
suggested pattern?

b) From http://www.perldoc.com/perl5.6/lib/CGI/Cookie.html it would seem to me
that to provide a session cookie (i.e. one that doesn't persist) you just need
to *omit* the -expires option. Have you tried that?

Comment 37

[Toms Baugis]

Attachment: clean up template code and reform cookie expiration stuff

when i posted my patch on comment #35 i haven't read all your comment. after i
read it i knew, that there will be something more, so i just waited :)

i didn't like the resulting condition in CGI.pm (if this or (this and that)) -
it is somehow unreadable, but it is anyway cleaner than to leave part of code
in template (and i could not figure out how to make it look better).

yes - i tried to remove expiration parameter and it did work. also i searched
around for cookies and found the same text as in perldoc - if it is not
specified, then expires at the end of session.

after that i read bugzilla patching guide (or how it is named) and fixed 4
space identing and stuff about curly brackets (else comes on next line after }
)
also i noticed, that in CGI.pm this has not been taken into account in one
place, so i thought i could fix this too.

i hope that at last i am done, but of course - the last word is after you :)

br, 
tm

Comment 38

[Toms Baugis]

Comment on attachment 138948 [details] [diff] [review]
fixes vulnerability and adds real cookie expiration at session end

obosolete #35

Comment 39

[Christian Reis]

Comment on attachment 139018 [details] [diff] [review]
clean up template code and reform cookie expiration stuff

Looks good. There's a missing space preceding a %] TT tag but that can be fixed
when checking in.

Comment 40

[Vlad Dascalu]

Kiko already commited this. FIXED.

01/16/2004 14:46	kiko%async.com.br 	mozilla/ webtools/ bugzilla/ Bugzilla/ Auth/
CGI.pm 	1.6 	22/7  	Fix for bug 90468: Bugzilla does not log out automatically
when closing the session. Patch by [e-mail] (Toms Baugis), with minor
cleanups by me. r=kiko, a=myk.
01/16/2004 14:46	kiko%async.com.br 	mozilla/ webtools/ bugzilla/ template/ en/
default/ account/ auth/ login.html.tmpl 	1.4 	15/0 
01/16/2004 14:46	kiko%async.com.br 	mozilla/ webtools/ bugzilla/ defparams.pl 
1.123 	18/0