From bc521effbd39f4e88e8de50dac650acd8a46705f Mon Sep 17 00:00:00 2001
From: "jake%acutex.net" <>
Date: Thu, 31 May 2001 22:52:23 +0000
Subject: [PATCH] Bugzilla was leaking information about bugs marked secure
(using bug groups). This checkin fixes bugs 39524, 39527, 39531, and 39533.
Patches by Myk Melez <myk@mozilla.org>. r= jake@acutex.net
---
CGI.pl | 49 +++++++++++++++++++++++++++++
process_bug.cgi | 70 ++++++++++++++++++++++++-----------------
showdependencygraph.cgi | 26 ++++++++++++---
showdependencytree.cgi | 23 ++++++++++----
showvotes.cgi | 61 +++++++++++++++++------------------
5 files changed, 160 insertions(+), 69 deletions(-)
diff --git a/CGI.pl b/CGI.pl
index e82ce8911..87639165b 100644
--- a/CGI.pl
+++ b/CGI.pl
@@ -226,6 +226,55 @@ sub CheckFormFieldDefined (\%$) {
}
}
+sub ValidateBugID {
+ # Validates and verifies a bug ID, making sure the number is a
+ # positive integer, that it represents an existing bug in the
+ # database, and that the user is authorized to access that bug.
+
+ my ($id) = @_;
+
+ # Make sure the bug number is a positive integer.
+ $id =~ /^([1-9][0-9]*)$/
+ || DisplayError("The bug number is invalid.")
+ && exit;
+
+ # Make sure the usergroupset variable is set. This variable stores
+ # the set of groups the user is a member of. This variable should
+ # be set by either confirm_login or quietly_check_login, but we set
+ # it here just in case one of those functions has not been run yet.
+ $::usergroupset ||= 0;
+
+ # Query the database for the bug, retrieving a boolean value that
+ # represents whether or not the user is authorized to access the bug.
+
+ # Users are authorized to access bugs if they are a member of all
+ # groups to which the bug is restricted. User group membership and
+ # bug restrictions are stored as bits within bitsets, so authorization
+ # can be determined by comparing the intersection of the user's
+ # bitset with the bug's bitset. If the result matches the bug's bitset
+ # the user is a member of all groups to which the bug is restricted
+ # and is authorized to access the bug.
+
+ # Bit arithmetic is performed by MySQL instead of Perl because bitset
+ # fields in the database are 64 bits wide (BIGINT), and Perl installations
+ # may or may not support integers larger than 32 bits. Using bitsets
+ # and doing bitset arithmetic is probably not cross-database compatible,
+ # however, so these mechanisms are likely to change in the future.
+ SendSQL("SELECT ((groupset & $::usergroupset) = groupset)
+ FROM bugs WHERE bug_id = $id");
+
+ # Make sure the bug exists in the database.
+ MoreSQLData()
+ || DisplayError("Bug #$id does not exist.")
+ && exit;
+
+ # Make sure the user is authorized to access the bug.
+ my ($isauthorized) = FetchSQLData();
+ $isauthorized
+ || DisplayError("You are not authorized to access bug #$id.")
+ && exit;
+}
+
# check and see if a given string actually represents a positive
# integer, and abort if not.
#
diff --git a/process_bug.cgi b/process_bug.cgi
index dded85dbb..4b4453dc1 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -48,6 +48,35 @@ my $whoid = confirm_login();
my $requiremilestone = 0;
+######################################################################
+# Begin Data/Security Validation
+######################################################################
+
+# Create a list of IDs of all bugs being modified in this request.
+# This list will either consist of a single bug number from the "id"
+# form/URL field or a series of numbers from multiple form/URL fields
+# named "id_x" where "x" is the bug number.
+my @idlist;
+if (defined $::FORM{'id'}) {
+ push @idlist, $::FORM{'id'};
+} else {
+ foreach my $i (keys %::FORM) {
+ if ($i =~ /^id_([1-9][0-9]*)/) {
+ push @idlist, $1;
+ }
+ }
+}
+
+# For each bug being modified, make sure its ID is a valid bug number
+# representing an existing bug that the user is authorized to access.
+foreach my $id (@idlist) {
+ ValidateBugID($id);
+}
+
+######################################################################
+# End Data/Security Validation
+######################################################################
+
print "Content-type: text/html\n\n";
PutHeader ("Bug processed");
@@ -221,9 +250,7 @@ empowered user, may make that change to the $f field.
-my @idlist;
-if (defined $::FORM{'id'}) {
-
+if (defined $::FORM{'id'} && Param('strictvaluechecks')) {
# since this means that we were called from show_bug.cgi, now is a good
# time to do a whole bunch of error checking that can't easily happen when
# we've been called from buglist.cgi, because buglist.cgi only tweaks
@@ -231,31 +258,18 @@ if (defined $::FORM{'id'}) {
# (XXX those error checks need to happen too, but implementing them
# is more work in the current architecture of this script...)
#
- if ( Param('strictvaluechecks') ) {
- CheckFormField(\%::FORM, 'rep_platform', \@::legal_platform);
- CheckFormField(\%::FORM, 'priority', \@::legal_priority);
- CheckFormField(\%::FORM, 'bug_severity', \@::legal_severity);
- CheckFormField(\%::FORM, 'component',
- \@{$::components{$::FORM{'product'}}});
- CheckFormFieldDefined(\%::FORM, 'bug_file_loc');
- CheckFormFieldDefined(\%::FORM, 'short_desc');
- CheckFormField(\%::FORM, 'product', \@::legal_product);
- CheckFormField(\%::FORM, 'version',
- \@{$::versions{$::FORM{'product'}}});
- CheckFormField(\%::FORM, 'op_sys', \@::legal_opsys);
- CheckFormFieldDefined(\%::FORM, 'longdesclength');
- CheckPosInt($::FORM{'id'});
- }
- push @idlist, $::FORM{'id'};
-} else {
- foreach my $i (keys %::FORM) {
- if ($i =~ /^id_/) {
- if ( Param('strictvaluechecks') ) {
- CheckPosInt(substr($i, 3));
- }
- push @idlist, substr($i, 3);
- }
- }
+ CheckFormField(\%::FORM, 'rep_platform', \@::legal_platform);
+ CheckFormField(\%::FORM, 'priority', \@::legal_priority);
+ CheckFormField(\%::FORM, 'bug_severity', \@::legal_severity);
+ CheckFormField(\%::FORM, 'component',
+ \@{$::components{$::FORM{'product'}}});
+ CheckFormFieldDefined(\%::FORM, 'bug_file_loc');
+ CheckFormFieldDefined(\%::FORM, 'short_desc');
+ CheckFormField(\%::FORM, 'product', \@::legal_product);
+ CheckFormField(\%::FORM, 'version',
+ \@{$::versions{$::FORM{'product'}}});
+ CheckFormField(\%::FORM, 'op_sys', \@::legal_opsys);
+ CheckFormFieldDefined(\%::FORM, 'longdesclength');
}
my $action = '';
diff --git a/showdependencygraph.cgi b/showdependencygraph.cgi
index f15534be3..df377c096 100755
--- a/showdependencygraph.cgi
+++ b/showdependencygraph.cgi
@@ -25,8 +25,28 @@ use strict;
require "CGI.pl";
+ConnectToDatabase();
+
+quietly_check_login();
+
+$::usergroupset = $::usergroupset; # More warning suppression silliness.
+
+######################################################################
+# Begin Data/Security Validation
+######################################################################
+
+# Make sure the bug ID is a positive integer representing an existing
+# bug that the user is authorized to access.
+if (defined $::FORM{'id'}) {
+ ValidateBugID($::FORM{'id'});
+}
+
+######################################################################
+# End Data/Security Validation
+######################################################################
+
my $id = $::FORM{'id'};
-die "Invalid id: $id" unless $id =~ /^\s*\d+\s*$/;
+
my $urlbase = Param("urlbase");
my %seen;
@@ -51,10 +71,6 @@ $::FORM{'rankdir'} = "LR" if !defined $::FORM{'rankdir'};
if (defined $id) {
- ConnectToDatabase();
- quietly_check_login();
- $::usergroupset = $::usergroupset; # More warning suppression silliness.
-
mkdir("data/webdot", 0777);
my $filename = "data/webdot/$$.dot";
diff --git a/showdependencytree.cgi b/showdependencytree.cgi
index 74e2778bc..bab36da61 100755
--- a/showdependencytree.cgi
+++ b/showdependencytree.cgi
@@ -29,6 +29,23 @@ require "CGI.pl";
use vars %::FORM;
+ConnectToDatabase();
+
+quietly_check_login();
+
+$::usergroupset = $::usergroupset; # More warning suppression silliness.
+
+######################################################################
+# Begin Data/Security Validation
+######################################################################
+
+# Make sure the bug ID is a positive integer representing an existing
+# bug that the user is authorized to access.
+ValidateBugID($::FORM{'id'});
+
+######################################################################
+# End Data/Security Validation
+######################################################################
my $id = $::FORM{'id'};
my $linkedid = qq{<a href="show_bug.cgi?id=$id">$id</a>};
@@ -36,12 +53,6 @@ my $linkedid = qq{<a href="show_bug.cgi?id=$id">$id</a>};
print "Content-type: text/html\n\n";
PutHeader("Dependency tree", "Dependency tree", "Bug $linkedid");
-ConnectToDatabase();
-
-quietly_check_login();
-
-$::usergroupset = $::usergroupset; # More warning suppression silliness.
-
my %seen;
sub DumpKids {
diff --git a/showvotes.cgi b/showvotes.cgi
index 575156786..bb87848f0 100755
--- a/showvotes.cgi
+++ b/showvotes.cgi
@@ -28,50 +28,51 @@ require "CGI.pl";
ConnectToDatabase();
+if (defined $::FORM{'voteon'} || (!defined $::FORM{'bug_id'} &&
+ !defined $::FORM{'user'})) {
+ confirm_login();
+ $::FORM{'user'} = DBNameToIdAndCheck($::COOKIE{'Bugzilla_login'});
+} else {
+ # Check whether or not the user is currently logged in without throwing
+ # an error if the user is not logged in. This function sets the value
+ # of $::usergroupset, the binary number that records the set of groups
+ # to which the user belongs and which gets used in ValidateBugID below
+ # to determine whether or not the user is authorized to access the bug
+ # whose votes are being shown or which is being voted on.
+ quietly_check_login();
+}
+
################################################################################
-# START Form Data Validation
+# Begin Data/Security Validation
################################################################################
-# For security and correctness, validate the value of the "voteon" form variable.
-# Valid values are those containing a number that is the ID of an existing bug.
-if (defined $::FORM{'voteon'}) {
- $::FORM{'voteon'} =~ /^(\d+)$/;
- $::FORM{'voteon'} = $1 || 0;
- SendSQL("SELECT bug_id FROM bugs WHERE bug_id = $::FORM{'voteon'}");
- FetchSQLData()
- || DisplayError("You entered an invalid bug number to vote on.") && exit;
+# Make sure the bug ID is a positive integer representing an existing
+# bug that the user is authorized to access.
+if (defined $::FORM{'bug_id'}) {
+ ValidateBugID($::FORM{'bug_id'});
}
-# For security and correctness, validate the value of the "bug_id" form variable.
-# Valid values are those containing a number that is the ID of an existing bug.
-if (defined $::FORM{'bug_id'}) {
- $::FORM{'bug_id'} =~ /^(\d+)$/;
- $::FORM{'bug_id'} = $1 || 0;
- SendSQL("SELECT bug_id FROM bugs WHERE bug_id = $::FORM{'bug_id'}");
- FetchSQLData()
- || DisplayError("You entered an invalid bug number.") && exit;
+# Make sure the bug ID being voted on is a positive integer representing
+# an existing bug that the user is authorized to access.
+if (defined $::FORM{'voteon'}) {
+ ValidateBugID($::FORM{'voteon'});
}
-# For security and correctness, validate the value of the "userid" form variable.
-# Valid values are those containing a number that is the ID of an existing user.
+# Make sure the user ID is a positive integer representing an existing user.
if (defined $::FORM{'user'}) {
- $::FORM{'user'} =~ /^(\d+)$/;
- $::FORM{'user'} = $1 || 0;
- SendSQL("SELECT userid FROM profiles WHERE userid = $::FORM{'user'}");
+ $::FORM{'user'} =~ /^([1-9][0-9]*)$/
+ || DisplayError("The user number is invalid.")
+ && exit;
+ SendSQL("SELECT 1 FROM profiles WHERE userid = $::FORM{'user'}");
FetchSQLData()
- || DisplayError("You specified an invalid user number.") && exit;
+ || DisplayError("User #$::FORM{'user'} does not exist.")
+ && exit;
}
################################################################################
-# END Form Data Validation
+# End Data/Security Validation
################################################################################
-if (defined $::FORM{'voteon'} || (!defined $::FORM{'bug_id'} &&
- !defined $::FORM{'user'})) {
- confirm_login();
- $::FORM{'user'} = DBNameToIdAndCheck($::COOKIE{'Bugzilla_login'});
-}
-
print "Content-type: text/html\n\n";
if (defined $::FORM{'bug_id'}) {
--
2.24.1