Closed
Bug 312498
Opened 19 years ago
Closed 18 years ago
[SECURITY] editparams.cgi doesn't check whether 'whinedays' and 'mostfreqthreshold' are numeric
Categories
(Bugzilla :: Administration, task)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.16
People
(Reporter: LpSolit, Assigned: LpSolit)
References
Details
(Whiteboard: [ready for 2.16.11][ready for 2.18.5][ready for 2.20.1][ready for 2.21.2])
Attachments
(3 files, 1 obsolete file)
962 bytes,
patch
|
wicked
:
review+
|
Details | Diff | Splinter Review |
794 bytes,
patch
|
wicked
:
review+
|
Details | Diff | Splinter Review |
1.29 KB,
patch
|
wicked
:
review+
|
Details | Diff | Splinter Review |
Inadvertently writting '4r' instead of '4' for the 'whinedays' param caused whineatnews.pl to crash: whineatnews.pl: DBD::mysql::st execute failed: Unknown column '4r' in 'where clause' [for Statement "SELECT bug_id, short_desc, login_name FROM bugs INNER JOIN profiles ON userid = assigned_to WHERE (bug_status = 'NEW' OR bug_status = 'REOPENED') AND TO_DAYS(NOW()) - TO_DAYS(delta_ts) > 4r ORDER BY bug_id"] at Bugzilla/DB.pm line 84 editparams.cgi should make sure that a valid numeric value is given.
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.22
Assignee | ||
Comment 1•19 years ago
|
||
Attachment #199592 -
Flags: review?(wicked)
Assignee | ||
Comment 2•19 years ago
|
||
'mostfreqthreshold' validation is also missing
Summary: editparams.cgi doesn't check whether 'whinedays' is numeric → editparams.cgi doesn't check whether 'whinedays' and 'mostfreqthreshold' are numeric
Assignee | ||
Comment 3•19 years ago
|
||
Attachment #199592 -
Attachment is obsolete: true
Attachment #199597 -
Flags: review?(wicked)
Assignee | ||
Updated•19 years ago
|
Attachment #199592 -
Flags: review?(wicked)
Assignee | ||
Updated•19 years ago
|
Severity: minor → critical
Flags: blocking2.20.1?
Flags: blocking2.18.4?
Flags: blocking2.16.11?
Target Milestone: Bugzilla 2.22 → Bugzilla 2.16
Assignee | ||
Comment 5•19 years ago
|
||
Looking at defparams.pl for all branches, here are the missing validations I found: 2.16.10: 'mostfreqthreshold', used in duplicates.cgi only: my $threshold = Param("mostfreqthreshold"); while (my ($key, $value) = each %count) { delete $count{$key} if ($value < $threshold); } 'maxpatchsize' and 'maxattachmentsize', used in attachment.cgi only: my $maxpatchsize = Param('maxpatchsize'); my $maxattachmentsize = Param('maxattachmentsize'); if ( $::FORM{'ispatch'} && $maxpatchsize && $len > $maxpatchsize*1024 ) { ... } elsif ( !$::FORM{'ispatch'} && $maxattachmentsize && $len > $maxattachmentsize*1024 ) { ... } So nothing critical for 2.16.10; except that some parameters are expected to be numeric while a string could be passed. In the worst case, Bugzilla would crash. 2.18.4, 2.20 and tip: 'mostfreqthreshold', used in duplicates.cgi only; see above. 'whinedays', used in whineatnews.pl: exit unless Param('whinedays') >= 1; SendSQL("select bug_id,short_desc,login_name from bugs,profiles where " . "(bug_status = 'NEW' or bug_status = 'REOPENED') and " . "to_days(now()) - to_days(delta_ts) > " . Param('whinedays') . " and userid=assigned_to order by bug_id"); Here again, 'mostfreqthreshold' isn't critical, but 'whinedays' is, due to the possible SQL injection.
Assignee | ||
Updated•19 years ago
|
Attachment #199597 -
Attachment description: patch, v2 → patch for tip, v2
Assignee | ||
Comment 6•19 years ago
|
||
Attachment #199642 -
Flags: review?(wicked)
Assignee | ||
Comment 7•19 years ago
|
||
Attachment #199646 -
Flags: review?(wicked)
Comment 8•19 years ago
|
||
The SQL Injection can only be done by somebody with editparams, though, right? So at least it's not coming through query params.
Comment 9•19 years ago
|
||
(In reply to comment #8) > The SQL Injection can only be done by somebody with editparams, though, right? > So at least it's not coming through query params. All you'd have to do is link to doeditparams.cgi with the right URL params to make that change, and load it in a hidden iframe on some other site said person with editparams is likely to visit. Since it's in a hidden iframe, they'd never see the result saying the param had been changed. There's other bugs open on fixing up form submission to check better if it really came from Bugzilla or not, but until that's fixed, this is still a real problem.
Comment 10•19 years ago
|
||
Comment on attachment 199597 [details] [diff] [review] patch for tip, v2 Now the mentioned params are validated to contain a zero or a positive number. Otherwise an error is thrown.
Attachment #199597 -
Flags: review?(wicked) → review+
Updated•19 years ago
|
Attachment #199642 -
Flags: review?(wicked) → review+
Updated•19 years ago
|
Attachment #199646 -
Flags: review?(wicked) → review+
Updated•19 years ago
|
Flags: approval?
Flags: approval2.20?
Flags: approval2.18?
Flags: approval2.16?
Assignee | ||
Updated•19 years ago
|
Whiteboard: [ready for 2.16.11][ready for 2.18.5][ready for 2.20.1][ready for 2.21.2]
Comment 11•19 years ago
|
||
2.18 and 2.16 are locked-down to security fixes only, and the patches for 2.16 and 2.18 here are not technically security fixes, only bug fixes. However, 2.20 has a security hole here, and so that needs to be handled. I don't understand how we can have one patch for both 2.18 and 2.20, when there is no Whining in 2.18.
Flags: blocking2.20.1?
Flags: blocking2.20.1+
Flags: blocking2.18.4?
Flags: blocking2.18.4-
Flags: blocking2.16.11?
Flags: blocking2.16.11-
Comment 12•19 years ago
|
||
LpSolit pointed out that whinedays does indeed exist in 2.18.
Flags: blocking2.18.4- → blocking2.18.4+
Updated•19 years ago
|
Flags: blocking2.22+
Flags: blocking2.18.5+
Flags: blocking2.18.4+
Flags: blocking2.16.11-
Flags: blocking2.16.11+
Comment 13•19 years ago
|
||
Thank you justdave for overwriting that blocking2.16.11 flag! :)
Comment 14•19 years ago
|
||
(In reply to comment #12) > LpSolit pointed out that whinedays does indeed exist in 2.18. It exists in 2.16, too. That's part of the old hard-coded whining, and I don't think the new whining stuff even uses it.
Assignee | ||
Comment 15•19 years ago
|
||
(In reply to comment #14) > It exists in 2.16, too. That's part of the old hard-coded whining, and I don't > think the new whining stuff even uses it. 'whinedays' is used by ./whineatnews.pl
Assignee | ||
Comment 16•18 years ago
|
||
For the record, the check for whinedays has been accidentally removed by bug 163829 and affects all versions >= 2.17.1.
Updated•18 years ago
|
Version: 2.21 → 2.17.1
Updated•18 years ago
|
Flags: approval?
Flags: approval2.20?
Flags: approval2.20+
Flags: approval2.18?
Flags: approval2.18+
Flags: approval2.16?
Flags: approval2.16+
Flags: approval+
Assignee | ||
Comment 17•18 years ago
|
||
tip: Checking in Bugzilla/Config/MTA.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/MTA.pm,v <-- MTA.pm new revision: 1.3; previous revision: 1.2 done Checking in Bugzilla/Config/Query.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Query.pm,v <-- Query.pm new revision: 1.3; previous revision: 1.2 done 2.20: Checking in defparams.pl; /cvsroot/mozilla/webtools/bugzilla/Attic/defparams.pl,v <-- defparams.pl new revision: 1.160.2.4; previous revision: 1.160.2.3 done 2.18.4: Checking in defparams.pl; /cvsroot/mozilla/webtools/bugzilla/Attic/defparams.pl,v <-- defparams.pl new revision: 1.128.2.5; previous revision: 1.128.2.4 done 2.16.10: Checking in defparams.pl; /cvsroot/mozilla/webtools/bugzilla/Attic/defparams.pl,v <-- defparams.pl new revision: 1.73.2.11; previous revision: 1.73.2.10 done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•18 years ago
|
Summary: editparams.cgi doesn't check whether 'whinedays' and 'mostfreqthreshold' are numeric → [SECURITY] editparams.cgi doesn't check whether 'whinedays' and 'mostfreqthreshold' are numeric
You need to log in
before you can comment on or make changes to this bug.
Description
•