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 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" %] + [% param FILTER html %] is not a valid parameter + for the [% function FILTER html %] function. + [% ELSIF error == "param_must_be_numeric" %] [% title = "Invalid Parameter" %] Invalid parameter [% param FILTER html %] passed to -- cgit v1.2.3-24-g4f1b