305773 - It's possible to bypass flag validation checks

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Frédéric Buclin]

This is again a regression due to bug 292022!

FlagType::validate() and Flag::validate() both only check whether the first
requestee of a given flag (type) is allowed to access a bug or an attachment. It
is then possible to bypass these validations by giving first a "trusted"
requestee name followed by subsequent "unstrusted" requestees.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: fixes problem

Comment 2

[Frédéric Buclin]

Comment on attachment 193850 [details] [diff] [review]
patch v1: fixes problem

>Index: Bugzilla/Flag.pm

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

>+        if ($status eq '?' && !$flag->{type}->{is_requesteeble}) {
>+            my $old_requestee =
>+                $flag->{'requestee'} ? $flag->{'requestee'}->login : '';
>+
>+            # If the user entered multiple requestees, $new_requestee
>+            # will be the concatenation of them, which is fine for this check
>+            # because we only care if the value was changed.
>+            my $new_requestee = trim($cgi->param("requestee-$id") || '');
>+
>+            if ($new_requestee ne $old_requestee) {
>+                ThrowCodeError("flag_requestee_disabled",
>+                               { name => $flag->{type}->{name} });
>+            }
>         }

This doesn't work! $new_requestee is *not* the concatenation of requestees; the
first item of the array is returned, so that I can still bypass this test and
create as many new requestees as desired despite the fact that the flag is not
specifically requestable.

Testcase:

before: [e-mail]
after:	[e-mail], [e-mail]

Your test only checks the first item, i.e. [e-mail], which is unchanged
and the test is successful! Now a new flag is created with [e-mail] as
requestee. :( Moreover, the error is triggered even when you remove the
requestee (!), because you removed the test which checked whether at least one
requestee is given. :(

The correct fix is to write:

  $new_requestee = join(' ', @{$cgi->param("requestee-$id")});

and then to test:

if ($new_requestee && $new_requestee ne $old_requestee)



>Index: Bugzilla/FlagType.pm

>         # Make sure the requestee is authorized to access the bug

Nit: could you update this comment now that more than one requestee is
possible?

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: fixes issues

(In reply to comment #2)
> (From update of attachment 193850 [details] [diff] [review] [edit])
> >Index: Bugzilla/Flag.pm
> 
> >	    # 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.
> 
> >+	    if ($status eq '?' && !$flag->{type}->{is_requesteeble}) {
> >+		my $old_requestee =
> >+		    $flag->{'requestee'} ? $flag->{'requestee'}->login : '';
> >+
> >+		# If the user entered multiple requestees, $new_requestee
> >+		# will be the concatenation of them, which is fine for this
check
> >+		# because we only care if the value was changed.
> >+		my $new_requestee = trim($cgi->param("requestee-$id") || '');
> >+
> >+		if ($new_requestee ne $old_requestee) {
> >+		    ThrowCodeError("flag_requestee_disabled",
> >+				   { name => $flag->{type}->{name} });
> >+		}
> >	    }
> 
> This doesn't work! $new_requestee is *not* the concatenation of requestees;
the
> first item of the array is returned, so that I can still bypass this test and

> create as many new requestees as desired despite the fact that the flag is
not
> specifically requestable.

Good catch.  $cgi->param is context-sensitive, so it returns a single value
when called in scalar context.	Fixed.

> Nit: could you update this comment now that more than one requestee is
> possible?

Yup, done.  I also now check to see if a user is trying to request a
non-multiplicable flag from multiple people and throw an error if so.

Comment 4

[Frédéric Buclin]

Comment on attachment 193968 [details] [diff] [review]
patch v2: fixes issues

>Index: Bugzilla/Flag.pm

>+        if ($status eq '?' && !$flag->{type}->{is_requesteeble}) {
>+            # Make sure the user didn't enter multiple requestees for a flag
>+            # that can't be requested from more than one person at a time.

For me, testing whether the flag is specifically requestable and whether the
flag is multiplicable are two distinct things and one should not depend on the
other one. In this case, if the flag is specifically requestable, the test
about having several flags of this type is not executed. This doesn't make
sense to me.

I would first check whether the flag is requestable, then whether the flag is
specifically requestable and finally whether the flag is multiplicable, in
three DISTINCT tests.


>+            my $new_requestee = join('', $cgi->param("requestee-$id"));
>+            if ($new_requestee && $new_requestee ne $old_requestee) {
>+                ThrowCodeError("flag_requestee_disabled", { flag => $flag });
>+            }

Nit: due to your previous test, you already know that the requestee field is
limited to one name. Why doing a join() on it? I would prefer a trim( .. || '')
as it's done already. join() would make sense only if your test about having
multiple flags is done *after* this test.



>Index: Bugzilla/FlagType.pm

>+        if ($status eq '?' && $flag_type->{is_requesteeble}) {
>+            foreach my $login ($cgi->param("requestee-$id")) {

That's the wrong field. The correct field is "requestee_type-$id".

Also, why don't you test whether the flag type is multiplicable in FlagType.pm?

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: fixes review issues

> I would first check whether the flag is requestable, then whether the flag is

> specifically requestable and finally whether the flag is multiplicable, in
> three DISTINCT tests.

Sure, done.


> Nit: due to your previous test, you already know that the requestee field is
> limited to one name. Why doing a join() on it? I would prefer a trim( .. || 
> '') as it's done already. join() would make sense only if your test about 
> having multiple flags is done *after* this test.

Good point, but now that the multiple flags test is done after this test, per
your first comment above, I've left this the way it is.


> That's the wrong field. The correct field is "requestee_type-$id".

Oops, fixed.


> Also, why don't you test whether the flag type is multiplicable in 
> FlagType.pm?

Because I forgot. :-/  Fixed.

Comment 6

[Frédéric Buclin]

Comment on attachment 194314 [details] [diff] [review]
patch v3: fixes review issues

>Index: Bugzilla/Flag.pm

>     foreach my $id (@ids) {
>         my $status = $cgi->param("flag-$id");
>+        my @requestees = $cgi->param("requestee-$id");

>+        if ($status eq '?' && !$flag->{type}->{is_requesteeble}) {
>+            my $old_requestee =
>+                $flag->{'requestee'} ? $flag->{'requestee'}->login : '';
>+            my $new_requestee = join('', $cgi->param("requestee-$id"));

Nit: you have defined @requestees above; why don't you use it here instead of
$cgi->param("requestee-$id")? The code would be more consistent.



>Index: template/en/default/global/user-error.html.tmpl

>+  [% ELSIF error == "flag_not_multiplicable" %]
>+    You can't ask more than one person at a time for
>+    <em>[% type.name FILTER html %]</em>.

Nit: it would be fine if a title were defined for this error. "Error" is not
very explicit. ;)


Both are nits; feel free to update your patch with these two nits fixed and
carry forward my r+ if you want. r=LpSolit

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: fixes one of two nits

This patch fixes the nit about inconsistent use of @requestees.  I don't add a
title for the error message, however, because I can't come up with one better
than a generic "Error".  Carrying review forward per reviewer comment.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.52; previous revision: 1.51
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.22; previous revision: 1.21
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v
 <--  code-error.html.tmpl
new revision: 1.56; previous revision: 1.55
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.121; previous revision: 1.120
done