Closed Bug 312498 Opened 14 years ago Closed 14 years ago

[SECURITY] editparams.cgi doesn't check whether 'whinedays' and 'mostfreqthreshold' are numeric

Categories

(Bugzilla :: Administration, task, critical)

2.17.1
task
Not set
critical

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)

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.
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.22
Attached patch patch, v1 (obsolete) — Splinter Review
Attachment #199592 - Flags: review?(wicked)
'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
Attachment #199592 - Attachment is obsolete: true
Attachment #199597 - Flags: review?(wicked)
Attachment #199592 - Flags: review?(wicked)
ewwww....   SQL Injection.
Group: webtools-security
Severity: minor → critical
Flags: blocking2.20.1?
Flags: blocking2.18.4?
Flags: blocking2.16.11?
Target Milestone: Bugzilla 2.22 → Bugzilla 2.16
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.
Attachment #199597 - Attachment description: patch, v2 → patch for tip, v2
Attachment #199642 - Flags: review?(wicked)
Attachment #199646 - Flags: review?(wicked)
The SQL Injection can only be done by somebody with editparams, though, right?
So at least it's not coming through query params.
(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 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+
Attachment #199642 - Flags: review?(wicked) → review+
Attachment #199646 - Flags: review?(wicked) → review+
Flags: approval?
Flags: approval2.20?
Flags: approval2.18?
Flags: approval2.16?
Whiteboard: [ready for 2.16.11][ready for 2.18.5][ready for 2.20.1][ready for 2.21.2]
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-
LpSolit pointed out that whinedays does indeed exist in 2.18.
Flags: blocking2.18.4- → blocking2.18.4+
Flags: blocking2.22+
Flags: blocking2.18.5+
Flags: blocking2.18.4+
Flags: blocking2.16.11-
Flags: blocking2.16.11+
Thank you justdave for overwriting that blocking2.16.11 flag! :)
(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.
(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
Blocks: 320312
For the record, the check for whinedays has been accidentally removed by bug 163829 and affects all versions >= 2.17.1.
Version: 2.21 → 2.17.1
Flags: approval?
Flags: approval2.20?
Flags: approval2.20+
Flags: approval2.18?
Flags: approval2.18+
Flags: approval2.16?
Flags: approval2.16+
Flags: approval+
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: 14 years ago
Resolution: --- → FIXED
Security Advisory posted; unlocking bug.
Group: webtools-security
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.