Last Comment Bug 95235 - Insecure variables passed to DisplayError() from buglist.cgi
: Insecure variables passed to DisplayError() from buglist.cgi
Product: Bugzilla
Classification: Server Software
Component: Bugzilla-General (show other bugs)
: unspecified
: Sun SunOS
P1 major (vote)
: Bugzilla 2.14
Assigned To: Dave Miller [:justdave] (
: default-qa
Depends on:
  Show dependency treegraph
Reported: 2001-08-14 06:47 PDT by GavinS
Modified: 2012-12-18 20:46 PST (History)
0 users
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

Patch v1 (586 bytes, patch)
2001-08-19 00:18 PDT, Dave Miller [:justdave] (
no flags Details | Diff | Splinter Review
My patch against 2.12 for these changes (3.22 KB, patch)
2001-08-23 01:01 PDT, GavinS
no flags Details | Diff | Splinter Review

Description User image GavinS 2001-08-14 06:47:47 PDT
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?
Comment 1 User image GavinS 2001-08-15 02:18:12 PDT
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
don't do any quoting. It wouldn't harm to do html_quote'ing there, would it?)

Comment 2 User image Gervase Markham [:gerv] 2001-08-18 10:33:09 PDT
Targetting at 2.14 for evaluation, given that 2.14 is a security release.

Comment 3 User image Dave Miller [:justdave] ( 2001-08-19 00:18:03 PDT
Created attachment 46370 [details] [diff] [review]
Patch v1
Comment 4 User image Jacob Steenhagen 2001-08-20 19:50:19 PDT
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.
Comment 5 User image GavinS 2001-08-21 00:49:59 PDT
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)
Comment 6 User image GavinS 2001-08-21 01:54:30 PDT
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:

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.
Comment 7 User image Zach Lipton [:zach] 2001-08-21 21:04:16 PDT as this is, though perhaps we should fix all of this for 2.14?

Comment 8 User image GavinS 2001-08-23 01:01:07 PDT
Created attachment 46869 [details] [diff] [review]
My patch against 2.12 for these changes
Comment 9 User image Zach Lipton [:zach] 2001-08-23 18:31:51 PDT
Can someone roll this into a patch to the tip?

Comment 10 User image Dave Miller [:justdave] ( 2001-08-24 10:35:18 PDT
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.
Comment 11 User image Dave Miller [:justdave] ( 2001-09-02 23:42:02 PDT
Moving to Bugzilla product

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