Bugzilla was leaking information about bugs marked secure (using bug groups).…

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.
 # 

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  = '';

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";

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 {

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'}) {