1317965 - Flag permission checks broken by bug 1257662 allowing unauthorized flag modification

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Bug Creation/Editing, defect)

Description

[Jose María Acuña]

Attachment: sec-bounty.png

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.99 Safari/537.36

Steps to reproduce:

I can change the sec-bounty flag of any bug with a fake mail account, for example: [e-mail]

1 - Right click on the web page
2 - To inspect
3 - Change the value of the select
4 - Click "Save Changes" button

Test: https://bugzilla.mozilla.org/show_bug.cgi?id=855373




Actual results:

I can change the sec-bounty flag of any bug


Expected results:

I think certain flags should not be public

Comment 1

[:Gijs (out until Jan 5th; he/him)]

Not sure if this is bugzilla or bmo-specific. Dylan/dkl, can you take a look and/or move this to the right place if this isn't it?

Comment 2

[Frederik Braun [:freddy]]

(In reply to Jose María Acuña from comment #0)
> I think certain flags should not be public

Oh yes. I think this flag ought to be restricted to the bounty team.

Comment 3

[Frederik Braun [:freddy]]

Also, Jose, can you please continue your security testing on our test version of Bugzilla and not on production? https://bugzilla.allizom.org

Comment 4

[Jose María Acuña]

(In reply to Frederik Braun [:freddyb] from comment #3)
> Además, José, ¿podría continuar con las pruebas de seguridad en nuestra
> versión de prueba de Bugzilla y no en la producción?
> https://bugzilla.allizom.org

Sorry, the request:

Forbidden: You don't have permission to access /error/noindex.html on this server.

Comment 5

[Jose María Acuña]

More information:
You can modify any flag, even those that are disabled

Comment 6

[David Lawrence [:dkl]]

Just verified that a sec-bounty+ can be cleared or set back to ? by non-permitted user. But not set to - which generated an error after submit.

Comment 7

[:Gijs (out until Jan 5th; he/him)]

(In reply to Jose María Acuña from comment #4)
> (In reply to Frederik Braun [:freddyb] from comment #3)
> > Además, José, ¿podría continuar con las pruebas de seguridad en nuestra
> > versión de prueba de Bugzilla y no en la producción?
> > https://bugzilla.allizom.org
> 
> Sorry, the request:
> 
> Forbidden: You don't have permission to access /error/noindex.html on this
> server.

I think Freddy meant https://landfill.bugzilla.org/ , which has several different bugzilla installs you can test with.

Comment 8

[Jose María Acuña]

(In reply to :Gijs Kruitbosch (PTO Nov. 17-20 inclusive) from comment #7)
> (In reply to Jose María Acuña from comment #4)
> > (In reply to Frederik Braun [:freddyb] from comment #3)
> > > Además, José, ¿podría continuar con las pruebas de seguridad en nuestra
> > > versión de prueba de Bugzilla y no en la producción?
> > > https://bugzilla.allizom.org
> > 
> > Sorry, the request:
> > 
> > Forbidden: You don't have permission to access /error/noindex.html on this
> > server.
> 
> I think Freddy meant https://landfill.bugzilla.org/ , which has several
> different bugzilla installs you can test with.

Thanks Gijs!

Comment 9

[Dylan Hardison [:dylan] (he/him)]

(In reply to Jose María Acuña from comment #8)
> (In reply to :Gijs Kruitbosch (PTO Nov. 17-20 inclusive) from comment #7)
> > (In reply to Jose María Acuña from comment #4)
> > > (In reply to Frederik Braun [:freddyb] from comment #3)
> > > > Además, José, ¿podría continuar con las pruebas de seguridad en nuestra
> > > > versión de prueba de Bugzilla y no en la producción?
> > > > https://bugzilla.allizom.org
> > > 
> > > Sorry, the request:
> > > 
> > > Forbidden: You don't have permission to access /error/noindex.html on this
> > > server.
> > 
> > I think Freddy meant https://landfill.bugzilla.org/ , which has several
> > different bugzilla installs you can test with.
> 
> Thanks Gijs!

That is also not correct. BMO is quite diverged from upstream at the moment.
the correct site is bugzilla-dev.allizom.org

Comment 10

[Dylan Hardison [:dylan] (he/him)]

The problem is that https://github.com/mozilla-bteam/bmo/blob/master/Bugzilla/Flag.pm#L776-L781 is wrong.

If it isn't immediately clear: $setter->can_unset_flag( $self->type, $self->{_old_status} ) is at the same level
as any of the conditions, so that means being able to unset a flag is equivalent to being able to set it.
However some other part of the code prevents us from setting -, which is no consolation.

This was introduced by bug 1257662


Debugging/proof:

        use Alive 'alive';
        alive(
               { or => [ ( $status eq $self->{_old_status} )
                             , ( $status eq 'X' && $setter->id == Bugzilla->user->id )
                             , ( ( $status eq 'X' || $status eq '?' )
                                  && $setter->can_request_flag( $self->type ) )
                             , $setter->can_unset_flag( $self->type, $self->{_old_status} )
                             , $setter->can_set_flag( $self->type ) ], } );

Result:

    or   [
        [0] "",
        [1] "",
        [2] "",
        [3] 1,
        [4] 0
    ]

Comment 11

[Jose María Acuña]

(In reply to Dylan Hardison [:dylan] from comment #9)
> (In reply to Jose María Acuña from comment #8)
> > (In reply to :Gijs Kruitbosch (PTO Nov. 17-20 inclusive) from comment #7)
> > > (In reply to Jose María Acuña from comment #4)
> > > > (In reply to Frederik Braun [:freddyb] from comment #3)
> > > > > Además, José, ¿podría continuar con las pruebas de seguridad en nuestra
> > > > > versión de prueba de Bugzilla y no en la producción?
> > > > > https://bugzilla.allizom.org
> > > > 
> > > > Sorry, the request:
> > > > 
> > > > Forbidden: You don't have permission to access /error/noindex.html on this
> > > > server.
> > > 
> > > I think Freddy meant https://landfill.bugzilla.org/ , which has several
> > > different bugzilla installs you can test with.
> > 
> > Thanks Gijs!
> 
> That is also not correct. BMO is quite diverged from upstream at the moment.
> the correct site is bugzilla-dev.allizom.org

I take note of that, Dylan

Comment 12

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1317965_1.patch

Comment 13

[David Lawrence [:dkl]]

Comment on attachment 8811326 [details] [diff] [review]
1317965_1.patch

Review of attachment 8811326 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Flag.pm
@@ +777,5 @@
>              || ($status eq 'X' && $setter->id == Bugzilla->user->id)
>              || (($status eq 'X' || $status eq '?')
>                  && $setter->can_request_flag($self->type))
> +            || ($status eq 'X'
> +                && $setter->can_unset_flag($self->type, $self->{_old_status}))

Looking at this closely, it now seems the line before this would also be a problem.

|| (($status eq 'X' || $status eq '?') && $setter->can_request_flag($self->type))

Someone with request rights but not grant rights could clear the 'set' flag which we also do not want to happen.

So the can_unset_flag should probably come before the can_request_flag check.

# Make sure the user is authorized to modify flags, see bug 180879:
# - The flag exists and is unchanged.              
# - The flag setter can unset flag.
# - users in the grant_group can clear set flags (flags set to "+" or "-")               
# - Users in the request_group can set/clear pending requests
# - Users in the grant_group can set/clear set flags (flags set to "+" and "-")

my $old_status = $self->{_old_status};
unless (($new_status eq $old_status)          
        || ($status eq 'X' && $setter->id == Bugzilla->user->id)
        || (($old_status eq '+' || $old_status eq '-') && $setter->can set_flag($self->type))
        || (($status eq 'X' || $status eq '?') && $setter->can_request_flag($self->type))                                                                                                                                                
        || $setter->can_set_flag($self->type))
      {           
          ThrowUserError('flag_update_denied',
                         { name       => $self->type->name,
                           status     => $status,
                           old_status => $self->{_old_status} });
      }

Comment 14

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1317965_2.patch

Comment 15

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8811345 [details] [diff] [review]
1317965_2.patch

nevermind, one second

Comment 16

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1317965_3.patch

I think this is much easier to verify. 

build a table of old_status:new_status -> permission,
where permission is a list of:

- any (anyone can do this)
- grant_group ($setter->can_set_flag($type)
- request_group ($setter->can_request_flag($type)
- is_setter ($setter->id == Bugzilla->user->id)

Comment 17

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1317965_4.patch

minor refactoring of v3 patch (+ fix first transition)

Comment 18

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1317965_5.patch

allow anyone that can request ? to clear ?

Comment 19

[Dylan Hardison [:dylan] (he/him)]

Found two cases of this for the bounty flag, https://bugzilla.mozilla.org/show_bug.cgi?id=1313916 and https://bugzilla.mozilla.org/show_bug.cgi?id=855373

Condition existed since bug 1257662 and has (probably) avoided abuse. There are a few dozen
possible-hits that dkl is currently investigating.

No evidence of this for approval or review flags (and we'd have plenty; we have a total history of all flag changes
which I will be checking next)

Comment 20

[David Lawrence [:dkl]]

Comment on attachment 8811454 [details] [diff] [review]
1317965_4.patch

Review of attachment 8811454 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice. Fixes on commit. r=dkl

::: Bugzilla/Flag.pm
@@ +50,4 @@
>  use Bugzilla::Mailer;
>  use Bugzilla::Constants;
>  use Bugzilla::Field;
> +use List::MoreUtils qw(any);

remove

@@ +757,5 @@
>      return $requestee;
>  }
>  
> +
> +

remove excess whitespace

::: Bugzilla/User.pm
@@ +1586,5 @@
> +sub can_change_flag {
> +    my ($self, $flag_type, $old_status, $new_status) = @_;
> +
> +    # "old_status:new_status" => [OR conditions
> +    state $flag_transitions = {

need "use feature 'state';" for versions older than 5.16.x.

@@ +1591,5 @@
> +        'X:-' => ['grant_group'],
> +        'X:+' => ['grant_group'],
> +        'X:?' => ['request_group'],
> +
> +        '?:X' => ['grant_group', 'is_setter'],

['request_group', 'is_setter']

Comment 21

[Dylan Hardison [:dylan] (he/him)]

(In reply to David Lawrence [:dkl] from comment #20)
> Comment on attachment 8811454 [details] [diff] [review]
> 1317965_4.patch
> 
> Review of attachment 8811454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very nice. Fixes on commit. r=dkl
> 
> ::: Bugzilla/Flag.pm
> @@ +50,4 @@
> >  use Bugzilla::Mailer;
> >  use Bugzilla::Constants;
> >  use Bugzilla::Field;
> > +use List::MoreUtils qw(any);
> 
> remove
> 
> @@ +757,5 @@
> >      return $requestee;
> >  }
> >  
> > +
> > +
> 
> remove excess whitespace
> 
> ::: Bugzilla/User.pm
> @@ +1586,5 @@
> > +sub can_change_flag {
> > +    my ($self, $flag_type, $old_status, $new_status) = @_;
> > +
> > +    # "old_status:new_status" => [OR conditions
> > +    state $flag_transitions = {
> 
> need "use feature 'state';" for versions older than 5.16.x.

Not needed, this is implied by the use 5.10.1 at the top

> @@ +1591,5 @@
> > +        'X:-' => ['grant_group'],
> > +        'X:+' => ['grant_group'],
> > +        'X:?' => ['request_group'],
> > +
> > +        '?:X' => ['grant_group', 'is_setter'],
> 
> ['request_group', 'is_setter']

I guess that was from the earlier patch.

Comment 22

[Dylan Hardison [:dylan] (he/him)]

To [e-mail]:mozilla-bteam/bmo.git
   648ddd3..10bf6d4  master -> master

Comment 23

[David Lawrence [:dkl]]

This is live now.

Comment 25

[Dylan Hardison [:dylan] (he/him)]

We've audited all flags changed since February and aside from the reporter testing this there
are no indications it was abused.

Someone in the bounty group should fix the invalid bounty flags on:

https://bugzilla.mozilla.org/show_bug.cgi?id=1313916 and https://bugzilla.mozilla.org/show_bug.cgi?id=855373

dveditz?

Comment 29

[Daniel Veditz [:dveditz] back January 5, 2026]

(In reply to Dylan Hardison [:dylan] from comment #25)
> Someone in the bounty group should fix the invalid bounty flags on:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1313916 and
> https://bugzilla.mozilla.org/show_bug.cgi?id=855373

Ah, caught the first one when this was fixed based on the reporter's activity log, missed the fake test account. Looks like Al got that one.

Comment 30

[Daniel Veditz [:dveditz] back January 5, 2026]

Minor damage, does not qualify for a bug bounty

Comment 31

[Jose María Acuña]

(In reply to Daniel Veditz [:dveditz] from comment #30)
> Minor damage, does not qualify for a bug bounty

I'm not sure about that.
If I had changed the flags of the resolved and fixed bugs, I would have manipulated the reports and statistics of the data and that is not minor damage.

Comment 32

[Al Billings [:abillings - ex-MoCo]]

(In reply to Jose María Acuña from comment #31)

> If I had changed the flags of the resolved and fixed bugs, I would have
> manipulated the reports and statistics of the data and that is not minor
> damage.

Yes, it is, because I'm the one that makes those reports and I use data generated by our accounting department, not Bugzilla, when I do so. I also hand process bounty payments when we decide to award them and we add the attachments for bounties with details. Manipulating the flags is annoying but wouldn't destroy our processes.  

This is not a "moderate" or higher rated security issue and, as a result, does not qualify for a bounty per our published guidelines.

Comment 33

[Jose María Acuña]

No problem ;)
Regards.