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

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Frédéric Buclin]

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.

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

Comment 2

[Frédéric Buclin]

'mostfreqthreshold' validation is also missing

Comment 3

[Frédéric Buclin]

Attachment: patch for tip, v2

Comment 4

[Dave Miller [:justdave]]

ewwww....   SQL Injection.

Comment 5

[Frédéric Buclin]

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.

Comment 6

[Frédéric Buclin]

Attachment: backport for 2.18 and 2.20, v1

Comment 7

[Frédéric Buclin]

Attachment: backport for 2.16, v1

Comment 8

[Max Kanat-Alexander]

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

[Dave Miller [:justdave]]

(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

[Teemu Mannermaa (:wicked)]

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.

Comment 11

[Max Kanat-Alexander]

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.

Comment 12

[Max Kanat-Alexander]

LpSolit pointed out that whinedays does indeed exist in 2.18.

Comment 13

[Vlad Dascalu]

Thank you justdave for overwriting that blocking2.16.11 flag! :)

Comment 14

[Dave Miller [:justdave]]

(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.

Comment 15

[Frédéric Buclin]

(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

Comment 16

[Frédéric Buclin]

For the record, the check for whinedays has been accidentally removed by bug 163829 and affects all versions >= 2.17.1.

Comment 17

[Frédéric Buclin]

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

Comment 18

[Max Kanat-Alexander]

Security Advisory posted; unlocking bug.