Closed
Bug 95235
Opened 23 years ago
Closed 23 years ago
Insecure variables passed to DisplayError() from buglist.cgi
Categories
(Bugzilla :: Bugzilla-General, defect, P1)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.14
People
(Reporter: bugzilla, Assigned: justdave)
References
()
Details
Attachments
(2 files)
586 bytes,
patch
|
Details | Diff | Splinter Review | |
3.22 KB,
patch
|
Details | Diff | Splinter Review |
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?
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•23 years ago
|
||
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•23 years ago
|
Assignee | ||
Comment 3•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Comment 4•23 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
Assignee | ||
Updated•23 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)
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.
Assignee | ||
Updated•23 years ago
|
Summary: Insecure error message in buglist.cgi:SqlifyDate() → Insecure variables passed to DisplayError() from buglist.cgi
Comment 7•23 years ago
|
||
r=zach@zachlipton.com as this is, though perhaps we should fix all of this for 2.14? Zach
Comment 9•23 years ago
|
||
Can someone roll this into a patch to the tip? Zach
Assignee | ||
Comment 10•23 years ago
|
||
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
Closed: 23 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 11•23 years ago
|
||
Moving to Bugzilla product
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: Bugzilla 2.12 → unspecified
Updated•11 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•