From: mkanat%kerio.com <> Date: Fri, 8 Jul 2005 12:30:58 +0000 (+0000) Subject: Bug 293159: [SECURITY] Anyone can change flags and access bug summaries due to a... X-Git-Tag: bugzilla-2.18.2~2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=37b443bd90f0837301ab69bcb08355c637086e33;p=thirdparty%2Fbugzilla.git 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 r=myk, a=justdave --- diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm index 37de4a7202..1b00676742 100644 --- a/Bugzilla/Flag.pm +++ b/Bugzilla/Flag.pm @@ -139,17 +139,39 @@ sub count { sub validate { # Validates fields containing flag modifications. - - my ($data, $bug_id) = @_; + my ($data, $bug_id, $attach_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 : (), keys %$data); - - foreach my $id (@ids) - { + + return unless scalar(@ids); + + # No flag reference should exist when changing several bugs at once. + ThrowCodeError("flags_not_available") unless $bug_id; + + # 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) + 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 = $data->{"flag-$id"}; # Make sure the flag exists. @@ -173,45 +195,59 @@ 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($data->{"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($data->{"requestee-$id"})) + && $new_requestee + && ($old_requestee ne $new_requestee)) { - my $requestee_email = trim($data->{"requestee-$id"}); - if ($requestee_email ne $flag->{'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 (!&::CanSeeBug($bug_id, $requestee->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} - && $data->{'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. + if (!&::CanSeeBug($bug_id, $requestee->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} + && $data->{'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 b28a40c143..58e7eb0bde 100644 --- a/Bugzilla/FlagType.pm +++ b/Bugzilla/FlagType.pm @@ -194,15 +194,28 @@ sub count { sub validate { my ($data, $bug_id, $attach_id) = @_; - + my $dbh = Bugzilla->dbh; + # Get a list of flag types to validate. Uses the "map" function # to extract flag type IDs from form field names by matching columns # whose name looks like "flag_type-nnn", where "nnn" is the ID, # and returning just the ID portion of matching field names. my @ids = map(/^flag_type-(\d+)$/ ? $1 : (), keys %$data); - - foreach my $id (@ids) - { + + return unless scalar(@ids); + + # No flag reference should exist when changing several bugs at once. + ThrowCodeError("flags_not_available") unless $bug_id; + + # 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 LIMIT 1"); + + ThrowCodeError("flag_type_inactive") if $inactive_flagtypes; + + foreach my $id (@ids) { my $status = $data->{"flag_type-$id"}; # Don't bother validating types the user didn't touch. @@ -223,23 +236,32 @@ 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. + my $new_requestee = trim($data->{"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($data->{"requestee_type-$id"})) + && $new_requestee) { - my $requestee_email = trim($data->{"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 (!&::CanSeeBug($bug_id, $requestee->id)) - { + if (!&::CanSeeBug($bug_id, $requestee->id)) { ThrowUserError("flag_requestee_unauthorized", { flag_type => $flag_type, requestee => $requestee, diff --git a/attachment.cgi b/attachment.cgi index ccd55fce5c..6b863d0ea4 100755 --- a/attachment.cgi +++ b/attachment.cgi @@ -146,7 +146,7 @@ elsif ($action eq "update") # in the requestee fields are legitimate user email addresses. Bugzilla::User::match_field({ '^requestee(_type)?-(\d+)$' => { 'type' => 'single' } }); - Bugzilla::Flag::validate(\%::FORM, $bugid); + Bugzilla::Flag::validate(\%::FORM, $bugid, $::FORM{'id'}); Bugzilla::FlagType::validate(\%::FORM, $bugid, $::FORM{'id'}); update(); diff --git a/process_bug.cgi b/process_bug.cgi index c78673af78..cf1046b69c 100755 --- a/process_bug.cgi +++ b/process_bug.cgi @@ -145,12 +145,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 $::FORM{'id'}) { - Bugzilla::Flag::validate(\%::FORM, $::FORM{'id'}); - Bugzilla::FlagType::validate(\%::FORM, $::FORM{'id'}); -} + +# Validate flags in all cases. validate() should not detect any +# reference to flags if $::FORM{'id'} is undefined. +Bugzilla::Flag::validate(\%::FORM, $::FORM{'id'}); +Bugzilla::FlagType::validate(\%::FORM, $::FORM{'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 5053f0b2fe..063c82dd01 100644 --- a/template/en/default/global/code-error.html.tmpl +++ b/template/en/default/global/code-error.html.tmpl @@ -145,10 +145,28 @@ is attached to [% terms.bug %] [%+ attach_bug_id FILTER html %], but you tried to flag it as obsolete while creating a new attachment to [% terms.bug %] [%+ my_bug_id FILTER html %]. - + + [% 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 == "flag_nonexistent" %] There is no flag with ID #[% id FILTER html %]. - + + [% ELSIF error == "flags_not_available" %] + [% title = "Flag Editing not Allowed" %] + Flags cannot be set or changed when + changing several [% terms.bugs %] at once. + + [% ELSIF error == "flag_requestee_disabled" %] + [% title = "Flag not Specifically Requestable" %] + The flag [% name FILTER html %] is not specifically requestable. + [% ELSIF error == "flag_status_invalid" %] The flag status [% status FILTER html %] [% IF id %] @@ -167,6 +185,10 @@ The flag type ID [% id FILTER html %] 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 [% id FILTER html %].