diff --git a/Bugzilla/Keyword.pm b/Bugzilla/Keyword.pm index 35dc2f594d..fc54374cc3 100644 --- a/Bugzilla/Keyword.pm +++ b/Bugzilla/Keyword.pm @@ -43,6 +43,14 @@ use constant UPDATE_COLUMNS => qw( is_active ); +# Keyword families that classify security vulnerabilities. Bug counts for these +# keywords are hidden from users who cannot see security bugs (bug 2056990). +# +# Note that C must be listed separately from C: the alternation +# is anchored on the trailing hyphen, so C only matches C and +# never C. +use constant SECURITY_KEYWORD_REGEX => qr/^(?:sec|csec|csectype|wsec|opsec)-/; + ############################### #### Accessors ###### ############################### @@ -59,6 +67,11 @@ sub bug_count { return $self->{'bug_count'}; } +sub is_security_keyword { + my ($self) = @_; + return $self->name =~ SECURITY_KEYWORD_REGEX ? 1 : 0; +} + ############################### #### Mutators ##### ############################### @@ -72,14 +85,34 @@ sub set_is_active { $_[0]->set('is_active', $_[1]); } ############################### sub get_all_with_bug_count { - my $class = shift; - my $dbh = Bugzilla->dbh; + my $class = shift; + my $dbh = Bugzilla->dbh; + my $user = Bugzilla->user; + + # Only count bugs that are visible to the current user based on group + # membership, so the counts don't leak the number of security-restricted + # bugs (e.g. the sec-* and csectype-* keyword families) to users who can't + # otherwise see them. A bug is hidden if it belongs to any group the user + # is not a member of; we detect that with a LEFT JOIN and only count the + # keyword rows that have no such group (bug_group_map.bug_id IS NULL). + # + # The reporter/assignee/qa/cc visibility exceptions (see + # Bugzilla::User->visible_bugs) are intentionally not applied here: ignoring + # them can only make a count lower than the user's true visibility, never + # higher, so no restricted data can leak. Using a conditional COUNT (rather + # than a WHERE clause) keeps keywords whose bugs are all restricted in the + # result set with a count of 0, instead of dropping them entirely. my $keywords = $dbh->selectall_arrayref( 'SELECT ' . join(', ', $class->_get_db_columns) . ', - COUNT(keywords.bug_id) AS bug_count + COUNT(CASE WHEN bug_group_map.bug_id IS NULL + THEN keywords.bug_id END) AS bug_count FROM keyworddefs LEFT JOIN keywords - ON keyworddefs.id = keywords.keywordid ' + ON keyworddefs.id = keywords.keywordid + LEFT JOIN bug_group_map + ON keywords.bug_id = bug_group_map.bug_id + AND bug_group_map.group_id NOT IN (' + . $user->groups_as_string . ') ' . $dbh->sql_group_by( 'keyworddefs.id', 'keyworddefs.name, keyworddefs.description' @@ -176,6 +209,17 @@ implements. Returns: A reference to an array of Keyword objects, or an empty arrayref if there are no keywords. +=item C + + Description: Indicates if the keyword belongs to one of the security + vulnerability keyword families (C, C, + C, C and C). Callers use this to + decide whether the keyword's bug count may be shown to the + current user. See C. + Params: none + Returns: a boolean value that is true if the keyword is a security + keyword. + =item C Description: Indicates if the keyword may be used on a bug diff --git a/describekeywords.cgi b/describekeywords.cgi index 10d442d49a..635fae86c7 100755 --- a/describekeywords.cgi +++ b/describekeywords.cgi @@ -46,7 +46,7 @@ my $can_see_security = Bugzilla->user->in_group('core-security-release'); my $keywords = Bugzilla::Keyword->get_all_with_bug_count(); foreach my $keyword (@$keywords) { $keyword->{'bug_count'} = 0 - if $keyword->name =~ /^(?:sec|csec|wsec|opsec)-/ && !$can_see_security; + if $keyword->is_security_keyword && !$can_see_security; } $vars->{'keywords'} = $keywords; diff --git a/t/bmo/keyword-security-count.t b/t/bmo/keyword-security-count.t new file mode 100755 index 0000000000..f9980969e5 --- /dev/null +++ b/t/bmo/keyword-security-count.t @@ -0,0 +1,184 @@ +#!/usr/bin/env perl +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. +# +# This Source Code Form is "Incompatible With Secondary Licenses", as +# defined by the Mozilla Public License, v. 2.0. + +# Regression tests for Bug 2056990: describekeywords.cgi leaked the number of +# hidden security bugs, both because the `csectype-*` family was missing from +# the security keyword list and because the keyword counts themselves were not +# filtered by group visibility. + +use 5.10.1; +use strict; +use warnings; +use lib qw(. lib local/lib/perl5); +use Test::More; + +use Bugzilla; +use Bugzilla::Bug; +use Bugzilla::Constants; +use Bugzilla::Group; +use Bugzilla::Keyword; +use Bugzilla::Product; +use Bugzilla::User; +BEGIN { Bugzilla->extensions } + +Bugzilla->usage_mode(USAGE_MODE_TEST); +Bugzilla->error_mode(ERROR_MODE_DIE); + +my $dbh = Bugzilla->dbh; + +my $admin = Bugzilla::User->check({id => 1}); +Bugzilla->set_user($admin); + +my ($product) = Bugzilla::Product->get_all; +plan skip_all => 'No product available' unless $product; +plan skip_all => 'Product has no component' unless @{$product->components}; +plan skip_all => 'Product has no version' unless @{$product->versions}; + +############################################################################### +# is_security_keyword() +############################################################################### + +# `csectype-*` is the family that regressed: the old pattern was +# /^(?:sec|csec|wsec|opsec)-/, and `csec` does not match `csectype-` because +# the alternation is anchored on the trailing hyphen. +my %expected = ( + 'csectype-uaf' => 1, + 'csectype-sandbox-escape' => 1, + 'csectype-priv-escalation' => 1, + 'sec-critical' => 1, + 'sec-high' => 1, + 'csec-high' => 1, + 'wsec-audit' => 1, + 'opsec-infra' => 1, + 'csectype' => 0, + 'security' => 0, + 'sectionfoo' => 0, + 'relnote' => 0, + 'perf' => 0, +); + +foreach my $name (sort keys %expected) { + my $keyword = bless({name => $name}, 'Bugzilla::Keyword'); + is($keyword->is_security_keyword, + $expected{$name}, "is_security_keyword('$name') is $expected{$name}"); +} + +############################################################################### +# get_all_with_bug_count() only counts bugs visible to the current user +############################################################################### + +my $suffix = "bug2056990-$$"; +my $keyword_name = "csectype-$suffix"; +my $group_name = "keyword-count-$suffix"; + +my $keyword = Bugzilla::Keyword->create({ + name => $keyword_name, + description => 'Temporary keyword for bug 2056990', + is_active => 1, +}); +ok($keyword->id, "Created keyword $keyword_name"); + +my $group = Bugzilla::Group->create({ + name => $group_name, + description => 'Temporary group for bug 2056990', + isbuggroup => 1, +}); +ok($group->id, "Created group $group_name"); + +$dbh->do( + 'INSERT INTO group_control_map + (group_id, product_id, entry, membercontrol, othercontrol, canedit) + VALUES (?, ?, 0, ?, 0, 0)', undef, $group->id, $product->id, CONTROLMAPSHOWN +); + +my @bug_ids; +foreach my $which (qw(public restricted)) { + my $bug = Bugzilla::Bug->create({ + short_desc => "Keyword count $which bug - Bug 2056990", + product => $product->name, + component => $product->components->[0]->name, + bug_type => 'defect', + bug_severity => 'normal', + op_sys => 'Unspecified', + rep_platform => 'Unspecified', + version => $product->versions->[0]->name, + }); + ok($bug->id, "Created $which bug " . $bug->id); + push @bug_ids, $bug->id; + + $dbh->do('INSERT INTO keywords (bug_id, keywordid) VALUES (?, ?)', + undef, $bug->id, $keyword->id); + $dbh->do('INSERT INTO bug_group_map (bug_id, group_id) VALUES (?, ?)', + undef, $bug->id, $group->id) + if $which eq 'restricted'; +} + +sub count_for_keyword { + my ($name) = @_; + my ($found) + = grep { $_->name eq $name } @{Bugzilla::Keyword->get_all_with_bug_count()}; + return $found ? $found->bug_count : undef; +} + +# An anonymous (logged out) user is in no groups at all. +Bugzilla->set_user(Bugzilla::User->new()); +is(count_for_keyword($keyword_name), + 1, 'Anonymous user only counts the unrestricted bug'); + +# A user who is a member of the restricting group sees both bugs. Use a +# freshly created user so no stale group membership can be cached for it. +my $login = "keyword-count-$suffix\@bugzilla.test"; +my $member = Bugzilla::User->create({ + login_name => $login, + cryptpassword => '*', + disabledtext => '', + disable_mail => 1, +}); +$dbh->do( + 'INSERT INTO user_group_map (user_id, group_id, isbless, grant_type) + VALUES (?, ?, 0, ?)', undef, $member->id, $group->id, GRANT_DIRECT +); +Bugzilla->memcached->clear_all(); + +$member = Bugzilla::User->new({id => $member->id, cache => 0}); +Bugzilla->set_user($member); +ok($member->in_group($group_name), "Test user is a member of $group_name"); +is(count_for_keyword($keyword_name), + 2, 'Group member counts both the restricted and unrestricted bug'); + +# Keywords whose bugs are all hidden must still be listed, with a count of 0, +# rather than disappearing from the report entirely. +Bugzilla->set_user($admin); +my $hidden_keyword = Bugzilla::Keyword->create({ + name => "csectype-hidden-$suffix", + description => 'Temporary keyword for bug 2056990', + is_active => 1, +}); +$dbh->do('INSERT INTO keywords (bug_id, keywordid) VALUES (?, ?)', + undef, $bug_ids[1], $hidden_keyword->id); + +Bugzilla->set_user(Bugzilla::User->new()); +is(count_for_keyword($hidden_keyword->name), + 0, 'Fully hidden keyword is still listed with a count of 0'); + +############################################################################### +# Cleanup +############################################################################### + +Bugzilla->set_user($admin); +$dbh->do('DELETE FROM keywords WHERE keywordid IN (?, ?)', + undef, $keyword->id, $hidden_keyword->id); +$dbh->do('DELETE FROM bug_group_map WHERE group_id = ?', undef, $group->id); +$dbh->do('DELETE FROM user_group_map WHERE group_id = ?', undef, $group->id); +$dbh->do('DELETE FROM group_control_map WHERE group_id = ?', undef, $group->id); +$keyword->remove_from_db(); +$hidden_keyword->remove_from_db(); +$group->remove_from_db(); +Bugzilla->memcached->clear_all(); + +done_testing();