From: lpsolit%gmail.com <> Date: Mon, 2 Feb 2009 18:42:01 +0000 (+0000) Subject: Bug 26257: [SECURITY] Bugzilla should prevent malicious webpages from making bugzilla... X-Git-Tag: bugzilla-3.2.1~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=add701585927ee2cc8b50ee0994e6b85353a4cbf;p=thirdparty%2Fbugzilla.git Bug 26257: [SECURITY] Bugzilla should prevent malicious webpages from making bugzilla users submit changes to bugs - Patch by Frédéric Buclin r=mkanat a=LpSolit --- diff --git a/Bugzilla/Install/Localconfig.pm b/Bugzilla/Install/Localconfig.pm index 7df9e07361..28fac39fca 100644 --- a/Bugzilla/Install/Localconfig.pm +++ b/Bugzilla/Install/Localconfig.pm @@ -32,6 +32,7 @@ use strict; use Bugzilla::Constants; use Bugzilla::Install::Util qw(bin_loc); +use Bugzilla::Util qw(generate_random_password); use Data::Dumper; use File::Basename qw(dirname); @@ -183,6 +184,17 @@ EOT desc => < 'site_wide_secret', + default => generate_random_password(256), + desc => < \&Bugzilla::Token::issue_hash_token, + # These don't work as normal constants. DB_MODULE => \&Bugzilla::Constants::DB_MODULE, REQUIRED_MODULES => diff --git a/Bugzilla/Token.pm b/Bugzilla/Token.pm index 157cc0622c..a54da4af59 100644 --- a/Bugzilla/Token.pm +++ b/Bugzilla/Token.pm @@ -39,10 +39,12 @@ use Bugzilla::User; use Date::Format; use Date::Parse; use File::Basename; +use Digest::MD5 qw(md5_hex); use base qw(Exporter); -@Bugzilla::Token::EXPORT = qw(issue_session_token check_token_data delete_token); +@Bugzilla::Token::EXPORT = qw(issue_session_token check_token_data delete_token + issue_hash_token check_hash_token); ################################################################################ # Public Functions @@ -171,6 +173,53 @@ sub issue_session_token { return _create_token(Bugzilla->user->id, 'session', $data); } +sub issue_hash_token { + my ($data, $time) = @_; + $data ||= []; + $time ||= time(); + + # The concatenated string is of the form + # token creation time + site-wide secret + user ID + data + my @args = ($time, Bugzilla->localconfig->{'site_wide_secret'}, Bugzilla->user->id, @$data); + my $token = md5_hex(join('*', @args)); + + # Prepend the token creation time, unencrypted, so that the token + # lifetime can be validated. + return $time . '-' . $token; +} + +sub check_hash_token { + my ($token, $data) = @_; + $data ||= []; + my ($time, $expected_token); + + if ($token) { + ($time, undef) = split(/-/, $token); + # Regenerate the token based on the information we have. + $expected_token = issue_hash_token($data, $time); + } + + if (!$token + || $expected_token ne $token + || time() - $time > MAX_TOKEN_AGE * 86400) + { + my $template = Bugzilla->template; + my $vars = {}; + $vars->{'script_name'} = basename($0); + $vars->{'token'} = issue_hash_token($data); + $vars->{'reason'} = (!$token) ? 'missing_token' : + ($expected_token ne $token) ? 'invalid_token' : + 'expired_token'; + print Bugzilla->cgi->header(); + $template->process('global/confirm-action.html.tmpl', $vars) + || ThrowTemplateError($template->error()); + exit; + } + + # If we come here, then the token is valid and not too old. + return 1; +} + sub CleanTokenTable { my $dbh = Bugzilla->dbh; $dbh->do('DELETE FROM tokens @@ -308,7 +357,7 @@ sub delete_token { # Note: this routine must not be called while tables are locked as it will try # to lock some tables itself, see CleanTokenTable(). sub check_token_data { - my ($token, $expected_action) = @_; + my ($token, $expected_action, $alternate_script) = @_; my $user = Bugzilla->user; my $template = Bugzilla->template; my $cgi = Bugzilla->cgi; @@ -328,6 +377,7 @@ sub check_token_data { $vars->{'token_action'} = $token_action; $vars->{'expected_action'} = $expected_action; $vars->{'script_name'} = basename($0); + $vars->{'alternate_script'} = $alternate_script || basename($0); # Now is a good time to remove old tokens from the DB. CleanTokenTable(); diff --git a/buglist.cgi b/buglist.cgi index 206d9651d9..4e8782f445 100755 --- a/buglist.cgi +++ b/buglist.cgi @@ -47,6 +47,7 @@ use Bugzilla::Product; use Bugzilla::Keyword; use Bugzilla::Field; use Bugzilla::Status; +use Bugzilla::Token; use Date::Parse; @@ -1185,6 +1186,7 @@ if ($dotweak && scalar @bugs) { } $vars->{'dotweak'} = 1; $vars->{'use_keywords'} = 1 if Bugzilla::Keyword::keyword_count(); + $vars->{'token'} = issue_session_token('buglist_mass_change'); $vars->{'products'} = Bugzilla->user->get_enterable_products; $vars->{'platforms'} = get_legal_field_values('rep_platform'); diff --git a/email_in.pl b/email_in.pl index 4f21eeab16..e1ebd3e0c9 100644 --- a/email_in.pl +++ b/email_in.pl @@ -47,6 +47,7 @@ use Bugzilla::Error; use Bugzilla::Mailer; use Bugzilla::User; use Bugzilla::Util; +use Bugzilla::Token; ############# # Constants # @@ -202,6 +203,7 @@ sub process_bug { $cgi->param(-name => $field, -value => $fields{$field}); } $cgi->param('longdesclength', scalar $bug->longdescs); + $cgi->param('token', issue_hash_token([$bug->id, $bug->delta_ts])); require 'process_bug.cgi'; } diff --git a/process_bug.cgi b/process_bug.cgi index 7e484d2b04..f21b1724e5 100755 --- a/process_bug.cgi +++ b/process_bug.cgi @@ -59,6 +59,7 @@ use Bugzilla::Component; use Bugzilla::Keyword; use Bugzilla::Flag; use Bugzilla::Status; +use Bugzilla::Token; use Storable qw(dclone); @@ -165,10 +166,6 @@ if (defined $cgi->param('dontchange')) { # reference to flags if $cgi->param('id') is undefined. Bugzilla::Flag::validate($cgi->param('id')); -###################################################################### -# End Data/Security Validation -###################################################################### - print $cgi->header() unless Bugzilla->usage_mode == USAGE_MODE_EMAIL; # Check for a mid-air collision. Currently this only works when updating @@ -191,6 +188,8 @@ if (defined $cgi->param('delta_ts') $vars->{'comments'} = Bugzilla::Bug::GetComments($first_bug->id, "oldest_to_newest"); $vars->{'bug'} = $first_bug; + # The token contains the old delta_ts. We need a new one. + $cgi->param('token', issue_hash_token([$first_bug->id, $first_bug->delta_ts])); # Warn the user about the mid-air collision and ask them what to do. $template->process("bug/process/midair.html.tmpl", $vars) @@ -198,6 +197,22 @@ if (defined $cgi->param('delta_ts') exit; } +# We couldn't do this check earlier as we first had to validate bug IDs +# and display the mid-air collision page if delta_ts changed. +# If we do a mass-change, we use session tokens. +my $token = $cgi->param('token'); + +if ($cgi->param('id')) { + check_hash_token($token, [$first_bug->id, $first_bug->delta_ts]); +} +else { + check_token_data($token, 'buglist_mass_change', 'query.cgi'); +} + +###################################################################### +# End Data/Security Validation +###################################################################### + $vars->{'title_tag'} = "bug_processed"; # Set up the vars for navigational elements diff --git a/template/en/default/admin/confirm-action.html.tmpl b/template/en/default/admin/confirm-action.html.tmpl index da551d0d7e..521d2d157b 100644 --- a/template/en/default/admin/confirm-action.html.tmpl +++ b/template/en/default/admin/confirm-action.html.tmpl @@ -20,6 +20,8 @@ # token_action: the action the token was supposed to serve. # expected_action: the action the user was going to do. # script_name: the script generating this warning. + # alternate_script: the suggested script to redirect the user to + # if he declines submission. #%] [% PROCESS "global/field-descs.none.tmpl" %] @@ -89,8 +91,8 @@ exclude="^(Bugzilla_login|Bugzilla_password)$" %] -

Or throw away these changes and go back to - [%- script_name FILTER html %].

+

Or throw away these changes and go back to + [%- alternate_script FILTER html %].

[% END %] [% PROCESS global/footer.html.tmpl %] diff --git a/template/en/default/bug/edit.html.tmpl b/template/en/default/bug/edit.html.tmpl index 77470eb6c1..82328b6e7e 100644 --- a/template/en/default/bug/edit.html.tmpl +++ b/template/en/default/bug/edit.html.tmpl @@ -144,6 +144,7 @@ + [% PROCESS section_title %] diff --git a/template/en/default/global/confirm-action.html.tmpl b/template/en/default/global/confirm-action.html.tmpl new file mode 100644 index 0000000000..e57a83c281 --- /dev/null +++ b/template/en/default/global/confirm-action.html.tmpl @@ -0,0 +1,63 @@ +[%# The contents of this file are subject to the Mozilla Public + # License Version 1.1 (the "License"); you may not use this file + # except in compliance with the License. You may obtain a copy of + # the License at http://www.mozilla.org/MPL/ + # + # Software distributed under the License is distributed on an "AS + # IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or + # implied. See the License for the specific language governing + # rights and limitations under the License. + # + # The Original Code is the Bugzilla Bug Tracking System. + # + # The Initial Developer of the Original Code is Frédéric Buclin. + # Portions created by Frédéric Buclin are Copyright (C) 2008 + # Frédéric Buclin. All Rights Reserved. + # + # Contributor(s): Frédéric Buclin + #%] + +[%# INTERFACE: + # script_name: the script generating this warning. + # token: a valid token for the current action. + # reason: reason of the failure. + #%] + +[% PROCESS global/header.html.tmpl title = "Suspicious Action" + style_urls = ['skins/standard/global.css'] %] + +
+ [% IF reason == "expired_token" %] + Your changes have been rejected because you exceeded the time limit + of [% constants.MAX_TOKEN_AGE FILTER html %] days before submitting your + changes to [% script_name FILTER html %]. Your page may have been displayed + for too long, or old changes have been resubmitted by accident. + + [% ELSIF reason == "missing_token" %] + It looks like you didn't come from the right page. + One reason could be that you entered the URL in the address bar of your + web browser directly, which should be safe. Another reason could be that + you clicked on a URL which redirected you here without your consent. + + [% ELSIF reason == "invalid_token" %] + You submitted changes to [% script_name FILTER html %] with an invalid + token, which may indicate that someone tried to abuse you, for instance + by making you click on a URL which redirected you here without your + consent. + [% END %] +

+ Are you sure you want to commit these changes? +

+
+ +
+ [% PROCESS "global/hidden-fields.html.tmpl" + exclude="^(Bugzilla_login|Bugzilla_password|token)$" %] + + + + +

No, throw away these changes (you will be redirected +to the home page).

+ +[% PROCESS global/footer.html.tmpl %] diff --git a/template/en/default/list/edit-multiple.html.tmpl b/template/en/default/list/edit-multiple.html.tmpl index 7c152e928a..3e582b3a50 100644 --- a/template/en/default/list/edit-multiple.html.tmpl +++ b/template/en/default/list/edit-multiple.html.tmpl @@ -25,6 +25,7 @@ [% dontchange = "--do_not_change--" %] +