Landing Myk's patch for bug #71767

CGI.pl

@@ -227,52 +227,70 @@ 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;
+    # 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;
+
+    # Get the values of the usergroupset and userid global variables
+    # and write them to local variables for use within this function,
+    # setting those local variables to the default value of zero if
+    # the global variables are undefined.
+
+    # "usergroupset" stores the set of groups the user is a member of,
+    # while "userid" stores the user's unique ID.  These variables are
+    # set globally by either confirm_login() or quietly_check_login(),
+    # one of which should be run before calling this function; otherwise
+    # this function will treat the user as if they were not logged in
+    # and throw an error if they try to access a bug that requires
+    # permissions/authorization to access.
+    my $usergroupset = $::usergroupset || 0;
+    my $userid = $::userid || 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
+      || (
+           $userid ?
+           DisplayError("You are not authorized to access bug #$id.") : 
+           DisplayError(
+             qq|You are not authorized to access bug #$id. 
+             To see this bug, you must first 
+             <a href="show_bug.cgi?id=$id&GoAheadAndLogIn=1">log in</a> 
+             to an account with the appropriate permissions.|
+           )
+         )
+      && exit;
 }
 
 # check and see if a given string actually represents a positive

process_bug.cgi

@@ -58,19 +58,33 @@ my $requiremilestone = 0;
 # named "id_x" where "x" is the bug number.
 my @idlist;
 if (defined $::FORM{'id'}) {
-  push @idlist, $::FORM{'id'};
+    push @idlist, $::FORM{'id'};
 } else {
-  foreach my $i (keys %::FORM) {
-    if ($i =~ /^id_([1-9][0-9]*)/) {
-      push @idlist, $1;
+    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);
+    ValidateBugID($id);
+}
+
+# If the user has a bug list and is processing one bug, then after
+# we process the bug we are going to show them the next bug on their
+# list.  Thus we have to make sure this bug ID is also valid,
+# since a malicious cracker might alter their cookies for the purpose
+# gaining access to bugs they are not authorized to access.
+if ( $::COOKIE{"BUGLIST"} ne "" && defined $::FORM{'id'} ) {
+    my @buglist = split( /:/ , $::COOKIE{"BUGLIST"} );
+    my $idx = lsearch( \@buglist , $::FORM{"id"} );
+    if ($idx < $#buglist) {
+        my $nextbugid = $buglist[$idx + 1];
+        ValidateBugID($nextbugid);
+    }
 }
 
 ######################################################################

show_bug.cgi

@@ -29,12 +29,28 @@ ConnectToDatabase();
 
 if ($::FORM{'GoAheadAndLogIn'}) {
     confirm_login();
+} else {
+    quietly_check_login();
 }
 
+######################################################################
+# 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
+######################################################################
+
 print "Content-type: text/html\n";
 print "\n";
 
-if (!defined $::FORM{'id'} || $::FORM{'id'} !~ /^\s*\d+\s*$/) {
+if (!defined $::FORM{'id'}) {
     PutHeader("Search by bug number");
     print "<FORM METHOD=GET ACTION=\"show_bug.cgi\">\n";
     print "You may find a single bug by entering its bug id here: \n";
@@ -47,14 +63,13 @@ if (!defined $::FORM{'id'} || $::FORM{'id'} !~ /^\s*\d+\s*$/) {
 
 GetVersionTable();
 
-SendSQL("select short_desc, groupset from bugs where bug_id = $::FORM{'id'}");
-my ($summary, $groupset) = FetchSQLData();
-if( $summary && $groupset == 0) {
-    $summary = html_quote($summary);
-    PutHeader("Bug $::FORM{'id'} - $summary", "Bugzilla Bug $::FORM{'id'}", $summary );
-}else {
-    PutHeader("Bugzilla bug $::FORM{'id'}", "Bugzilla Bug", $::FORM{'id'});
-}
+# Get the bug's summary (short description) and display it as
+# the page title.
+SendSQL("SELECT short_desc FROM bugs WHERE bug_id = $::FORM{'id'}");
+my ($summary) = FetchSQLData();
+$summary = html_quote($summary);
+PutHeader("Bug $::FORM{'id'} - $summary", "Bugzilla Bug $::FORM{'id'}", $summary );
+
 navigation_header();
 
 print "<HR>\n";