From dc51769c9f7fb84ac2e43112f2d106a4770f5781 Mon Sep 17 00:00:00 2001 From: "lpsolit%gmail.com" <> Date: Mon, 2 Feb 2009 18:33:29 +0000 Subject: 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 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Bugzilla/Install/Localconfig.pm | 13 ++++++ Bugzilla/Template.pm | 4 ++ Bugzilla/Token.pm | 54 +++++++++++++++++++++- buglist.cgi | 2 + email_in.pl | 2 + process_bug.cgi | 23 +++++++-- template/en/default/admin/confirm-action.html.tmpl | 6 ++- template/en/default/bug/edit.html.tmpl | 1 + template/en/default/list/edit-multiple.html.tmpl | 1 + 9 files changed, 98 insertions(+), 8 deletions(-) diff --git a/Bugzilla/Install/Localconfig.pm b/Bugzilla/Install/Localconfig.pm index 45005f032..654b44b9f 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,18 @@ 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 313d43212..f87490db1 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 @@ -170,6 +172,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 @@ -310,7 +359,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; @@ -330,6 +379,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 d51112a5c..f5284439c 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; @@ -1241,6 +1242,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 bed5a1477..1edce55d8 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 # @@ -201,6 +202,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 a0aadc1c5..83041230b 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); @@ -158,10 +159,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 @@ -184,6 +181,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) @@ -191,6 +190,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 da551d0d7..521d2d157 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 97a2bd54f..80c5745fc 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/list/edit-multiple.html.tmpl b/template/en/default/list/edit-multiple.html.tmpl index 6a62a80dc..46130ef6b 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--" %] +