From 7380ea9ae11764633a4b6e64850da2d84b2aaeb2 Mon Sep 17 00:00:00 2001
From: "mkanat%bugzilla.org" <>
Date: Fri, 11 Sep 2009 16:10:13 +0000
Subject: Bug 515191: [SECURITY] SQL Injection via Bug.search (CVE-2009-3125)
 and Bug.create (CVE-2009-3165) Patch by Max Kanat-Alexander
 <mkanat@bugzilla.org> r=LpSolit, a=mkanat

---
 Bugzilla/Object.pm                              | 28 ++++++++++++++++++++++++-
 Bugzilla/WebService/Bug.pm                      |  1 +
 Bugzilla/WebService/Constants.pm                |  2 ++
 template/en/default/global/code-error.html.tmpl |  5 +++++
 4 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index bb8b45d76..456888b38 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -169,7 +169,19 @@ sub match {
         next if $field eq 'OFFSET';
         if ( $field eq 'LIMIT' ) {
             next unless defined $value;
-            $postamble = $dbh->sql_limit( $value, $criteria->{OFFSET} );
+            detaint_natural($value)
+              or ThrowCodeError('param_must_be_numeric', 
+                                { param    => 'LIMIT', 
+                                  function => "${class}::match" });
+            my $offset;
+            if (defined $criteria->{OFFSET}) {
+                $offset = $criteria->{OFFSET};
+                detaint_signed($offset)
+                  or ThrowCodeError('param_must_be_numeric', 
+                                    { param    => 'OFFSET',
+                                      function => "${class}::match" });
+            }
+            $postamble = $dbh->sql_limit($value, $offset);
             next;
         }
         elsif ( $field eq 'WHERE' ) {
@@ -185,6 +197,8 @@ sub match {
             next;
         }
         
+        $class->_check_field($field, 'match');
+
         if (ref $value eq 'ARRAY') {
             # IN () is invalid SQL, and if we have an empty list
             # to match against, we're just returning an empty
@@ -364,6 +378,17 @@ sub create {
     return $object;
 }
 
+# Used to validate that a field name is in fact a valid column in the
+# current table before inserting it into SQL.
+sub _check_field {
+    my ($invocant, $field, $function) = @_;
+    my $class = ref($invocant) || $invocant;
+    if (!Bugzilla->dbh->bz_column_info($class->DB_TABLE, $field)) {
+        ThrowCodeError('param_invalid', { param    => $field,
+                                          function => "${class}::$function" });
+    }
+}
+
 sub check_required_create_fields {
     my ($class, $params) = @_;
 
@@ -406,6 +431,7 @@ sub insert_create_data {
 
     my (@field_names, @values);
     while (my ($field, $value) = each %$field_values) {
+        $class->_check_field($field, 'create');
         push(@field_names, $field);
         push(@values, $value);
     }
diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm
index c6d620976..44382e79f 100644
--- a/Bugzilla/WebService/Bug.pm
+++ b/Bugzilla/WebService/Bug.pm
@@ -258,6 +258,7 @@ sub search {
     }
     
     $params = _map_fields($params);
+    delete $params->{WHERE};
     
     # Do special search types for certain fields.
     if ( my $bug_when = delete $params->{delta_ts} ) {
diff --git a/Bugzilla/WebService/Constants.pm b/Bugzilla/WebService/Constants.pm
index bdfe24f0a..7fd7e2ae8 100644
--- a/Bugzilla/WebService/Constants.pm
+++ b/Bugzilla/WebService/Constants.pm
@@ -53,7 +53,9 @@ use constant WS_ERROR_CODE => {
     param_required              => 50,
     params_required             => 50,
     object_does_not_exist       => 51,
+    param_must_be_numeric       => 52,
     xmlrpc_invalid_value        => 52,
+    param_invalid               => 53,
     # Bug errors usually occupy the 100-200 range.
     improper_bug_id_field_value => 100,
     bug_id_does_not_exist       => 101,
diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl
index e96e6d48e..4ba5e647a 100644
--- a/template/en/default/global/code-error.html.tmpl
+++ b/template/en/default/global/code-error.html.tmpl
@@ -338,6 +338,11 @@
     There is no valid transition from
     [%+ get_status("UNCONFIRMED") FILTER html %] to an open state.
 
+  [% ELSIF error == "param_invalid" %]
+    [% title = "Invalid Parameter" %]
+    <code>[% param FILTER html %]</code> is not a valid parameter
+    for the [% function FILTER html %] function.
+
   [% ELSIF error == "param_must_be_numeric" %]
     [% title = "Invalid Parameter" %]
     Invalid parameter <code>[% param FILTER html %]</code> passed to 
-- 
cgit v1.2.3-24-g4f1b