]> git.ipfire.org Git - thirdparty/bugzilla.git/commitdiff
Bug 26257: [SECURITY] Bugzilla should prevent malicious webpages from making bugzilla...
authorlpsolit%gmail.com <>
Mon, 2 Feb 2009 18:42:01 +0000 (18:42 +0000)
committerlpsolit%gmail.com <>
Mon, 2 Feb 2009 18:42:01 +0000 (18:42 +0000)
Bugzilla/Install/Localconfig.pm
Bugzilla/Template.pm
Bugzilla/Token.pm
buglist.cgi
email_in.pl
process_bug.cgi
template/en/default/admin/confirm-action.html.tmpl
template/en/default/bug/edit.html.tmpl
template/en/default/global/confirm-action.html.tmpl [new file with mode: 0644]
template/en/default/list/edit-multiple.html.tmpl

index 7df9e073617bf9305e89f7533f29bc513552b5a5..28fac39fcae1429c7204d0beec9ee3915c620087 100644 (file)
@@ -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    => <<EOT
 # The interdiff feature needs diff, so we have to have that path.
 # Please specify the directory name only; do not use trailing slash.
+EOT
+    },
+    {
+        name    => 'site_wide_secret',
+        default => generate_random_password(256),
+        desc    => <<EOT
+# This secret key is used by your installation for the creation and
+# validation of encrypted tokens to prevent unsolicited changes,
+# such as bug changes. A random string is generated by default.
+# It's very important that this key is kept secret. It also must be
+# very long.
 EOT
     },
 );
index 68773d4e738a9c45aea3130193358cf1b732b3d6..1362ecae3e32f4f672dea68da11095ddfb3230f1 100644 (file)
@@ -41,6 +41,7 @@ use Bugzilla::Util;
 use Bugzilla::User;
 use Bugzilla::Error;
 use Bugzilla::Status;
+use Bugzilla::Token;
 use Bugzilla::Template::Parser;
 
 use Cwd qw(abs_path);
@@ -744,6 +745,9 @@ sub create {
                 return $docs_urlbase;
             },
 
+            # Allow templates to generate a token themselves.
+            'issue_hash_token' => \&Bugzilla::Token::issue_hash_token,
+
             # These don't work as normal constants.
             DB_MODULE        => \&Bugzilla::Constants::DB_MODULE,
             REQUIRED_MODULES => 
index 157cc0622c3051cd1011f77f3dd849d9d80a7977..a54da4af59848ab8dac507ac6efd9c939d8a07b7 100644 (file)
@@ -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();
index 206d9651d9f363e1e8c33dc25d825ef6d799c8ef..4e8782f44528bbd054833f32ab687dd105bc167f 100755 (executable)
@@ -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');
index 4f21eeab1697bef5e889d00e0da5aa57ff2ee493..e1ebd3e0c9ed11855389ddb033bd5285fba5d2a1 100644 (file)
@@ -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';
 }
index 7e484d2b04cc8cc9e6543ef5e2dc731e2c8afc2f..f21b1724e5230c1f62a1a4f01e906533662cb615 100755 (executable)
@@ -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 <link> elements
index da551d0d7e3fe646a3c5b3c7dae81e316f485146..521d2d157bf4a19b2f3c0701fe98dfacc1aa7682 100644 (file)
@@ -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)$" %]
     <input type="submit" id="confirm" value="Confirm Changes">
   </form>
-  <p>Or throw away these changes and go back to <a href="[% script_name FILTER html %]">
-    [%- script_name FILTER html %]</a>.</p>
+  <p>Or throw away these changes and go back to <a href="[% alternate_script FILTER html %]">
+    [%- alternate_script FILTER html %]</a>.</p>
 [% END %]
 
 [% PROCESS global/footer.html.tmpl %]
index 77470eb6c12b2abea72f331fcb9078a992b80446..82328b6e7e05845c179dd69633063a6fd21bd38f 100644 (file)
   <input type="hidden" name="delta_ts" value="[% bug.delta_ts %]">
   <input type="hidden" name="longdesclength" value="[% bug.longdescs.size %]">
   <input type="hidden" name="id" value="[% bug.bug_id %]">
+  <input type="hidden" name="token" value="[% issue_hash_token([bug.id, bug.delta_ts]) FILTER html %]">
 
   [% PROCESS section_title %]
   <table>
diff --git a/template/en/default/global/confirm-action.html.tmpl b/template/en/default/global/confirm-action.html.tmpl
new file mode 100644 (file)
index 0000000..e57a83c
--- /dev/null
@@ -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 <LpSolit@gmail.com>
+  #%]
+
+[%# 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'] %]
+
+<div class="throw_error">
+  [% 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 <b>without your consent</b>.
+
+  [% 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 <b>without your
+    consent</b>.
+  [% END %]
+  <p>
+    Are you sure you want to commit these changes?
+  </p>
+</div>
+
+<form name="check" id="check" method="post" action="[% script_name FILTER html %]">
+  [% PROCESS "global/hidden-fields.html.tmpl"
+             exclude="^(Bugzilla_login|Bugzilla_password|token)$" %]
+  <input type="hidden" name="token" value="[% token FILTER html %]">
+  <input type="submit" id="confirm" value="Yes, Confirm Changes">
+</form>
+
+<p><a href="index.cgi">No, throw away these changes</a> (you will be redirected
+to the home page).</p>
+
+[% PROCESS global/footer.html.tmpl %]
index 7c152e928ad1c1ecadc59f4c88550dbd3b7b1762..3e582b3a50f5f0e8f8afb311a7be4937aeaa591a 100644 (file)
@@ -25,6 +25,7 @@
 
 [% dontchange = "--do_not_change--" %]
 <input type="hidden" name="dontchange" value="[% dontchange FILTER html %]">
+<input type="hidden" name="token" value="[% token FILTER html %]">
 
 <script type="text/javascript">
   var numelements = document.forms.changeform.elements.length;