From 094e2ff1e60e5cf5348e7c4d1c522da28866db39 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 16 Sep 2026 12:08:47 -0400 Subject: [PATCH] Bug 2069466 --- buglist.cgi | 25 +++++-- t/buglist-series-idor.t | 154 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 173 insertions(+), 6 deletions(-) create mode 100644 t/buglist-series-idor.t diff --git a/buglist.cgi b/buglist.cgi index dd6cd584da..848c0ca08c 100755 --- a/buglist.cgi +++ b/buglist.cgi @@ -20,6 +20,7 @@ use Bugzilla::Search; use Bugzilla::Search::Quicksearch; use Bugzilla::Search::Recent; use Bugzilla::Search::Saved; +use Bugzilla::Series; use Bugzilla::User; use Bugzilla::Bug; use Bugzilla::Product; @@ -229,12 +230,23 @@ sub LookupSeries { my ($series_id) = @_; detaint_natural($series_id) || ThrowCodeError("invalid_series_id"); - my $dbh = Bugzilla->dbh; - my $result - = $dbh->selectrow_array("SELECT query FROM series " . "WHERE series_id = ?", - undef, ($series_id)); - $result || ThrowCodeError("invalid_series_id", {'series_id' => $series_id}); - return $result; + # Series are only visible to charting users, as in chart.cgi. + my $user = Bugzilla->user; + $user->in_group(Bugzilla->params->{"chartgroup"}) || ThrowUserError( + "auth_failure", + { + group => Bugzilla->params->{"chartgroup"}, + action => "use", + object => "charts" + } + ); + + # Bugzilla::Series enforces the creator / is_public / category group checks, + # and returns undef for both missing and unauthorized series so this is not + # an existence oracle. + my $series = Bugzilla::Series->new($series_id) + || ThrowCodeError("invalid_series_id", {'series_id' => $series_id}); + return $series->{'query'}; } sub GetQuip { @@ -334,6 +346,7 @@ if ($cmdtype eq "dorem") { } elsif ($remaction eq "runseries") { + $user = Bugzilla->login(LOGIN_REQUIRED); $buffer = LookupSeries(scalar $cgi->param("series_id")); $vars->{'searchname'} = $cgi->param('namedcmd'); $vars->{'searchtype'} = "series"; diff --git a/t/buglist-series-idor.t b/t/buglist-series-idor.t new file mode 100644 index 0000000000..455ec1ff8b --- /dev/null +++ b/t/buglist-series-idor.t @@ -0,0 +1,154 @@ +#!/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. + +# buglist.cgi?cmdtype=dorem&remaction=runseries reads a chart series' saved +# query out of the series table and reflects it back into the rendered page as +# urlquerypart. That query is private data: it names products, components, +# groups and the email addresses of the people the series tracks. +# +# The read used to be a bare primary-key lookup with no authorization at all, +# so anyone -- including a logged out visitor -- could walk series_id and read +# every saved query in the installation. The two gates the rest of the charting +# code uses (chartgroup membership, and Bugzilla::Series' creator/is_public/ +# category-group check) both now apply here, which is what this pins. + +use strict; +use warnings; +use 5.10.1; +use lib qw( . lib local/lib/perl5 ); + +BEGIN { + $ENV{LOG4PERL_CONFIG_FILE} = 'log4perl-t.conf'; + + # The Hostage plugin requires specific Host: headers; disable for tests. + $ENV{BUGZILLA_DISABLE_HOSTAGE} = 1; +} + +# Before anything pulls in Bugzilla::Error: CGI::Compile installs +# CORE::GLOBAL::exit, and the legacy CGI wrapper relies on that to turn a +# script's exit into a catchable exception. Perl binds `exit` at compile time, +# so a Bugzilla module compiled ahead of CGI::Compile gets the real one and +# takes this test process down with it on the first ThrowUserError. +use CGI::Compile; + +use Bugzilla::Test::MockLocalconfig (urlbase => 'http://bmo.test'); +use Bugzilla::Test::MockDB; +use Bugzilla::Test::MockParams; +use Bugzilla::Test::Util qw(create_user); + +use Bugzilla; +use Bugzilla::App (); +use Bugzilla::Constants; +use Bugzilla::Group; +use Bugzilla::Util qw(generate_random_password); + +use Mojo::Cookie::Response; +use Test2::V0; +use Test::Mojo; + +# A marker that only appears in the series' saved query, so "did the query +# leak" is a single substring check against the response body. +my $secret = 'sekritwhiteboardmarker'; +my $secret_re = qr/\Q$secret\E/; +my $query = "bug_status=UNCONFIRMED&status_whiteboard=$secret"; + +my $dbh = Bugzilla->dbh; + +my $owner = create_user('series-owner@bmo.test', '*'); +my $charter = create_user('series-charter@bmo.test', '*'); +my $outsider = create_user('series-outsider@bmo.test', '*'); + +# chartgroup defaults to editbugs, whose userregexp is '.*' (Bugzilla/Install.pm +# SYSTEM_GROUPS), so every user created above is already a member. Revoke that +# from the outsider only, leaving the owner and the charter with charting +# rights. The charter covers the second gate: a user who may use charts but has +# no business reading someone else's private series. +my $chartgroup = Bugzilla::Group->new({name => Bugzilla->params->{chartgroup}}); +$dbh->do('DELETE FROM user_group_map WHERE user_id = ? AND group_id = ?', + undef, $outsider->id, $chartgroup->id); + +$dbh->do('INSERT INTO series_categories (name) VALUES (?)', undef, 'idor-cat'); +my $category + = $dbh->selectrow_array('SELECT id FROM series_categories WHERE name = ?', + undef, 'idor-cat'); + +$dbh->do( + 'INSERT INTO series + (creator, category, subcategory, name, frequency, query, is_public) + VALUES (?, ?, ?, ?, 1, ?, 0)', undef, $owner->id, $category, $category, + 'idor-series', $query +); +my $series_id + = $dbh->selectrow_array('SELECT series_id FROM series WHERE name = ?', + undef, 'idor-series'); + +my $url = "/buglist.cgi?cmdtype=dorem&remaction=runseries&series_id=$series_id"; + +# One Test::Mojo for the whole file: building the app twice makes CGI::Compile +# die with "Tried to load admin.cgi more than once". +my $t = Test::Mojo->new('Bugzilla::App'); +my $ua = $t->ua; + +# buglist.cgi redirects a logged in user to a canonical search URL before it +# looks at cmdtype at all, so without this the interesting cases would only +# ever assert against an empty 302 body. +$ua->max_redirects(3); + +# Log in the way Bugzilla::App::Plugin::Login does for the web UI: a +# logincookies row plus the matching pair of cookies. These go in the cookie +# jar rather than on the request, because Mojo::UserAgent strips a Cookie +# header when it follows a redirect and buglist.cgi always redirects first. +sub login_as { + my ($user) = @_; + my $cookie = generate_random_password(16); + $dbh->do( + 'INSERT INTO logincookies (cookie, userid, lastused) VALUES (?, ?, NOW())', + undef, $cookie, $user->id); + my $jar = $ua->cookie_jar; + $jar->empty; + $jar->add( + map { + Mojo::Cookie::Response->new( + name => $_->[0], + value => $_->[1], + domain => $ua->server->url->host, + path => '/' + ) + } ['Bugzilla_login', $user->id], + ['Bugzilla_logincookie', $cookie] + ); +} + +# One row per way of being refused: who asks, and the page they should get +# instead of the series. Anonymous is the report's headline case; the outsider +# covers the chartgroup gate (being logged in is not on its own permission to +# read a series); the charter covers the creator/is_public check inside +# Bugzilla::Series, which is what a regression to the old raw primary-key +# lookup would reopen for every charting user. Asserting the page each one +# lands on, and not just the absence of the marker, stops a 500 or the wrong +# error page from passing for a working gate. +for my $case ( + [undef, qr{
}, 'anonymous'], + [$outsider, qr/Authorization Required/, 'a user outside chartgroup'], + [$charter, qr/Invalid Series/, 'a chartgroup member who is not the creator'], + ) +{ + my ($user, $expect, $who) = @$case; + $user ? login_as($user) : $ua->cookie_jar->empty; + $t->get_ok($url)->status_is(200)->content_like($expect, "$who is refused"); + $t->content_unlike($secret_re, "$who does not leak the query"); +} + +# The owner still gets their own series, so the fix is a gate and not a +# removal of the feature. This also keeps the checks above honest: without it +# they would still pass if runseries were broken outright. +login_as($owner); +$t->get_ok($url)->status_is(200) + ->content_like($secret_re, 'the series creator can still run it'); + +done_testing;