diff options
| -rwxr-xr-x | Bug.pm | 4 | ||||
| -rwxr-xr-x | Bugzilla/Bug.pm | 4 | ||||
| -rw-r--r-- | CGI.pl | 84 | ||||
| -rwxr-xr-x | buglist.cgi | 12 | ||||
| -rwxr-xr-x | duplicates.cgi | 14 | ||||
| -rw-r--r-- | globals.pl | 118 | ||||
| -rwxr-xr-x | long_list.cgi | 7 | ||||
| -rwxr-xr-x | process_bug.cgi | 46 | ||||
| -rwxr-xr-x | processmail | 30 | ||||
| -rwxr-xr-x | showdependencygraph.cgi | 8 | ||||
| -rwxr-xr-x | showdependencytree.cgi | 8 |
11 files changed, 177 insertions, 158 deletions
@@ -113,10 +113,9 @@ sub initBug { groupset, delta_ts, sum(votes.count) from bugs left join votes using(bug_id) where bugs.bug_id = $bug_id - and bugs.groupset & $usergroupset = bugs.groupset group by bugs.bug_id"; - &::SendSQL($query); + &::SendSQL(&::SelectVisible($query, $user_id, $usergroupset)); my @row; if (@row = &::FetchSQLData()) { @@ -445,6 +444,7 @@ sub Collision { my $write = "WRITE"; # Might want to make a param to control # whether we do LOW_PRIORITY ... &::SendSQL("LOCK TABLES bugs $write, bugs_activity $write, cc $write, " . + "cc AS selectVisible_cc $write, " . "profiles $write, dependencies $write, votes $write, " . "keywords $write, longdescs $write, fielddefs $write, " . "keyworddefs READ, groups READ, attachments READ, products READ"); diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index 76ea56156..8b4243c25 100755 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -113,10 +113,9 @@ sub initBug { groupset, delta_ts, sum(votes.count) from bugs left join votes using(bug_id) where bugs.bug_id = $bug_id - and bugs.groupset & $usergroupset = bugs.groupset group by bugs.bug_id"; - &::SendSQL($query); + &::SendSQL(&::SelectVisible($query, $user_id, $usergroupset)); my @row; if (@row = &::FetchSQLData()) { @@ -445,6 +444,7 @@ sub Collision { my $write = "WRITE"; # Might want to make a param to control # whether we do LOW_PRIORITY ... &::SendSQL("LOCK TABLES bugs $write, bugs_activity $write, cc $write, " . + "cc AS selectVisible_cc $write, " . "profiles $write, dependencies $write, votes $write, " . "keywords $write, longdescs $write, fielddefs $write, " . "keyworddefs READ, groups READ, attachments READ, products READ"); @@ -258,88 +258,20 @@ sub ValidateBugID { # 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. - - # A user is also authorized to access a bug if she is the reporter, - # assignee, QA contact, or member of the cc: list of the bug and the bug - # allows users in those roles to see the bug. The boolean fields - # reporter_accessible, assignee_accessible, qacontact_accessible, and - # cclist_accessible identify whether or not those roles can see 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. - - # Get data from the database about whether or not the user belongs to - # all groups the bug is in, and who are the bug's reporter and qa_contact - # along with which roles can always access the bug. - SendSQL("SELECT ((groupset & $usergroupset) = groupset) , reporter , assigned_to , qa_contact , - reporter_accessible , assignee_accessible , qacontact_accessible , cclist_accessible - FROM bugs - WHERE bug_id = $id"); - - # Make sure the bug exists in the database. - MoreSQLData() - || DisplayError("Bug #$id does not exist.") - && exit; - - my ($isauthorized, $reporter, $assignee, $qacontact, $reporter_accessible, - $assignee_accessible, $qacontact_accessible, $cclist_accessible) = FetchSQLData(); + # First check that the bug exists + SendSQL("SELECT bug_id FROM bugs WHERE bug_id = $id"); - # Finish validation and return if the user is a member of all groups to which the bug belongs. - return if $isauthorized; - - # Finish validation and return if the user is in a role that has access to the bug. - if ($userid) { - return - if ($reporter_accessible && $reporter == $userid) - || ($assignee_accessible && $assignee == $userid) - || ($qacontact_accessible && $qacontact == $userid); - } - - # Try to authorize the user one more time by seeing if they are on - # the cc: list. If so, finish validation and return. - if ( $cclist_accessible ) { - my @cclist; - SendSQL("SELECT cc.who - FROM bugs , cc - WHERE bugs.bug_id = $id - AND cc.bug_id = bugs.bug_id - "); - while (my ($ccwho) = FetchSQLData()) { - # more efficient to just check the var here instead of - # creating a potentially huge array to grep against - return if ($userid == $ccwho); - } + FetchOneColumn() + || DisplayError("Bug #$id does not exist.") + && exit; - } + return if CanSeeBug($id, $::userid, $::usergroupset); # The user did not pass any of the authorization tests, which means they # are not authorized to see the bug. Display an error and stop execution. # The error the user sees depends on whether or not they are logged in - # (i.e. $userid contains the user's positive integer ID). - if ($userid) { + # (i.e. $::userid contains the user's positive integer ID). + if ($::userid) { DisplayError("You are not authorized to access bug #$id."); } else { DisplayError( diff --git a/buglist.cgi b/buglist.cgi index c22475837..120234471 100755 --- a/buglist.cgi +++ b/buglist.cgi @@ -37,6 +37,7 @@ sub sillyness { $zz = $::db_name; $zz = $::defaultqueryname; $zz = $::unconfirmedstate; + $zz = $::userid; $zz = @::components; $zz = @::default_column_list; $zz = @::legal_keywords; @@ -162,9 +163,7 @@ sub GenerateSQL { "LEFT JOIN profiles map_qa_contact ON bugs.qa_contact = map_qa_contact.userid")); unshift(@wherepart, ("bugs.assigned_to = map_assigned_to.userid", - "bugs.reporter = map_reporter.userid", - "bugs.groupset & $::usergroupset = bugs.groupset")); - + "bugs.reporter = map_reporter.userid")); my $minvotes; if (defined $F{'votes'}) { @@ -805,10 +804,14 @@ sub GenerateSQL { $suppseen{$str} = 1; } } + my $query = ("SELECT " . join(', ', @fields) . " FROM $suppstring" . " WHERE " . join(' AND ', (@wherepart, @andlist)) . " GROUP BY bugs.bug_id"); + + $query = SelectVisible($query, $::userid, $::usergroupset); + if ($debug) { print "<P><CODE>" . value_quote($query) . "</CODE><P>\n"; exit(); @@ -1069,8 +1072,6 @@ ReconnectToShadowDatabase(); my $query = GenerateSQL(\@fields, undef, undef, $::buffer); - - if ($::COOKIE{'LASTORDER'}) { if ((!$::FORM{'order'}) || $::FORM{'order'} =~ /^reuse/i) { $::FORM{'order'} = url_decode($::COOKIE{'LASTORDER'}); @@ -1132,6 +1133,7 @@ if ($::FORM{'debug'} && $serverpush) { if (Param('expectbigqueries')) { SendSQL("set option SQL_BIG_TABLES=1"); } + SendSQL($query); my $count = 0; diff --git a/duplicates.cgi b/duplicates.cgi index d1640cbf6..cd2d14c2a 100755 --- a/duplicates.cgi +++ b/duplicates.cgi @@ -33,6 +33,12 @@ require "CGI.pl"; ConnectToDatabase(1); GetVersionTable(); +quietly_check_login(); + +# Silly used-once warnings +$::userid = $::userid; +$::usergroupset = $::usergroupset; + my %dbmcount; my %count; my $dobefore = 0; @@ -195,10 +201,10 @@ $i = 0; foreach (@sortedcount) { my $id = $_; - SendSQL("SELECT component, bug_severity, op_sys, target_milestone, short_desc, groupset, bug_status, resolution" . - " FROM bugs WHERE bug_id = $id"); - my ($component, $severity, $op_sys, $milestone, $summary, $groupset, $bug_status, $resolution) = FetchSQLData(); - next unless $groupset == 0; + SendSQL(SelectVisible("SELECT component, bug_severity, op_sys, target_milestone, short_desc, bug_status, resolution" . + " FROM bugs WHERE bugs.bug_id = $id", $::userid, $::usergroupset)); + next unless MoreSQLData(); + my ($component, $severity, $op_sys, $milestone, $summary, $bug_status, $resolution) = FetchSQLData(); $summary = html_quote($summary); # Show all bugs except those CLOSED _OR_ VERIFIED but not INVALID or WONTFIX. diff --git a/globals.pl b/globals.pl index 32a55d143..7df3bfd40 100644 --- a/globals.pl +++ b/globals.pl @@ -20,6 +20,7 @@ # Contributor(s): Terry Weissman <terry@mozilla.org> # Dan Mosedale <dmose@mozilla.org> # Jake <jake@acutex.net> +# Bradley Baetz <bbaetz@cs.mcgill.ca> # Contains some global variables and routines used throughout bugzilla. @@ -71,8 +72,9 @@ use Date::Parse; # For str2time(). #use Carp; # for confess use RelationSet; -# $ENV{PATH} is not taint safe +# Some environment variables are not taint safe delete $ENV{PATH}; +delete $ENV{BASH_ENV}; # Contains the version string for the current running Bugzilla. $::param{'version'} = '2.15'; @@ -704,6 +706,98 @@ sub GenerateRandomPassword { return $password; } +sub SelectVisible { + my ($query, $userid, $usergroupset) = @_; + + # Run the SQL $query with the additional restriction that + # the bugs can be seen by $userid. $usergroupset is provided + # as an optimisation when this is already known, eg from CGI.pl + # If not present, it will be obtained from the db. + # Assumes that 'bugs' is mentioned as a table name. You should + # also make sure that bug_id is qualified bugs.bug_id! + # Your query must have a WHERE clause. This is unlikely to be a problem. + + # Also, note that mySQL requires aliases for tables to be locked, as well + # This means that if you change the name from selectVisible_cc (or add + # additional tables), you will need to update anywhere which does a + # LOCK TABLE, and then calls routines which call this + + $usergroupset = 0 unless $userid; + + unless (defined($usergroupset)) { + PushGlobalSQLState(); + SendSQL("SELECT groupset FROM profiles WHERE userid = $userid"); + $usergroupset = FetchOneColumn(); + PopGlobalSQLState(); + } + + # 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. + + # A user is also authorized to access a bug if she is the reporter, + # assignee, QA contact, or member of the cc: list of the bug and the bug + # allows users in those roles to see the bug. The boolean fields + # reporter_accessible, assignee_accessible, qacontact_accessible, and + # cclist_accessible identify whether or not those roles can see 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. + + my $replace = " "; + + if ($userid) { + $replace .= "LEFT JOIN cc selectVisible_cc ON + bugs.bug_id = selectVisible_cc.bug_id AND + selectVisible_cc.who = $userid " + } + + $replace .= "WHERE ((bugs.groupset & $usergroupset) = bugs.groupset "; + + if ($userid) { + # There is a mysql bug affecting v3.22 and 3.23 (at least), where this will + # cause all rows to be returned! We work arround this by adding an not isnull + # test to the JOINed cc table. See http://lists.mysql.com/cgi-ez/ezmlm-cgi?9:mss:11417 + # Its needed, even though it shouldn't be + $replace .= "OR (bugs.reporter_accessible = 1 AND bugs.reporter = $userid) + OR (bugs.assignee_accessible = 1 AND bugs.assigned_to = $userid) + OR (bugs.qacontact_accessible = 1 AND bugs.qa_contact = $userid) + OR (bugs.cclist_accessible = 1 AND selectVisible_cc.who = $userid AND not isnull(selectVisible_cc.who))"; + } + + $replace .= ") AND "; + + $query =~ s/\sWHERE\s/$replace/i; + + return $query; +} + +sub CanSeeBug { + # Note that we pass in the usergroupset, since this is known + # in most cases (ie viewing bugs). Maybe make this an optional + # parameter? + + my ($id, $userid, $usergroupset) = @_; + + # Query the database for the bug, retrieving a boolean value that + # represents whether or not the user is authorized to access the bug. + + PushGlobalSQLState(); + SendSQL(SelectVisible("SELECT bugs.bug_id FROM bugs WHERE bugs.bug_id = $id", + $userid, $usergroupset)); + + my $ret = defined(FetchSQLData()); + PopGlobalSQLState(); + + return $ret; +} sub ValidatePassword { # Determines whether or not a password is valid (i.e. meets Bugzilla's @@ -835,16 +929,22 @@ sub DBNameToIdAndCheck { exit(0); } -# Use detaint_string() when you know that there is no way that the data +# Use trick_taint() when you know that there is no way that the data # in a scalar can be tainted, but taint mode still bails on it. # WARNING!! Using this routine on data that really could be tainted # defeats the purpose of taint mode. It should only be # used on variables that cannot be touched by users. -sub detaint_string { - my ($str) = @_; - $str =~ m/^(.*)$/s; - $str = $1; +sub trick_taint { + $_[0] =~ /^(.*)$/s; + $_[0] = $1; + return (defined($_[0])); +} + +sub detaint_natural { + $_[0] =~ /^(\d+)$/; + $_[0] = $1; + return (defined($_[0])); } # This routine quoteUrls contains inspirations from the HTML::FromText CPAN @@ -983,7 +1083,9 @@ sub GetBugLink { $link_text = value_quote($link_text); $link_return .= qq{<a href="show_bug.cgi?id=$bug_num" title="$bug_stat}; if ($bug_res ne "") {$link_return .= " $bug_res"} - if ($bug_grp == 0) { $link_return .= " - $bug_desc" } + if ($bug_grp == 0 || CanSeeBug($bug_num, $::userid, $::usergroupset)) { + $link_return .= " - $bug_desc"; + } $link_return .= qq{">$link_text</a>}; if ($bug_res ne "") { $link_return .= "</strike>" } if ($bug_stat eq "UNCONFIRMED") { $link_return .= "</i>"} @@ -1128,7 +1230,7 @@ sub SqlQuote { $str =~ s/([\\\'])/\\$1/g; $str =~ s/\0/\\0/g; # If it's been SqlQuote()ed, then it's safe, so we tell -T that. - $str = detaint_string($str); + trick_taint($str); return "'$str'"; } diff --git a/long_list.cgi b/long_list.cgi index 494103823..0cde1e93a 100755 --- a/long_list.cgi +++ b/long_list.cgi @@ -32,6 +32,7 @@ require "CGI.pl"; sub sillyness { my $zz; $zz = $::legal_keywords; + $zz = $::userid; $zz = $::usergroupset; $zz = %::FORM; } @@ -68,12 +69,12 @@ select bugs.status_whiteboard, bugs.keywords from bugs,profiles assign,profiles report -where assign.userid = bugs.assigned_to and report.userid = bugs.reporter and -bugs.groupset & $::usergroupset = bugs.groupset and"; +where assign.userid = bugs.assigned_to and report.userid = bugs.reporter and"; $::FORM{'buglist'} = "" unless exists $::FORM{'buglist'}; foreach my $bug (split(/:/, $::FORM{'buglist'})) { - SendSQL("$generic_query bugs.bug_id = $bug"); + SendSQL(SelectVisible("$generic_query bugs.bug_id = $bug", + $::userid, $::usergroupset)); my @row; if (@row = FetchSQLData()) { diff --git a/process_bug.cgi b/process_bug.cgi index 0882db0ae..416ee9ccd 100755 --- a/process_bug.cgi +++ b/process_bug.cgi @@ -352,6 +352,11 @@ empowered user, may make that change to the $f field. # Confirm that the reporter of the current bug can access the bug we are duping to. sub DuplicateUserConfirm { + # if we've already been through here, then exit + if (defined $::FORM{'confirm_add_duplicate'}) { + return; + } + my $dupe = trim($::FORM{'id'}); my $original = trim($::FORM{'dup_id'}); @@ -360,45 +365,13 @@ sub DuplicateUserConfirm { SendSQL("SELECT profiles.groupset FROM profiles WHERE profiles.userid =".SqlQuote($reporter)); my $reportergroupset = FetchOneColumn(); - SendSQL("SELECT ((groupset & $reportergroupset) = groupset) , reporter , assigned_to , qa_contact , - reporter_accessible , assignee_accessible , qacontact_accessible , cclist_accessible - FROM bugs - WHERE bug_id = $original"); - - my ($isauthorized, $originalreporter, $assignee, $qacontact, $reporter_accessible, - $assignee_accessible, $qacontact_accessible, $cclist_accessible) = FetchSQLData(); - - # If reporter is authorized via the database, or is the original reporter, assignee, - # or QA Contact, we'll automatically confirm they can be added to the cc list - if ($isauthorized - || ($reporter_accessible && $originalreporter == $reporter) - || ($assignee_accessible && $assignee == $reporter) - || ($qacontact_accessible && $qacontact == $reporter)) { - + if (CanSeeBug($original, $reporter, $reportergroupset)) { $::FORM{'confirm_add_duplicate'} = "1"; - return; - } - - # Try to authorize the user one more time by seeing if they are on - # the cc: list. If so, finish validation and return. - if ($cclist_accessible ) { - my @cclist; - SendSQL("SELECT cc.who - FROM bugs , cc - WHERE bugs.bug_id = $original - AND cc.bug_id = bugs.bug_id - "); - while (my ($ccwho) = FetchSQLData()) { - if ($reporter == $ccwho) { - $::FORM{'confirm_add_duplicate'} = "1"; - return; - } - } - } - - if (defined $::FORM{'confirm_add_duplicate'}) { return; } + + SendSQL("SELECT cclist_accessible FROM bugs WHERE bug_id = $original"); + my $cclist_accessible = FetchOneColumn(); # Once in this part of the subroutine, the user has not been auto-validated # and the duper has not chosen whether or not to add to CC list, so let's @@ -985,6 +958,7 @@ foreach my $id (@idlist) { my $write = "WRITE"; # Might want to make a param to control # whether we do LOW_PRIORITY ... SendSQL("LOCK TABLES bugs $write, bugs_activity $write, cc $write, " . + "cc AS selectVisible_cc $write, " . "profiles $write, dependencies $write, votes $write, " . "keywords $write, longdescs $write, fielddefs $write, " . "keyworddefs READ, groups READ, attachments READ, products READ"); diff --git a/processmail b/processmail index 22dc85cc3..75a7b4bf9 100755 --- a/processmail +++ b/processmail @@ -111,8 +111,8 @@ sub ProcessOneBug { } my ($start, $end) = (@row); # $start and $end are considered safe because users can't touch them - $start = detaint_string($start); - $end = detaint_string($end); + trick_taint($start); + trick_taint($end); my $ccSet = new RelationSet(); $ccSet->mergeFromDB("SELECT who FROM cc WHERE bug_id = $id"); @@ -644,31 +644,26 @@ sub NewProcessOnePerson ($$$$$$$$$$$) { if ($nomail{$person}) { return; } + - # Sanitize $values{'groupset'} - if ($values{'groupset'} =~ m/(\d+)/) { - $values{'groupset'} = $1; - } else { - $values{'groupset'} = 0; - } - SendSQL("SELECT userid, groupset & $values{'groupset'} " . + SendSQL("SELECT userid, groupset " . "FROM profiles WHERE login_name = " . SqlQuote($person)); my ($userid, $groupset) = (FetchSQLData()); - + $seen{$person} = 1; + detaint_natural($userid); + detaint_natural($groupset); # if this person doesn't have permission to see info on this bug, # return. # - # XXX - I _think_ this currently means that if a bug is suddenly given + # XXX - This currently means that if a bug is suddenly given # more restrictive permissions, people without those permissions won't # see the action of restricting the bug itself; the bug will just # quietly disappear from their radar. # - if ($groupset ne $values{'groupset'}) { - return; - } + return unless CanSeeBug($id, $userid, $groupset); my %mailhead = %defmailhead; @@ -824,9 +819,10 @@ if ($ARGV[0] eq "rescanall") { push @list, $row[0]; } foreach my $id (@list) { - $ARGV[0] = $id; - print "<br> Doing bug $id\n"; - ProcessOneBug($ARGV[0]); + if (detaint_natural($id)) { + print "<br> Doing bug $id\n"; + ProcessOneBug($id); + } } } else { my $bugnum; diff --git a/showdependencygraph.cgi b/showdependencygraph.cgi index df377c096..c62506c45 100755 --- a/showdependencygraph.cgi +++ b/showdependencygraph.cgi @@ -29,7 +29,9 @@ ConnectToDatabase(); quietly_check_login(); -$::usergroupset = $::usergroupset; # More warning suppression silliness. +# More warning suppression silliness. +$::userid = $::userid; +$::usergroupset = $::usergroupset; ###################################################################### # Begin Data/Security Validation @@ -122,7 +124,9 @@ node [URL="${urlbase}show_bug.cgi?id=\\N", style=filled, color=lightgrey] my $summary = ""; my $stat; if ($::FORM{'showsummary'}) { - SendSQL("select bug_status, short_desc from bugs where bug_id = $k and bugs.groupset & $::usergroupset = bugs.groupset"); + SendSQL(SelectVisible("select bug_status, short_desc from bugs where bug_id = $k", + $::userid, + $::usergroupset)); ($stat, $summary) = (FetchSQLData()); $stat = "NEW" if !defined $stat; $summary = "" if !defined $summary; diff --git a/showdependencytree.cgi b/showdependencytree.cgi index 1d6e3dbb2..a6a72eb3f 100755 --- a/showdependencytree.cgi +++ b/showdependencytree.cgi @@ -34,7 +34,9 @@ ConnectToDatabase(); quietly_check_login(); -$::usergroupset = $::usergroupset; # More warning suppression silliness. +# More warning suppression silliness. +$::userid = $::userid; +$::usergroupset = $::usergroupset; ###################################################################### # Begin Data/Security Validation @@ -128,10 +130,10 @@ sub DumpKids { my ($bugid, $stat, $milestone) = ("", "", ""); my ($userid, $short_desc) = ("", ""); if (Param('usetargetmilestone')) { - SendSQL("select bug_id, bug_status, target_milestone, assigned_to, short_desc from bugs where bug_id = $kid and bugs.groupset & $::usergroupset = bugs.groupset"); + SendSQL(SelectVisible("select bugs.bug_id, bug_status, target_milestone, assigned_to, short_desc from bugs where bugs.bug_id = $kid", $::userid, $::usergroupset)); ($bugid, $stat, $milestone, $userid, $short_desc) = (FetchSQLData()); } else { - SendSQL("select bug_id, bug_status, assigned_to, short_desc from bugs where bug_id = $kid and bugs.groupset & $::usergroupset = bugs.groupset"); + SendSQL(SelectVisible("select bugs.bug_id, bug_status, assigned_to, short_desc from bugs where bugs.bug_id = $kid", $::userid, $::usergroupset)); ($bugid, $stat, $userid, $short_desc) = (FetchSQLData()); } |