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

RESOLVED FIXED in Bugzilla 2.16

Status

()

Bugzilla
Creating/Changing Bugs
P1
critical
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: GavinS, Assigned: gerv)

Tracking

2.15
Bugzilla 2.16

Details

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

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

16 years ago
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
(Assignee)

Comment 2

16 years ago
Created attachment 53513 [details] [diff] [review]
Patch v.1
(Assignee)

Comment 3

16 years ago
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-
(Assignee)

Comment 5

16 years ago
Created attachment 53658 [details] [diff] [review]
Patch v.2
(Assignee)

Comment 6

16 years ago
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
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 9

16 years ago
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

16 years ago
Nope, it shouldn't...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 11

16 years ago
Created attachment 53743 [details] [diff] [review]
bugfix v1
(Assignee)

Comment 12

16 years ago
Good catch.

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

Gerv
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 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
Created attachment 59138 [details] [diff] [review]
Composite patch against 2.14
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.