Insecure variables passed to DisplayError() from buglist.cgi

RESOLVED FIXED in Bugzilla 2.14

Status

()

Bugzilla
Bugzilla-General
P1
major
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: GavinS, Assigned: justdave)

Tracking

unspecified
Bugzilla 2.14
Sun
SunOS

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

16 years ago
Apologies if this is a duplicate, but querying bugzilla is broken at the moment,
so I can't check.

The SqlifyDate() routine has the following:

>>   if (!defined $date) {
>>        PuntTryAgain("The string '<tt>$str</tt>' is not a legal date.");
 
Should there be either a html_quote(), or a value_quote() around the $str?
(Reporter)

Comment 1

16 years ago
I think there are some more issues in buglist.cgi in all the places which call
Error(). Many of these may pass unsafe values to Error(), which does no quoting
itself. (e.g. at least N votes)

I think it should, shouldn't it?


(And on a related note, the DisplayError() and PuntTryAgain() routines in CGI.pl
don't do any quoting. It wouldn't harm to do html_quote'ing there, would it?)

Targetting at 2.14 for evaluation, given that 2.14 is a security release.

Gerv
Summary: Insecure error message in buglist.cgi:SqlifyDate() ?? → Insecure error message in buglist.cgi:SqlifyDate()
Target Milestone: --- → Bugzilla 2.14

Updated

16 years ago
Created attachment 46370 [details] [diff] [review]
Patch v1
Keywords: patch, review
Priority: -- → P1

Comment 4

16 years ago
DisplayError() is sometimes intentially passed html code (such as a link to log
in) so it should not be html_quote()ed.

Attachment 46370 [details] [diff] looks good to me.
r=jake
Keywords: review → approval
(Reporter)

Comment 5

16 years ago
buglist.cgi still calls Error() without any quoting, which calls PuntTryAgain().
I think one of these needs fixing as well.

(e.g. it is possible to use <script> cmds in the 'at least N votes' field)

(The fact that DisplayError() doesn't quote stuff should probably go in the new
Reviewers Guide)
(Reporter)

Comment 6

16 years ago
Ok, so I've checked the Error() routine, and PuntTryAgain(), and both of these
want to specify valid html as part of the error message, so any quoting has to
be done in the calls to these 2 routines.

There are quite a few places which call one of these unsafely in the case of an
error. In 2.12, these were:

buglist.cgi:
1) sqlifydate
2) 'at least n votes'
3) 'must specify 1 or more fields to search for email'
4) 'changed in last n days'
5) invalid keyword

And in process_bug.cgi:
1) invalid keyword.


I can attach a patch against 2.12 , but that probably won't help much. I'd
suggest a search of all the places calling PuntTryAgain(), Error(), and
DisplayError() in your 2.13 tree.
Summary: Insecure error message in buglist.cgi:SqlifyDate() → Insecure variables passed to DisplayError() from buglist.cgi

Comment 7

16 years ago
r=zach@zachlipton.com as this is, though perhaps we should fix all of this for 2.14?

Zach
(Reporter)

Comment 8

16 years ago
Created attachment 46869 [details] [diff] [review]
My patch against 2.12 for these changes

Comment 9

16 years ago
Can someone roll this into a patch to the tip?

Zach
believe it or not, Zach, this patch applied cleanly (with offsets) to the cvs
tip.  I scanned through it real quick and verified no additional calls to
Error() or PuntTryAgain() were added since then, and all looks good.

r= justdave

Checked in.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Moving to Bugzilla product
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: Bugzilla 2.12 → unspecified
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.