Last Comment Bug 98146 - [security] Minor problem w/ error message if some sort of login error in doeditvotes
: [security] Minor problem w/ error message if some sort of login error in doed...
Status: RESOLVED FIXED
security: untrusted var in HTML | app...
:
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 2.15
: All All
: P1 critical (vote)
: Bugzilla 2.16
Assigned To: Gervase Markham [:gerv]
: default-qa
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2001-09-04 03:38 PDT by GavinS
Modified: 2012-12-18 20:46 PST (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v.1 (715 bytes, patch)
2001-10-14 18:55 PDT, Gervase Markham [:gerv]
myk: review-
Details | Diff | Splinter Review
Patch v.2 (810 bytes, patch)
2001-10-15 15:25 PDT, Gervase Markham [:gerv]
myk: review+
myk: review+
Details | Diff | Splinter Review
bugfix v1 (591 bytes, patch)
2001-10-16 06:58 PDT, Jacob Steenhagen
no flags Details | Diff | Splinter Review
Composite patch against 2.14 (782 bytes, patch)
2001-11-25 21:01 PST, Dave Miller [:justdave] (justdave@bugzilla.org)
no flags Details | Diff | Splinter Review

Description GavinS 2001-09-04 03:38:38 PDT
doeditvotes.cgi contains the following:

> if ( (! defined $who) || (!$who) ) {
> PutHeader("Bad login.");
>
> print "The login info got confused.  If you want to adjust the votes\n";
> print "for <tt>$::COOKIE{'Bugzilla_login'}</tt>, then please\n";
> print "<a href=showvotes.cgi?user=$who>click here</a>.<hr>\n";
>
> PutFooter();
>
> exit();
>}


If something went mad, and $who was undefined, then this warning message would
try and use it in the showvotes.cgi link ( => http log warning + bad link generated)

Should the message just say 'please log in (again) and try again', or something
like that?
Comment 1 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-09-04 11:44:59 PDT
my first though on this is "oh, it didn't get escaped" but after looking at it
for a minute, I agree with Gavin's thoughts on how to fix it.  If we thing the
login info is screwed up in any way, we shouldn't be displaying what we think is
screwed up to the user, in case it's not really them and someone trying to use a
faked cookie or something.
Comment 2 Gervase Markham [:gerv] 2001-10-14 18:55:47 PDT
Created attachment 53513 [details] [diff] [review]
Patch v.1
Comment 3 Gervase Markham [:gerv] 2001-10-14 18:56:21 PDT
1 down, 352 to go...

Gerv
Comment 4 Myk Melez [:myk] [@mykmelez] 2001-10-15 15:06:56 PDT
Comment on attachment 53513 [details] [diff] [review]
Patch v.1

>+    print "The login info got confused. Please log in (again) and try again.\n";

Shouldn't "log in (again)" be a link?
Comment 5 Gervase Markham [:gerv] 2001-10-15 15:25:45 PDT
Created attachment 53658 [details] [diff] [review]
Patch v.2
Comment 6 Gervase Markham [:gerv] 2001-10-15 15:26:08 PDT
myk: re-review?

Gerv
Comment 7 Myk Melez [:myk] [@mykmelez] 2001-10-15 16:06:46 PDT
Comment on attachment 53658 [details] [diff] [review]
Patch v.2

>Index: doeditvotes.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/doeditvotes.cgi,v
>retrieving revision 1.12
>diff -u -r1.12 doeditvotes.cgi
>--- doeditvotes.cgi	2001/08/28 17:55:14	1.12
>+++ doeditvotes.cgi	2001/10/15 22:23:31
>@@ -84,9 +84,11 @@
> 
> if ( (! defined $who) || (!$who) ) {
>     PutHeader("Bad login.");
>-    print "The login info got confused.  If you want to adjust the votes\n";
>-    print "for <tt>$::COOKIE{'Bugzilla_login'}</tt>, then please\n";
>-    print "<a href=showvotes.cgi?user=$who>click here</a>.<hr>\n";
>+    print qq|
>+    The login info got confused. Please
>+    <a href="http://bugzilla.mozilla.org/query.cgi?GoAheadAndLogIn=1">log in</a>
>+    (again) and try again.\n
>+    |;
>     PutFooter();
>     exit();
> }

Looks good. I can't reproduce the problem, but the fix is trivial, the
script works as it should, and tests pass.  r=myk, no second review needed.

Patch checked in.
Comment 8 Myk Melez [:myk] [@mykmelez] 2001-10-15 16:07:21 PDT
Checking in doeditvotes.cgi;
/cvsroot/mozilla/webtools/bugzilla/doeditvotes.cgi,v  <--  doeditvotes.cgi
new revision: 1.13; previous revision: 1.12
done

Patch checked in.  Resolving fixed.
Comment 9 David D. Kilzer (ddk) 2001-10-16 06:51:55 PDT
Should the fix really have a URL prefixed with
"http://bugzilla.mozilla.org/"?  That would make all installations
point back to bmo, which (I believe) is incorrect behavior.

I'd reopen this bug, but I don't have permission to do so.
Comment 10 Jacob Steenhagen 2001-10-16 06:56:22 PDT
Nope, it shouldn't...
Comment 11 Jacob Steenhagen 2001-10-16 06:58:24 PDT
Created attachment 53743 [details] [diff] [review]
bugfix v1
Comment 12 Gervase Markham [:gerv] 2001-10-16 11:46:10 PDT
Good catch.

/cvsroot/mozilla/webtools/bugzilla/doeditvotes.cgi,v  <--  doeditvotes.cgi
new revision: 1.14; previous revision: 1.13

Gerv
Comment 13 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-11-25 20:59:06 PST
Patch (to be attached momentarily) applied to the 2.14.1 branch

/cvsroot/mozilla/webtools/bugzilla/doeditvotes.cgi,v  <--  doeditvotes.cgi
new revision: 1.12.2.1; previous revision: 1.12
Comment 14 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-11-25 21:01:16 PST
Created attachment 59138 [details] [diff] [review]
Composite patch against 2.14
Comment 15 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-12-10 17:27:14 PST
Hmm, it seems the bulk change thinks I'm not changing anything if all I do is
add names to the CC list, so I guess I have to make a comment.  Anyhow, adding
the representatives from the organizations we know of that support Bugzilla
distributions so they're aware of our upcoming security release

Note You need to log in before you can comment on or make changes to this bug.