95235 - Insecure variables passed to DisplayError() from buglist.cgi

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[GavinS]

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

[GavinS]

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?)

Comment 2

[Gervase Markham [:gerv] (not reading bugmail)]

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

Gerv

Comment 3

[Dave Miller [:justdave] ([e-mail])]

Attachment: Patch v1

Comment 4

[Jacob Steenhagen]

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

Comment 5

[GavinS]

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

[GavinS]

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.

Comment 7

[Zach Lipton [:zach]]

r=[e-mail] as this is, though perhaps we should fix all of this for 2.14?

Zach

Comment 8

[GavinS]

Attachment: My patch against 2.12 for these changes

Comment 9

[Zach Lipton [:zach]]

Can someone roll this into a patch to the tip?

Zach

Comment 10

[Dave Miller [:justdave] ([e-mail])]

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

[Dave Miller [:justdave] ([e-mail])]

Moving to Bugzilla product