Closed Bug 98146 Opened 23 years ago Closed 23 years ago

[security] Minor problem w/ error message if some sort of login error in doeditvotes

Categories

(Bugzilla :: Creating/Changing Bugs, defect, P1)

2.15
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: bugzilla, Assigned: gerv)

Details

(Whiteboard: security: untrusted var in HTML | applied to 2.14.1)

Attachments

(3 files, 1 obsolete file)

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?
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.
Assignee: justdave → myk
Severity: minor → critical
Component: Bugzilla-General → Creating/Changing Bugs
Priority: -- → P1
Whiteboard: security: untrusted var in HTML
Target Milestone: --- → Bugzilla 2.16
Attached patch Patch v.1 (obsolete) — Splinter Review
1 down, 352 to go...

Gerv
Assignee: myk → gerv
Keywords: patch, review
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?
Attachment #53513 - Flags: review-
Attached patch Patch v.2Splinter Review
myk: re-review?

Gerv
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.
Attachment #53658 - Flags: review+
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.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
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.
Nope, it shouldn't...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch bugfix v1Splinter Review
Good catch.

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

Gerv
Status: REOPENED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
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
Whiteboard: security: untrusted var in HTML → security: untrusted var in HTML | applied to 2.14.1
Attachment #53513 - Attachment is obsolete: true
Summary: 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 doeditvotes
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
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: