Bug 293159: [SECURITY] Anyone can change flags and access bug summaries due to a bad check in Flag::validate() and Flag::modify()

Patch By Frederic Buclin <LpSolit@gmail.com> r=myk, a=justdave

5 files changed, 159 insertions, 60 deletions

diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index f19369c24..b0a0586c2 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -235,17 +235,47 @@ Validates fields containing flag modifications.
 =cut
 
 sub validate {
+    my ($cgi, $bug_id, $attach_id) = @_;
+
     my $user = Bugzilla->user;
-    my ($cgi, $bug_id) = @_;
-  
+    my $dbh = Bugzilla->dbh;
+
     # Get a list of flags to validate.  Uses the "map" function
     # to extract flag IDs from form field names by matching fields
     # whose name looks like "flag-nnn", where "nnn" is the ID,
     # and returning just the ID portion of matching field names.
     my @ids = map(/^flag-(\d+)$/ ? $1 : (), $cgi->param());
-  
-    foreach my $id (@ids)
-    {
+
+    return unless scalar(@ids);
+    
+    # No flag reference should exist when changing several bugs at once.
+    ThrowCodeError("flags_not_available", { type => 'b' }) unless $bug_id;
+
+    # No reference to existing flags should exist when creating a new
+    # attachment.
+    if ($attach_id && ($attach_id < 0)) {
+        ThrowCodeError("flags_not_available", { type => 'a' });
+    }
+
+    # Make sure all flags belong to the bug/attachment they pretend to be.
+    my $field = ($attach_id) ? "attach_id" : "bug_id";
+    my $field_id = $attach_id || $bug_id;
+    my $not = ($attach_id) ? "" : "NOT";
+
+    my $invalid_data =
+        $dbh->selectrow_array("SELECT 1 FROM flags
+                               WHERE id IN (" . join(',', @ids) . ")
+                               AND ($field != ? OR attach_id IS $not NULL) " .
+                               $dbh->sql_limit(1),
+                               undef, $field_id);
+
+    if ($invalid_data) {
+        ThrowCodeError("invalid_flag_association",
+                       { bug_id    => $bug_id,
+                         attach_id => $attach_id });
+    }
+
+    foreach my $id (@ids) {
         my $status = $cgi->param("flag-$id");
         
         # Make sure the flag exists.
@@ -269,48 +299,60 @@ sub validate {
             ThrowCodeError("flag_status_invalid", 
                            { id => $id, status => $status });
         }
-        
+
+        # Make sure the user didn't specify a requestee unless the flag
+        # is specifically requestable. If the requestee was set before
+        # the flag became specifically unrequestable, leave it as is.
+        my $old_requestee =
+            $flag->{'requestee'} ? $flag->{'requestee'}->login : '';
+        my $new_requestee = trim($cgi->param("requestee-$id") || '');
+
+        if ($status eq '?'
+            && !$flag->{type}->{is_requesteeble}
+            && $new_requestee
+            && ($new_requestee ne $old_requestee))
+        {
+            ThrowCodeError("flag_requestee_disabled",
+                           { name => $flag->{type}->{name} });
+        }
+
         # Make sure the requestee is authorized to access the bug.
         # (and attachment, if this installation is using the "insider group"
         # feature and the attachment is marked private).
         if ($status eq '?'
             && $flag->{type}->{is_requesteeble}
-            && trim($cgi->param("requestee-$id")))
+            && $new_requestee
+            && ($old_requestee ne $new_requestee))
         {
-            my $requestee_email = trim($cgi->param("requestee-$id"));
-            my $old_requestee = 
-              $flag->{'requestee'} ? $flag->{'requestee'}->login : '';
-
-            if ($old_requestee ne $requestee_email) {
-                # We know the requestee exists because we ran
-                # Bugzilla::User::match_field before getting here.
-                my $requestee = Bugzilla::User->new_from_login($requestee_email);
-
-                # Throw an error if the user can't see the bug.
-                if (!$requestee->can_see_bug($bug_id))
-                {
-                    ThrowUserError("flag_requestee_unauthorized",
-                                   { flag_type => $flag->{'type'},
-                                     requestee => $requestee,
-                                     bug_id => $bug_id,
-                                     attach_id =>
-                                       $flag->{target}->{attachment}->{id} });
-                }
-
-                # Throw an error if the target is a private attachment and
-                # the requestee isn't in the group of insiders who can see it.
-                if ($flag->{target}->{attachment}->{exists}
-                    && $cgi->param('isprivate')
-                    && Param("insidergroup")
-                    && !$requestee->in_group(Param("insidergroup")))
-                {
-                    ThrowUserError("flag_requestee_unauthorized_attachment",
-                                   { flag_type => $flag->{'type'},
-                                     requestee => $requestee,
-                                     bug_id    => $bug_id,
-                                     attach_id =>
-                                      $flag->{target}->{attachment}->{id} });
-                }
+            # We know the requestee exists because we ran
+            # Bugzilla::User::match_field before getting here.
+            my $requestee = Bugzilla::User->new_from_login($new_requestee);
+
+            # Throw an error if the user can't see the bug.
+            # Note that if permissions on this bug are changed,
+            # can_see_bug() will refer to old settings.
+            if (!$requestee->can_see_bug($bug_id)) {
+                ThrowUserError("flag_requestee_unauthorized",
+                               { flag_type => $flag->{'type'},
+                                 requestee => $requestee,
+                                 bug_id => $bug_id,
+                                 attach_id =>
+                                   $flag->{target}->{attachment}->{id} });
+            }
+
+            # Throw an error if the target is a private attachment and
+            # the requestee isn't in the group of insiders who can see it.
+            if ($flag->{target}->{attachment}->{exists}
+                && $cgi->param('isprivate')
+                && Param("insidergroup")
+                && !$requestee->in_group(Param("insidergroup")))
+            {
+                ThrowUserError("flag_requestee_unauthorized_attachment",
+                               { flag_type => $flag->{'type'},
+                                 requestee => $requestee,
+                                 bug_id    => $bug_id,
+                                 attach_id =>
+                                  $flag->{target}->{attachment}->{id} });
             }
         }
 
diff --git a/Bugzilla/FlagType.pm b/Bugzilla/FlagType.pm
index ceeb9a38a..97c6f2c0e 100644
--- a/Bugzilla/FlagType.pm
+++ b/Bugzilla/FlagType.pm
@@ -325,13 +325,32 @@ and returning just the ID portion of matching field names.
 =cut
 
 sub validate {
-    my $user = Bugzilla->user;
     my ($cgi, $bug_id, $attach_id) = @_;
-  
+
+    my $user = Bugzilla->user;
+    my $dbh = Bugzilla->dbh;
+
     my @ids = map(/^flag_type-(\d+)$/ ? $1 : (), $cgi->param());
   
-    foreach my $id (@ids)
-    {
+    return unless scalar(@ids);
+    
+    # No flag reference should exist when changing several bugs at once.
+    ThrowCodeError("flags_not_available", { type => 'b' }) unless $bug_id;
+
+    # We don't check that these flag types are valid for
+    # this bug/attachment. This check will be done later when
+    # processing new flags, see Flag::FormToNewFlags().
+
+    # All flag types have to be active
+    my $inactive_flagtypes =
+        $dbh->selectrow_array("SELECT 1 FROM flagtypes
+                               WHERE id IN (" . join(',', @ids) . ")
+                               AND is_active = 0 " .
+                               $dbh->sql_limit(1));
+
+    ThrowCodeError("flag_type_inactive") if $inactive_flagtypes;
+
+    foreach my $id (@ids) {
         my $status = $cgi->param("flag_type-$id");
         
         # Don't bother validating types the user didn't touch.
@@ -353,22 +372,31 @@ sub validate {
                            { id => $id , status => $status });
         }
         
+        # Make sure the user didn't specify a requestee unless the flag
+        # is specifically requestable.
+        my $new_requestee = trim($cgi->param("requestee_type-$id") || '');
+
+        if ($status eq '?'
+            && !$flag_type->{is_requesteeble}
+            && $new_requestee)
+        {
+            ThrowCodeError("flag_requestee_disabled",
+                           { name => $flag_type->{name} });
+        }
+
         # Make sure the requestee is authorized to access the bug
         # (and attachment, if this installation is using the "insider group"
         # feature and the attachment is marked private).
         if ($status eq '?'
             && $flag_type->{is_requesteeble}
-            && trim($cgi->param("requestee_type-$id")))
+            && $new_requestee)
         {
-            my $requestee_email = trim($cgi->param("requestee_type-$id"));
-
             # We know the requestee exists because we ran
             # Bugzilla::User::match_field before getting here.
-            my $requestee = Bugzilla::User->new_from_login($requestee_email);
+            my $requestee = Bugzilla::User->new_from_login($new_requestee);
 
             # Throw an error if the user can't see the bug.
-            if (!$requestee->can_see_bug($bug_id))
-            {
+            if (!$requestee->can_see_bug($bug_id)) {
                 ThrowUserError("flag_requestee_unauthorized",
                                { flag_type => $flag_type,
                                  requestee => $requestee,
diff --git a/attachment.cgi b/attachment.cgi
index 0c010a061..e4cbe8eed 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -913,8 +913,11 @@ sub insert
         $vars->{'message'} = 'user_match_multiple';
     }
 
-    Bugzilla::Flag::validate($cgi, $bugid);
-    Bugzilla::FlagType::validate($cgi, $bugid, $cgi->param('id'));
+    # Flag::validate() should not detect any reference to existing
+    # flags when creating a new attachment. Setting the third param
+    # to -1 will force this function to check this point.
+    Bugzilla::Flag::validate($cgi, $bugid, -1);
+    Bugzilla::FlagType::validate($cgi, $bugid);
 
     # Escape characters in strings that will be used in SQL statements.
     my $sql_filename = SqlQuote($filename);
@@ -1148,7 +1151,7 @@ sub update
     Bugzilla::User::match_field($cgi, {
         '^requestee(_type)?-(\d+)$' => { 'type' => 'single' }
     });
-    Bugzilla::Flag::validate($cgi, $bugid);
+    Bugzilla::Flag::validate($cgi, $bugid, $attach_id);
     Bugzilla::FlagType::validate($cgi, $bugid, $attach_id);
 
   # Lock database tables in preparation for updating the attachment.
diff --git a/process_bug.cgi b/process_bug.cgi
index 1fa8400e9..4b6410b2c 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -165,12 +165,11 @@ foreach my $field ("dependson", "blocked") {
     'assigned_to'               => { 'type' => 'single' },
     '^requestee(_type)?-(\d+)$' => { 'type' => 'single' },
 });
-# Validate flags, but only if the user is changing a single bug,
-# since the multi-change form doesn't include flag changes.
-if (defined $cgi->param('id')) {
-    Bugzilla::Flag::validate($cgi, $cgi->param('id'));
-    Bugzilla::FlagType::validate($cgi, $cgi->param('id'));
-}
+
+# Validate flags in all cases. validate() should not detect any
+# reference to flags if $cgi->param('id') is undefined.
+Bugzilla::Flag::validate($cgi, $cgi->param('id'));
+Bugzilla::FlagType::validate($cgi, $cgi->param('id'));
 
 ######################################################################
 # End Data/Security Validation
diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl
index fd3f8fb20..36a752949 100644
--- a/template/en/default/global/code-error.html.tmpl
+++ b/template/en/default/global/code-error.html.tmpl
@@ -135,6 +135,15 @@
     [% title = "Invalid Dimensions" %]
     The width or height specified is not a positive integer.
 
+  [% ELSIF error == "invalid_flag_association" %]
+    [% title = "Invalid Flag Association" %]
+    Some flags do not belong to
+    [% IF attach_id %]
+      attachment [% attach_id FILTER html %].
+    [% ELSE %]
+      [%+ terms.bug %] [%+ bug_id FILTER html %].
+    [% END %]
+
   [% ELSIF error == "invalid_isactive_flag" %]
     [% title = "Invalid isactive flag" %]        
     The active flag was improperly set.  There may be
@@ -153,6 +162,20 @@
         
   [% ELSIF error == "flag_nonexistent" %]
     There is no flag with ID #[% id FILTER html %].
+
+  [% ELSIF error == "flags_not_available" %]
+    [% title = "Flag Editing not Allowed" %]
+    [% IF type == "b" %]
+      Flags cannot be set or changed when
+      changing several [% terms.bugs %] at once.
+    [% ELSE %]
+      References to existing flags when creating
+      a new attachment are invalid.
+    [% END %] 
+
+  [% ELSIF error == "flag_requestee_disabled" %]
+    [% title = "Flag not Specifically Requestable" %]
+    The flag <em>[% name FILTER html %]</em> is not specifically requestable.
   
   [% ELSIF error == "flag_status_invalid" %]
     The flag status <em>[% status FILTER html %]</em>
@@ -172,6 +195,10 @@
     The flag type ID <em>[% id FILTER html %]</em> is not
     a positive integer.
 
+  [% ELSIF error == "flag_type_inactive" %]
+    [% title = "Inactive Flag Types" %]
+    Some flag types are inactive and cannot be used to create new flags.
+
   [% ELSIF error == "flag_type_nonexistent" %]
     There is no flag type with the ID <em>[% id FILTER html %]</em>.