From 4b93a777a8031b46ffcbdf21793acbb76dceadd9 Mon Sep 17 00:00:00 2001 From: "lpsolit%gmail.com" <> Date: Mon, 2 Feb 2009 18:51:52 +0000 Subject: [PATCH] =?utf8?q?Bug=20466748:=20[SECURITY]=20Shared/saved=20sear?= =?utf8?q?ches=20can=20be=20deleted=20without=20user=20confirmation=20usin?= =?utf8?q?g=20predictable=20URL=20-=20Patch=20by=20Fr=C3=83=C2=A9d=C3=83?= =?utf8?q?=C2=A9ric=20Buclin=20=20r=3Dmkanat=20a=3DLpSo?= =?utf8?q?lit?= MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit --- Bugzilla/Install/Localconfig.pm | 12 ++++ Bugzilla/Template.pm | 6 +- Bugzilla/Token.pm | 51 ++++++++++++++- buglist.cgi | 20 ++++-- .../account/prefs/saved-searches.html.tmpl | 3 +- .../default/global/confirm-action.html.tmpl | 64 +++++++++++++++++++ .../en/default/global/user-error.html.tmpl | 5 +- template/en/default/list/list.html.tmpl | 5 +- 8 files changed, 154 insertions(+), 12 deletions(-) create mode 100644 template/en/default/global/confirm-action.html.tmpl diff --git a/Bugzilla/Install/Localconfig.pm b/Bugzilla/Install/Localconfig.pm index ed502d8a77..ce94f5b838 100644 --- a/Bugzilla/Install/Localconfig.pm +++ b/Bugzilla/Install/Localconfig.pm @@ -31,6 +31,7 @@ package Bugzilla::Install::Localconfig; use strict; use Bugzilla::Constants; +use Bugzilla::Util qw(generate_random_password); use Data::Dumper; use IO::File; @@ -181,6 +182,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 62aeb30869..f26f623c0a 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->bz_lock_tables('tokens WRITE'); diff --git a/buglist.cgi b/buglist.cgi index ca693dfb3a..afe16ae4a6 100755 --- a/buglist.cgi +++ b/buglist.cgi @@ -46,6 +46,7 @@ use Bugzilla::Bug; use Bugzilla::Product; use Bugzilla::Keyword; use Bugzilla::Field; +use Bugzilla::Token; use Date::Parse; @@ -273,7 +274,7 @@ sub LookupNamedQuery { $result || ThrowUserError("buglist_parameters_required", {'queryname' => $name}); - return $result; + return wantarray ? ($result, $id) : $result; } # Inserts a Named Query (a "Saved Search") into the database, or @@ -405,14 +406,16 @@ if ($cgi->param('cmdtype') eq "dorem" && $cgi->param('remaction') =~ /^run/) { # Take appropriate action based on user's request. if ($cgi->param('cmdtype') eq "dorem") { if ($cgi->param('remaction') eq "run") { - $buffer = LookupNamedQuery(scalar $cgi->param("namedcmd"), - scalar $cgi->param('sharer_id')); + my $query_id; + ($buffer, $query_id) = LookupNamedQuery(scalar $cgi->param("namedcmd"), + scalar $cgi->param('sharer_id')); # If this is the user's own query, remember information about it # so that it can be modified easily. $vars->{'searchname'} = $cgi->param('namedcmd'); if (!$cgi->param('sharer_id') || $cgi->param('sharer_id') == Bugzilla->user->id) { $vars->{'searchtype'} = "saved"; + $vars->{'search_id'} = $query_id; } $params = new Bugzilla::CGI($buffer); $order = $params->param('order') || $order; @@ -461,6 +464,10 @@ if ($cgi->param('cmdtype') eq "dorem") { # The user has no query of this name. Play along. } else { + # Make sure the user really wants to delete his saved search. + my $token = $cgi->param('token'); + check_hash_token($token, [$query_id, $qname]); + $dbh->do('DELETE FROM namedqueries WHERE id = ?', undef, $query_id); @@ -514,9 +521,12 @@ elsif (($cgi->param('cmdtype') eq "doit") && defined $cgi->param('remtype')) { my %bug_ids; my $is_new_name = 0; if ($query_name) { + my ($query, $query_id) = + LookupNamedQuery($query_name, undef, QUERY_LIST, !THROW_ERROR); # Make sure this name is not already in use by a normal saved search. - if (LookupNamedQuery($query_name, undef, QUERY_LIST, !THROW_ERROR)) { - ThrowUserError('query_name_exists', {'name' => $query_name}); + if ($query) { + ThrowUserError('query_name_exists', {name => $query_name, + query_id => $query_id}); } $is_new_name = 1; } diff --git a/template/en/default/account/prefs/saved-searches.html.tmpl b/template/en/default/account/prefs/saved-searches.html.tmpl index 4beb0e8231..38e787c8df 100644 --- a/template/en/default/account/prefs/saved-searches.html.tmpl +++ b/template/en/default/account/prefs/saved-searches.html.tmpl @@ -118,7 +118,8 @@ Remove from whining first [% ELSE %] Forget + [% q.name FILTER url_quote %]&token= + [% issue_hash_token([q.id, q.name]) FILTER url_quote %]">Forget [% END %] 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..998551dcee --- /dev/null +++ b/template/en/default/global/confirm-action.html.tmpl @@ -0,0 +1,64 @@ +[%# 1.0@bugzilla.org %] +[%# 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/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index 1ded511021..e1a4bcf910 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/template/en/default/global/user-error.html.tmpl @@ -1269,8 +1269,9 @@ The name [% name FILTER html %] is already used by another saved search. You first have to delete it if you really want to use - this name. + [%- name FILTER url_quote %]&token= + [% issue_hash_token([query_id, name]) FILTER url_quote %]">delete + it if you really want to use this name. [% ELSIF error == "query_name_missing" %] [% title = "No Search Name Specified" %] diff --git a/template/en/default/list/list.html.tmpl b/template/en/default/list/list.html.tmpl index a12c39a9ec..757bb4787d 100644 --- a/template/en/default/list/list.html.tmpl +++ b/template/en/default/list/list.html.tmpl @@ -205,8 +205,9 @@ | Forget Search ' - [% searchname FILTER html %]' + [% searchname FILTER url_quote %]&token= + [% issue_hash_token([search_id, searchname]) FILTER url_quote %]"> + Forget Search '[% searchname FILTER html %]' [% ELSE %]   -- 2.47.2