From 80882f085e8918346ddb0ec3250f0d31ddaba5e6 Mon Sep 17 00:00:00 2001 From: Frédéric Buclin Date: Tue, 22 Nov 2011 22:06:00 +0100 Subject: Bug 703975: CSRF vulnerability in post_bug.cgi allows possible unauthorized bug creation r=mkanat a=LpSolit --- enter_bug.cgi | 2 +- post_bug.cgi | 35 ++----------- process_bug.cgi | 3 ++ .../bug/create/confirm-create-dupe.html.tmpl | 57 ---------------------- 4 files changed, 8 insertions(+), 89 deletions(-) delete mode 100644 template/en/default/bug/create/confirm-create-dupe.html.tmpl diff --git a/enter_bug.cgi b/enter_bug.cgi index ffba2b09f..4778e4418 100755 --- a/enter_bug.cgi +++ b/enter_bug.cgi @@ -395,7 +395,7 @@ $vars->{'qa_contact_disabled'} = !$has_editbugs; $vars->{'cloned_bug_id'} = $cloned_bug_id; -$vars->{'token'} = issue_session_token('createbug:'); +$vars->{'token'} = issue_session_token('create_bug'); my @enter_bug_fields = grep { $_->enter_bug } Bugzilla->active_custom_fields; diff --git a/post_bug.cgi b/post_bug.cgi index 6ca46fb3c..c0878b0da 100755 --- a/post_bug.cgi +++ b/post_bug.cgi @@ -62,30 +62,7 @@ unless ($cgi->param()) { # Detect if the user already used the same form to submit a bug my $token = trim($cgi->param('token')); -if ($token) { - my ($creator_id, $date, $old_bug_id) = Bugzilla::Token::GetTokenData($token); - unless ($creator_id - && ($creator_id == $user->id) - && ($old_bug_id =~ "^createbug:")) - { - # The token is invalid. - ThrowUserError('token_does_not_exist'); - } - - $old_bug_id =~ s/^createbug://; - - if ($old_bug_id && (!$cgi->param('ignore_token') - || ($cgi->param('ignore_token') != $old_bug_id))) - { - $vars->{'bugid'} = $old_bug_id; - $vars->{'allow_override'} = defined $cgi->param('ignore_token') ? 0 : 1; - - print $cgi->header(); - $template->process("bug/create/confirm-create-dupe.html.tmpl", $vars) - || ThrowTemplateError($template->error()); - exit; - } -} +check_token_data($token, 'create_bug', 'index.cgi'); # do a match on the fields if applicable Bugzilla::User::match_field ({ @@ -169,8 +146,10 @@ foreach my $field (@multi_selects) { my $bug = Bugzilla::Bug->create(\%bug_params); -# Get the bug ID back. +# Get the bug ID back and delete the token used to create this bug. my $id = $bug->bug_id; +delete_token($token); + # We do this directly from the DB because $bug->creation_ts has the seconds # formatted out of it (which should be fixed some day). my $timestamp = $dbh->selectrow_array( @@ -243,12 +222,6 @@ Bugzilla::Hook::process('post_bug_after_creation', { vars => $vars }); ThrowCodeError("bug_error", { bug => $bug }) if $bug->error; -if ($token) { - trick_taint($token); - $dbh->do('UPDATE tokens SET eventdata = ? WHERE token = ?', undef, - ("createbug:$id", $token)); -} - my $recipients = { changer => $user }; my $bug_sent = Bugzilla::BugMail::Send($id, $recipients); $bug_sent->{type} = 'created'; diff --git a/process_bug.cgi b/process_bug.cgi index 382ee8b59..7c6e9590c 100755 --- a/process_bug.cgi +++ b/process_bug.cgi @@ -385,6 +385,9 @@ foreach my $bug (@bug_objects) { $bug->send_changes($changes, $vars); } +# Delete the session token used for the mass-change. +delete_token($token) unless $cgi->param('id'); + if (Bugzilla->usage_mode == USAGE_MODE_EMAIL) { # Do nothing. } diff --git a/template/en/default/bug/create/confirm-create-dupe.html.tmpl b/template/en/default/bug/create/confirm-create-dupe.html.tmpl deleted file mode 100644 index b0a5cddda..000000000 --- a/template/en/default/bug/create/confirm-create-dupe.html.tmpl +++ /dev/null @@ -1,57 +0,0 @@ -[%# 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 Olav Vitters. - # - # Contributor(s): Olav Vitters - #%] - -[%# INTERFACE: - # bugid: integer. ID of the bug previously used to create a bug. - # allow_override: boolean int. Is 1 if the user may submit the bug again. - #%] - -[% PROCESS "global/field-descs.none.tmpl" %] - -[% PROCESS global/header.html.tmpl - title = "Already filed $terms.bug" -%] - -[% USE Bugzilla %] - - - - - -
- - You already used the form to file [% "$terms.bug $bugid" FILTER bug_link(bugid) FILTER none %]. - -
- -

You are highly encouraged to visit [% "$terms.bug $bugid" -FILTER bug_link(bugid) FILTER none %].

- -[% IF allow_override %] -

If you are sure you used the same form to submit a new [% terms.bug %], - click 'File [% terms.bug %] again'.

- -

- [% PROCESS "global/hidden-fields.html.tmpl" - exclude="^(Bugzilla_login|Bugzilla_password|ignore_token)$" %] - - -
-[% END %] - -[% PROCESS global/footer.html.tmpl %] -- cgit v1.2.3-24-g4f1b From 39a3753d0cd38ff1752b00f24627cbaba412848a Mon Sep 17 00:00:00 2001 From: Frédéric Buclin Date: Sat, 26 Nov 2011 01:13:18 +0100 Subject: Bug 255606: Do not let buglist.cgi return all bugs by default r/a=mkanat --- Bugzilla/Config.pm | 5 +++++ Bugzilla/Config/Query.pm | 2 +- Bugzilla/Search.pm | 5 +++++ buglist.cgi | 2 +- collectstats.pl | 1 + template/en/default/admin/params/query.html.tmpl | 11 ++++++++--- template/en/default/global/messages.html.tmpl | 4 ++++ template/en/default/search/search-specific.html.tmpl | 2 +- whine.pl | 10 +++++++++- 9 files changed, 35 insertions(+), 7 deletions(-) diff --git a/Bugzilla/Config.pm b/Bugzilla/Config.pm index c247a072a..990fd8dd2 100644 --- a/Bugzilla/Config.pm +++ b/Bugzilla/Config.pm @@ -193,6 +193,11 @@ sub update_params { $new_params{'ssl_redirect'} = 1; } + # "specific_search_allow_empty_words" has been renamed to "search_allow_no_criteria". + if (exists $param->{'specific_search_allow_empty_words'}) { + $new_params{'search_allow_no_criteria'} = $param->{'specific_search_allow_empty_words'}; + } + # --- DEFAULTS FOR NEW PARAMS --- _load_params unless %params; diff --git a/Bugzilla/Config/Query.pm b/Bugzilla/Config/Query.pm index 17a74998e..4038c13ef 100644 --- a/Bugzilla/Config/Query.pm +++ b/Bugzilla/Config/Query.pm @@ -68,7 +68,7 @@ sub get_param_list { }, { - name => 'specific_search_allow_empty_words', + name => 'search_allow_no_criteria', type => 'b', default => 1 }, diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm index d47e0ae99..6bbf4ab42 100644 --- a/Bugzilla/Search.pm +++ b/Bugzilla/Search.pm @@ -1168,6 +1168,11 @@ sub _sql_where { if ($clause_sql) { $where .= "\n AND " . $clause_sql; } + elsif (!Bugzilla->params->{'search_allow_no_criteria'} + && !$self->{allow_unlimited}) + { + ThrowUserError('buglist_parameters_required'); + } return $where; } diff --git a/buglist.cgi b/buglist.cgi index bf8b443e6..85a8ae760 100755 --- a/buglist.cgi +++ b/buglist.cgi @@ -82,7 +82,7 @@ if (defined($searchstring)) { # If configured to not allow empty words, reject empty searches from the # Find a Specific Bug search form, including words being a single or # several consecutive whitespaces only. -if (!Bugzilla->params->{'specific_search_allow_empty_words'} +if (!Bugzilla->params->{'search_allow_no_criteria'} && defined($cgi->param('content')) && $cgi->param('content') =~ /^\s*$/) { ThrowUserError("buglist_parameters_required"); diff --git a/collectstats.pl b/collectstats.pl index 26bead6ab..007669fad 100755 --- a/collectstats.pl +++ b/collectstats.pl @@ -507,6 +507,7 @@ sub CollectSeriesData { eval { my $search = new Bugzilla::Search('params' => scalar $cgi->Vars, 'fields' => ["bug_id"], + 'allow_unlimited' => 1, 'user' => $user); my $sql = $search->sql; $data = $shadow_dbh->selectall_arrayref($sql); diff --git a/template/en/default/admin/params/query.html.tmpl b/template/en/default/admin/params/query.html.tmpl index d8f5f0c42..255c75a6b 100644 --- a/template/en/default/admin/params/query.html.tmpl +++ b/template/en/default/admin/params/query.html.tmpl @@ -51,9 +51,14 @@ "access the advanced query page. It's in URL parameter " _ "format, which makes it hard to read. Sorry!", - specific_search_allow_empty_words => - "Whether to allow a search on the 'Simple Search' page with an empty" - _ " 'Words' field.", + search_allow_no_criteria => + "Unless the code explicitly allows all $terms.bugs to be returned, this " _ + "parameter permits to block the execution of queries with no criteria. " _ + "When turned off, a query must have some criteria specified to limit " _ + "the number of $terms.bugs returned to the user. When turned on, a user " _ + "is allowed to run a query with no criteria and get all $terms.bugs he can " _ + "see in his list. Turning this parameter on is not recommended on large " _ + "installations.", default_search_limit => "By default, $terms.Bugzilla limits searches done in the web" diff --git a/template/en/default/global/messages.html.tmpl b/template/en/default/global/messages.html.tmpl index 6e24198dd..2567d4a7a 100644 --- a/template/en/default/global/messages.html.tmpl +++ b/template/en/default/global/messages.html.tmpl @@ -924,6 +924,10 @@ No changes made to version [% version.name FILTER html %]. [% END %] + [% ELSIF message_tag == "whine_query_failed" %] + The query '[% query_name FILTER html %]' from [% author.login FILTER html %] + failed: [% reason FILTER html %] + [% ELSIF message_tag == "workflow_updated" %] The workflow has been updated. [% END %] diff --git a/template/en/default/search/search-specific.html.tmpl b/template/en/default/search/search-specific.html.tmpl index 31d950ec5..9ef299425 100644 --- a/template/en/default/search/search-specific.html.tmpl +++ b/template/en/default/search/search-specific.html.tmpl @@ -110,7 +110,7 @@ for "crash secure SSL flash". - [% IF Param('specific_search_allow_empty_words') %] + [% IF Param('search_allow_no_criteria') %] [% ELSE %] scalar $searchparams->Vars, 'user' => $args->{'recipient'}, # the search runs as the recipient ); - my $sqlquery = $search->sql; + # If a query fails for whatever reason, it shouldn't kill the script. + my $sqlquery = eval { $search->sql }; + if ($@) { + print get_text('whine_query_failed', { query_name => $thisquery->{'name'}, + author => $args->{'author'}, + reason => $@ }) . "\n"; + next; + } + $sth = $dbh->prepare($sqlquery); $sth->execute; -- cgit v1.2.3-24-g4f1b From fb5a11226b8c5c65df7afa90547e6ddfe711a839 Mon Sep 17 00:00:00 2001 From: Frédéric Buclin Date: Sat, 26 Nov 2011 14:18:04 +0100 Subject: Bug 368250: collectstats.pl creates files with wrong ownership r/a=mkanat --- Bugzilla/Install/Filesystem.pm | 27 +++++++++++++++++++++++++++ collectstats.pl | 29 ++++++++++++----------------- 2 files changed, 39 insertions(+), 17 deletions(-) diff --git a/Bugzilla/Install/Filesystem.pm b/Bugzilla/Install/Filesystem.pm index 15106dab9..c5215ecfa 100644 --- a/Bugzilla/Install/Filesystem.pm +++ b/Bugzilla/Install/Filesystem.pm @@ -46,6 +46,7 @@ our @EXPORT = qw( update_filesystem create_htaccess fix_all_file_permissions + fix_dir_permissions fix_file_permissions ); @@ -645,6 +646,26 @@ sub _update_old_charts { } } +sub fix_dir_permissions { + my ($dir) = @_; + return if ON_WINDOWS; + # Note that _get_owner_and_group is always silent here. + my ($owner_id, $group_id) = _get_owner_and_group(); + + my $perms; + my $fs = FILESYSTEM(); + if ($perms = $fs->{recurse_dirs}->{$dir}) { + _fix_perms_recursively($dir, $owner_id, $group_id, $perms); + } + elsif ($perms = $fs->{all_dirs}->{$dir}) { + _fix_perms($dir, $owner_id, $group_id, $perms); + } + else { + # Do nothing. We know nothing about this directory. + warn "Unknown directory $dir"; + } +} + sub fix_file_permissions { my ($file) = @_; return if ON_WINDOWS; @@ -843,6 +864,12 @@ Params: C<$output> - C if you want this function to print Returns: nothing +=item C + +Given the name of a directory, its permissions will be fixed according to +how they are supposed to be set in Bugzilla's current configuration. +If it fails to set the permissions, a warning will be printed to STDERR. + =item C Given the name of a file, its permissions will be fixed according to diff --git a/collectstats.pl b/collectstats.pl index 007669fad..1487e5a72 100755 --- a/collectstats.pl +++ b/collectstats.pl @@ -41,6 +41,7 @@ use Bugzilla::Search; use Bugzilla::User; use Bugzilla::Product; use Bugzilla::Field; +use Bugzilla::Install::Filesystem qw(fix_dir_permissions); my %switch; GetOptions(\%switch, 'help|h', 'regenerate'); @@ -139,32 +140,28 @@ my $tstart = time; my @myproducts = Bugzilla::Product->get_all; unshift(@myproducts, "-All-"); -foreach (@myproducts) { - my $dir = "$datadir/mining"; - - &check_data_dir ($dir); +my $dir = "$datadir/mining"; +if (!-d $dir) { + mkdir $dir or die "mkdir $dir failed: $!"; + fix_dir_permissions($dir); +} +foreach (@myproducts) { if ($switch{'regenerate'}) { regenerate_stats($dir, $_, \%bug_resolution, \%bug_status, \%removed); } else { &collect_stats($dir, $_); } } +# Fix permissions for all files in mining/. +fix_dir_permissions($dir); + my $tend = time; # Uncomment the following line for performance testing. #print "Total time taken " . delta_time($tstart, $tend) . "\n"; CollectSeriesData(); -sub check_data_dir { - my $dir = shift; - - if (! -d $dir) { - mkdir $dir, 0755; - chmod 0755, $dir; - } -} - sub collect_stats { my $dir = shift; my $product = shift; @@ -250,7 +247,6 @@ FIN } print DATA (join '|', @row) . "\n"; close DATA; - chmod 0644, $file; } sub get_old_data { @@ -406,14 +402,13 @@ FIN foreach (@resolutions) { print DATA "|$bugcount{$_}"; } print DATA "\n"; } - + # Finish up output feedback for this product. my $tend = time; print "\rRegenerating $product \[100.0\%] - " . delta_time($tstart, $tend) . "\n"; - + close DATA; - chmod 0640, $file; } } -- cgit v1.2.3-24-g4f1b From 4aeec3917bbcc69ed71d47420aeb0141f0be6b11 Mon Sep 17 00:00:00 2001 From: Frédéric Buclin Date: Mon, 28 Nov 2011 00:25:02 +0100 Subject: Redirect the error to STDERR if a query cannot be run, see bug 277073 --- whine.pl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/whine.pl b/whine.pl index 9b48c8eda..ad6067228 100755 --- a/whine.pl +++ b/whine.pl @@ -455,9 +455,9 @@ sub run_queries { # If a query fails for whatever reason, it shouldn't kill the script. my $sqlquery = eval { $search->sql }; if ($@) { - print get_text('whine_query_failed', { query_name => $thisquery->{'name'}, - author => $args->{'author'}, - reason => $@ }) . "\n"; + print STDERR get_text('whine_query_failed', { query_name => $thisquery->{'name'}, + author => $args->{'author'}, + reason => $@ }) . "\n"; next; } -- cgit v1.2.3-24-g4f1b From 2fe1db36b3ced43ca9b76a5fbc293c845fd13066 Mon Sep 17 00:00:00 2001 From: Frédéric Buclin Date: Mon, 28 Nov 2011 17:10:07 +0100 Subject: Bug 705393: Improve the error message thrown by Update.pm when updates.bugzilla.org is unavailable r=glob a=LpSolit --- Bugzilla/Constants.pm | 7 +++++++ Bugzilla/Update.pm | 26 +++++++++++++------------- template/en/default/index.html.tmpl | 16 +++++++++++----- 3 files changed, 31 insertions(+), 18 deletions(-) diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm index 5c9fecc57..e44306520 100644 --- a/Bugzilla/Constants.pm +++ b/Bugzilla/Constants.pm @@ -40,6 +40,9 @@ use Memoize; @Bugzilla::Constants::EXPORT = qw( BUGZILLA_VERSION + REMOTE_FILE + LOCAL_FILE + bz_locations IS_NULL @@ -201,6 +204,10 @@ use Memoize; # Bugzilla version use constant BUGZILLA_VERSION => "4.1.3+"; +# Location of the remote and local XML files to track new releases. +use constant REMOTE_FILE => 'http://updates.bugzilla.org/bugzilla-update.xml'; +use constant LOCAL_FILE => 'bugzilla-update.xml'; # Relative to datadir. + # These are unique values that are unlikely to match a string or a number, # to be used in criteria for match() functions and other things. They start # and end with spaces because most Bugzilla stuff has trim() called on it, diff --git a/Bugzilla/Update.pm b/Bugzilla/Update.pm index a94fd167d..c9942a4f0 100644 --- a/Bugzilla/Update.pm +++ b/Bugzilla/Update.pm @@ -20,8 +20,6 @@ use strict; use Bugzilla::Constants; -use constant REMOTE_FILE => 'http://updates.bugzilla.org/bugzilla-update.xml'; -use constant LOCAL_FILE => "/bugzilla-update.xml"; # Relative to datadir. use constant TIME_INTERVAL => 86400; # Default is one day, in seconds. use constant TIMEOUT => 5; # Number of seconds before timeout. @@ -30,26 +28,25 @@ sub get_notifications { return if !Bugzilla->feature('updates'); return if (Bugzilla->params->{'upgrade_notification'} eq 'disabled'); - my $local_file = bz_locations()->{'datadir'} . LOCAL_FILE; + my $local_file = bz_locations()->{'datadir'} . '/' . LOCAL_FILE; # Update the local XML file if this one doesn't exist or if # the last modification time (stat[9]) is older than TIME_INTERVAL. if (!-e $local_file || (time() - (stat($local_file))[9] > TIME_INTERVAL)) { unlink $local_file; # Make sure the old copy is away. - if (-e $local_file) { - return { 'error' => 'no_update', xml_file => $local_file }; - } + return { 'error' => 'no_update' } if (-e $local_file); + my $error = _synchronize_data(); # If an error is returned, leave now. return $error if $error; } # If we cannot access the local XML file, ignore it. - return {'error' => 'no_access', 'xml_file' => $local_file} unless (-r $local_file); + return { 'error' => 'no_access' } unless (-r $local_file); my $twig = XML::Twig->new(); $twig->safe_parsefile($local_file); # If the XML file is invalid, return. - return {'error' => 'corrupted', 'xml_file' => $local_file} if $@; + return { 'error' => 'corrupted' } if $@; my $root = $twig->root; my @releases; @@ -119,7 +116,7 @@ sub get_notifications { } sub _synchronize_data { - my $local_file = bz_locations()->{'datadir'} . LOCAL_FILE; + my $local_file = bz_locations()->{'datadir'} . '/' . LOCAL_FILE; my $ua = LWP::UserAgent->new(); $ua->timeout(TIMEOUT); @@ -133,7 +130,7 @@ sub _synchronize_data { else { $ua->env_proxy; } - $ua->mirror(REMOTE_FILE, $local_file); + my $response = eval { $ua->mirror(REMOTE_FILE, $local_file) }; # $ua->mirror() forces the modification time of the local XML file # to match the modification time of the remote one. @@ -144,11 +141,14 @@ sub _synchronize_data { # Try to alter its last modification time. my $can_alter = utime(undef, undef, $local_file); # This error should never happen. - $can_alter || return {'error' => 'no_update', 'xml_file' => $local_file}; + $can_alter || return { 'error' => 'no_update' }; } - else { + elsif ($response && $response->is_error) { # We have been unable to download the file. - return {'error' => 'cannot_download', 'xml_file' => $local_file}; + return { 'error' => 'cannot_download', 'reason' => $response->status_line }; + } + else { + return { 'error' => 'no_write', 'reason' => $@ }; } # Everything went well. diff --git a/template/en/default/index.html.tmpl b/template/en/default/index.html.tmpl index b1370261f..98648e25e 100644 --- a/template/en/default/index.html.tmpl +++ b/template/en/default/index.html.tmpl @@ -92,18 +92,24 @@ YAHOO.util.Event.onDOMReady(onLoadActions); You can configure this notification from the Parameters page.

[% ELSIF release.error == "cannot_download" %] -

The local XML file '[% release.xml_file FILTER html %]' cannot be created. - Please make sure the web server can write in this directory and that you can access +

The remote file + [%~ constants.REMOTE_FILE FILTER html %] cannot be downloaded + (reason: [% release.reason FILTER html %]).
+ Either the remote server is temporarily unavailable, or your web server cannot access the web. If you are behind a proxy, set the proxy_url parameter correctly.

+ [% ELSIF release.error == "no_write" %] +

The local XML file '[% constants.LOCAL_FILE FILTER html %]' cannot be created + (reason: [% release.reason FILTER html %]).
+ Please make sure the web server can write into this directory. [% ELSIF release.error == "no_update" %] -

The local XML file '[% release.xml_file FILTER html %]' cannot be updated. +

The local XML file '[% constants.LOCAL_FILE FILTER html %]' cannot be updated. Please make sure the web server can edit this file.

[% ELSIF release.error == "no_access" %] -

The local XML file '[% release.xml_file FILTER html %]' cannot be read. +

The local XML file '[% constants.LOCAL_FILE FILTER html %]' cannot be read. Please make sure this file has the correct rights set on it.

[% ELSIF release.error == "corrupted" %] -

The local XML file '[% release.xml_file FILTER html %]' has an invalid XML format. +

The local XML file '[% constants.LOCAL_FILE FILTER html %]' has an invalid XML format. Please delete it and try accessing this page again.

[% ELSIF release.error == "unknown_parameter" %]

'[% Param("upgrade_notification") FILTER html %]' is not a valid notification -- cgit v1.2.3-24-g4f1b