From 406dea639a1d4dde19b39f69c29ebed687d76c52 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Fr=C3=A9d=C3=A9ric=20Buclin?= Date: Mon, 6 Oct 2014 14:22:46 +0000 Subject: [PATCH] Bug 1075578: [SECURITY] Improper filtering of CGI arguments r=dkl,a=sgreen --- Bugzilla/Attachment.pm | 13 ++++---- Bugzilla/Chart.pm | 7 ++--- attachment.cgi | 10 +++--- buglist.cgi | 2 +- editgroups.cgi | 4 +-- post_bug.cgi | 11 ++++--- query.cgi | 4 +-- relogin.cgi | 31 +++++++++---------- template/en/default/filterexceptions.pl | 1 - template/en/default/global/messages.html.tmpl | 2 +- token.cgi | 2 +- 11 files changed, 44 insertions(+), 43 deletions(-) diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm index 3f6b5d9230..2fa9dfdbb8 100644 --- a/Bugzilla/Attachment.pm +++ b/Bugzilla/Attachment.pm @@ -988,10 +988,12 @@ sub get_content_type { return 'text/plain' if ($cgi->param('ispatch') || $cgi->param('attachurl')); my $content_type; - if (!defined $cgi->param('contenttypemethod')) { + my $method = $cgi->param('contenttypemethod'); + + if (!defined $method) { ThrowUserError("missing_content_type_method"); } - elsif ($cgi->param('contenttypemethod') eq 'autodetect') { + elsif ($method eq 'autodetect') { defined $cgi->upload('data') || ThrowUserError('file_not_specified'); # The user asked us to auto-detect the content type, so use the type # specified in the HTTP request headers. @@ -1012,18 +1014,17 @@ sub get_content_type { $content_type = 'image/png'; } } - elsif ($cgi->param('contenttypemethod') eq 'list') { + elsif ($method eq 'list') { # The user selected a content type from the list, so use their # selection. $content_type = $cgi->param('contenttypeselection'); } - elsif ($cgi->param('contenttypemethod') eq 'manual') { + elsif ($method eq 'manual') { # The user entered a content type manually, so use their entry. $content_type = $cgi->param('contenttypeentry'); } else { - ThrowCodeError("illegal_content_type_method", - { contenttypemethod => $cgi->param('contenttypemethod') }); + ThrowCodeError("illegal_content_type_method", { contenttypemethod => $method }); } return $content_type; } diff --git a/Bugzilla/Chart.pm b/Bugzilla/Chart.pm index dfbf32a51d..8fd4706e45 100644 --- a/Bugzilla/Chart.pm +++ b/Bugzilla/Chart.pm @@ -110,10 +110,9 @@ sub init { if ($self->{'datefrom'} && $self->{'dateto'} && $self->{'datefrom'} > $self->{'dateto'}) { - ThrowUserError("misarranged_dates", - {'datefrom' => $cgi->param('datefrom'), - 'dateto' => $cgi->param('dateto')}); - } + ThrowUserError('misarranged_dates', { 'datefrom' => scalar $cgi->param('datefrom'), + 'dateto' => scalar $cgi->param('dateto') }); + } } # Alter Chart so that the selected series are added to it. diff --git a/attachment.cgi b/attachment.cgi index 2706cde246..a6bda6a357 100755 --- a/attachment.cgi +++ b/attachment.cgi @@ -228,8 +228,9 @@ sub validateContext { my $context = $cgi->param('context') || "patch"; if ($context ne "file" && $context ne "patch") { - detaint_natural($context) - || ThrowUserError("invalid_context", { context => $cgi->param('context') }); + my $orig_context = $context; + detaint_natural($context) + || ThrowUserError("invalid_context", { context => $orig_context }); } return $context; @@ -547,13 +548,14 @@ sub insert { # Get the filehandle of the attachment. my $data_fh = $cgi->upload('data'); + my $attach_url = $cgi->param('attachurl'); my $attachment = Bugzilla::Attachment->create( {bug => $bug, creation_ts => $timestamp, - data => scalar $cgi->param('attachurl') || $data_fh, + data => $attach_url || $data_fh, description => scalar $cgi->param('description'), - filename => $cgi->param('attachurl') ? '' : scalar $cgi->upload('data'), + filename => $attach_url ? '' : $data_fh, ispatch => scalar $cgi->param('ispatch'), isprivate => scalar $cgi->param('isprivate'), isurl => scalar $cgi->param('attachurl'), diff --git a/buglist.cgi b/buglist.cgi index 931259e27b..7c223ea2d6 100755 --- a/buglist.cgi +++ b/buglist.cgi @@ -1083,7 +1083,7 @@ if (scalar(@products) == 1) { # This is used in the "Zarroo Boogs" case. elsif (my @product_input = $cgi->param('product')) { if (scalar(@product_input) == 1 and $product_input[0] ne '') { - $one_product = new Bugzilla::Product({ name => $cgi->param('product') }); + $one_product = new Bugzilla::Product({ name => $product_input[0] }); } } # We only want the template to use it if the user can actually diff --git a/editgroups.cgi b/editgroups.cgi index a879aa7702..ccd0bd4329 100755 --- a/editgroups.cgi +++ b/editgroups.cgi @@ -242,7 +242,7 @@ if ($action eq 'new') { if ($action eq 'del') { # Check that an existing group ID is given - my $group = Bugzilla::Group->check({ id => $cgi->param('group') }); + my $group = Bugzilla::Group->check({ id => scalar $cgi->param('group') }); $group->check_remove({ test_only => 1 }); $vars->{'shared_queries'} = $dbh->selectrow_array('SELECT COUNT(*) @@ -266,7 +266,7 @@ if ($action eq 'del') { if ($action eq 'delete') { check_token_data($token, 'delete_group'); # Check that an existing group ID is given - my $group = Bugzilla::Group->check({ id => $cgi->param('group') }); + my $group = Bugzilla::Group->check({ id => scalar $cgi->param('group') }); $vars->{'name'} = $group->name; $group->remove_from_db({ remove_from_users => scalar $cgi->param('removeusers'), diff --git a/post_bug.cgi b/post_bug.cgi index 7a070a7563..86ffe8c6b7 100755 --- a/post_bug.cgi +++ b/post_bug.cgi @@ -186,7 +186,10 @@ if (defined $cgi->param('version')) { # after the bug is filed. # Add an attachment if requested. -if (defined($cgi->upload('data')) || $cgi->param('attachurl')) { +my $data_fh = $cgi->upload('data'); +my $attach_url = $cgi->param('attachurl'); + +if ($data_fh || $attach_url) { $cgi->param('isprivate', $cgi->param('comment_is_private')); # Must be called before create() as it may alter $cgi->param('ispatch'). @@ -201,12 +204,12 @@ if (defined($cgi->upload('data')) || $cgi->param('attachurl')) { $attachment = Bugzilla::Attachment->create( {bug => $bug, creation_ts => $timestamp, - data => scalar $cgi->param('attachurl') || $cgi->upload('data'), + data => $attach_url || $data_fh, description => scalar $cgi->param('description'), - filename => $cgi->param('attachurl') ? '' : scalar $cgi->upload('data'), + filename => $attach_url ? '' : $data_fh, ispatch => scalar $cgi->param('ispatch'), isprivate => scalar $cgi->param('isprivate'), - isurl => scalar $cgi->param('attachurl'), + isurl => $attach_url, mimetype => $content_type, store_in_file => scalar $cgi->param('bigfile'), }); diff --git a/query.cgi b/query.cgi index e10a698a3b..405c7b70c1 100755 --- a/query.cgi +++ b/query.cgi @@ -269,8 +269,8 @@ for (my $chart = 0; $cgi->param("field$chart-0-0"); $chart++) { if (!defined($value)) { $value = ''; } - push(@cols, { field => $cgi->param("field$chart-$row-$col"), - type => $cgi->param("type$chart-$row-$col") || 'noop', + push(@cols, { field => scalar $cgi->param("field$chart-$row-$col"), + type => scalar $cgi->param("type$chart-$row-$col") || 'noop', value => $value }); } push(@rows, \@cols); diff --git a/relogin.cgi b/relogin.cgi index d31c809932..29dd4731ad 100755 --- a/relogin.cgi +++ b/relogin.cgi @@ -87,19 +87,21 @@ elsif ($action eq 'begin-sudo') { { $credentials_provided = 1; } - + # Next, log in the user my $user = Bugzilla->login(LOGIN_REQUIRED); - + + my $target_login = $cgi->param('target_login'); + my $reason = $cgi->param('reason') || ''; + # At this point, the user is logged in. However, if they used a method # where they could have provided a username/password (i.e. CGI), but they # did not provide a username/password, then throw an error. if ($user->authorizer->can_login && !$credentials_provided) { ThrowUserError('sudo_password_required', - { target_login => $cgi->param('target_login'), - reason => $cgi->param('reason')}); + { target_login => $target_login, reason => $reason }); } - + # The user must be in the 'bz_sudoers' group unless ($user->in_group('bz_sudoers')) { ThrowUserError('auth_failure', { group => 'bz_sudoers', @@ -123,30 +125,22 @@ elsif ($action eq 'begin-sudo') { && ($token_data eq 'sudo_prepared')) { ThrowUserError('sudo_preparation_required', - { target_login => scalar $cgi->param('target_login'), - reason => scalar $cgi->param('reason')}); + { target_login => $target_login, reason => $reason }); } delete_token($cgi->param('token')); # Get & verify the target user (the user who we will be impersonating) - my $target_user = - new Bugzilla::User({ name => $cgi->param('target_login') }); + my $target_user = new Bugzilla::User({ name => $target_login }); unless (defined($target_user) && $target_user->id && $user->can_see_user($target_user)) { - ThrowUserError('user_match_failed', - { 'name' => $cgi->param('target_login') } - ); + ThrowUserError('user_match_failed', { name => $target_login }); } if ($target_user->in_group('bz_sudo_protect')) { ThrowUserError('sudo_protected', { login => $target_user->login }); } - # If we have a reason passed in, keep it under 200 characters - my $reason = $cgi->param('reason') || ''; - $reason = substr($reason, $[, 200); - # Calculate the session expiry time (T + 6 hours) my $time_string = time2str('%a, %d-%b-%Y %T %Z', time + MAX_SUDO_TOKEN_AGE, 'GMT'); @@ -159,9 +153,12 @@ elsif ($action eq 'begin-sudo') { # For the present, change the values of Bugzilla::user & Bugzilla::sudoer Bugzilla->sudo_request($target_user, $user); - + # NOTE: If you want to log the start of an sudo session, do it here. + # If we have a reason passed in, keep it under 200 characters + $reason = substr($reason, $[, 200); + # Go ahead and send out the message now my $message; my $mail_template = Bugzilla->template_inner($target_user->settings->{'lang'}->{'value'}); diff --git a/template/en/default/filterexceptions.pl b/template/en/default/filterexceptions.pl index e04e9de147..6928e79f83 100644 --- a/template/en/default/filterexceptions.pl +++ b/template/en/default/filterexceptions.pl @@ -197,7 +197,6 @@ ], 'global/messages.html.tmpl' => [ - 'message_tag', 'series.frequency * 2', ], diff --git a/template/en/default/global/messages.html.tmpl b/template/en/default/global/messages.html.tmpl index 52502a1d75..9fd92a0b7c 100644 --- a/template/en/default/global/messages.html.tmpl +++ b/template/en/default/global/messages.html.tmpl @@ -846,7 +846,7 @@ [% IF !message %] [% message = BLOCK %] You are using [% terms.Bugzilla %]'s messaging functions incorrectly. You - passed in the string '[% message_tag %]'. The correct use is to pass + passed in the string '[% message_tag FILTER html %]'. The correct use is to pass in a tag, and define that tag in the file messages.html.tmpl.

If you are a [% terms.Bugzilla %] end-user seeing this message, please diff --git a/token.cgi b/token.cgi index 65b0df8bf3..a1f0f7098f 100755 --- a/token.cgi +++ b/token.cgi @@ -377,7 +377,7 @@ sub confirm_create_account { my $otheruser = Bugzilla::User->create({ login_name => $login_name, - realname => $cgi->param('realname'), + realname => scalar $cgi->param('realname'), cryptpassword => $password}); # Now delete this token. -- 2.47.2