Closed
Bug 90468
Opened 23 years ago
Closed 21 years ago
Implement optional session logout modes for Bugzilla
Categories
(Bugzilla :: User Accounts, enhancement, P3)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: afrey, Assigned: toms.baugis)
Details
Attachments
(1 file, 8 obsolete files)
4.36 KB,
patch
|
kiko
:
review+
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
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).
Updated•23 years ago
|
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.18
Updated•23 years ago
|
Severity: normal → enhancement
Comment 2•23 years ago
|
||
-> Bugzilla product, User Accounts component, reassigning.
Assignee: tara → myk
Component: Bugzilla → User Accounts
Product: Webtools → Bugzilla
Version: Bugzilla 2.12 → 2.12
Assignee | ||
Comment 3•22 years ago
|
||
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).
Assignee | ||
Updated•22 years ago
|
Assignee | ||
Comment 4•22 years ago
|
||
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
Assignee | ||
Updated•22 years ago
|
Attachment #85769 -
Attachment is obsolete: true
Comment 5•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
That sounds fine. The current patch does it as a per-user default though - I don't know if we want that.
Comment 8•22 years ago
|
||
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.
Attachment #86228 -
Flags: review-
Assignee | ||
Comment 9•22 years ago
|
||
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?
Updated•21 years ago
|
OS: Linux → All
Hardware: PC → All
Comment 10•21 years ago
|
||
After adding the main patch, add this change and run checksetup.pl before using bugzilla.
Assignee | ||
Comment 11•21 years ago
|
||
Someone, please - check and correct my english - it would just make too much time for me to figure out the right texts.
Comment 12•21 years ago
|
||
Comment on attachment 135918 [details] [diff] [review] Patch against HEAD, with solution from comment #6 since you reviewed this before... (many moons ago :)
Attachment #135918 -
Flags: review?(bugreport)
Comment 13•21 years ago
|
||
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.
Attachment #135918 -
Flags: review?(bugreport) → review-
Comment 14•21 years ago
|
||
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•21 years ago
|
||
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
Assignee: myk → toms
Comment 16•21 years ago
|
||
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.
Assignee | ||
Comment 17•21 years ago
|
||
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.
Attachment #135918 -
Attachment is obsolete: true
Assignee | ||
Comment 18•21 years ago
|
||
Comment on attachment 136100 [details] [diff] [review] patch v1.1 against HEAD, with solution from comm #6 train has stopped, again. please review!
Attachment #136100 -
Flags: review?(bugreport)
Comment 19•21 years ago
|
||
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•21 years ago
|
||
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.
Attachment #136100 -
Flags: review?(bugreport) → review-
Comment 21•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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.
Assignee | ||
Comment 26•21 years ago
|
||
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•21 years ago
|
||
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?)
Status: NEW → ASSIGNED
Assignee | ||
Comment 28•21 years ago
|
||
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•21 years ago
|
||
Not AFAICS.
Assignee | ||
Comment 30•21 years ago
|
||
fixing nits of comment #19
Assignee | ||
Updated•21 years ago
|
Attachment #86228 -
Attachment is obsolete: true
Attachment #135912 -
Attachment is obsolete: true
Attachment #136100 -
Attachment is obsolete: true
Assignee | ||
Comment 31•21 years ago
|
||
Comment on attachment 138622 [details] [diff] [review] patch v1.2 against HEAD asking for review from kiko.
Attachment #138622 -
Flags: review?(kiko)
Comment 32•21 years ago
|
||
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?
Assignee | ||
Comment 33•21 years ago
|
||
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 %]
Assignee | ||
Updated•21 years ago
|
Attachment #138622 -
Attachment is obsolete: true
Comment 34•21 years ago
|
||
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 :-)
Attachment #138926 -
Flags: review-
Assignee | ||
Comment 35•21 years ago
|
||
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.
Attachment #138926 -
Attachment is obsolete: true
Comment 36•21 years ago
|
||
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?
Assignee | ||
Comment 37•21 years ago
|
||
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
Assignee | ||
Comment 38•21 years ago
|
||
Comment on attachment 138948 [details] [diff] [review] fixes vulnerability and adds real cookie expiration at session end obosolete #35
Attachment #138948 -
Attachment is obsolete: true
Comment 39•21 years ago
|
||
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.
Attachment #139018 -
Flags: review+
Updated•21 years ago
|
Flags: approval?
Updated•21 years ago
|
Attachment #138622 -
Flags: review?(kiko)
Updated•21 years ago
|
Flags: approval? → approval+
Comment 40•21 years ago
|
||
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 toms@myrealbox.com (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
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•20 years ago
|
Summary: Bugzilla does not log out automatically when closing the session → Implement optional session logout modes for Bugzilla
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•