504461 - Allow everyone to make bugs security-sensitive retroactively

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, enhancement)

Description

[Jesse Ruderman]

Triagers and developers who notice that crashes are exploitable should be able to mark existing bugs as security-sensitive.  Right now they have to contact a member of the security group, which discourages them from doing so.

This might have helped prevent the recent 0day involving bug 503286.

(But only the bug reporter and members of the security group should be able to *un*hide a bug; see bug 128590.)

Comment 1

[Gervase Markham [:gerv]]

The UI problem here would be that a naive implementation would add bug group controls to every bug, no matter who you were, and that would increase complexity.

Perhaps we want the "this is a security bug" checkbox that you get on enter_bug, with server-side smarts to work out which group is meant.

Gerv

Comment 4

[:glob ✱]

Attachment: 504461_1.patch

adds a checkbox where group controls are normally visible:

[ ] Restrict access to this bug

with a tooltip of:

This bug is security sensitity and should be hidden from the public until it is resolved

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Byron Jones ‹:glob› from comment #4)
> This bug is security sensitity and should be hidden from the public until it
> is resolved

Drive-by nit: Probably should be "security sensitive"?

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 8396938 [details] [diff] [review]
504461_1.patch

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

Would be nicer if could be incorporated into the normal checkbox list as it looks out of place above the group section header. If you would rather
leave it where it is, incorporate some padding below it.

::: extensions/BMO/template/en/default/hook/bug/edit-before_restrict_visibility.html.tmpl
@@ +7,5 @@
> +  #%]
> +
> +[% RETURN IF
> +  bug.in_group(product.default_security_group_obj)
> +  || user.in_group(product.default_security_group) %]

s/product/bug.product/ in both places.

@@ +15,5 @@
> +         value="[% product.default_security_group FILTER none %]"
> +         id="group_[% product.default_security_group FILTER html %]"
> +  >
> +  <label for="group_[% product.default_security_group FILTER html %]"
> +    title="This [% terms.bug %] is security sensitity and should be hidden from the public until it is resolved">

s/sensitity/sensitive/ :)

Comment 7

[:glob ✱]

Attachment: 504461_2.patch

> Would be nicer if could be incorporated into the normal checkbox list as it
> looks out of place above the group section header. If you would rather
> leave it where it is, incorporate some padding below it.

in most cases the current user won't be a member of any security groups, so i've left it in its current location with some padding for users who are members of some security groups, but not the current product's default.

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 8400443 [details] [diff] [review]
504461_2.patch

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

Fixes on commit. r=dkl

::: extensions/BMO/template/en/default/hook/bug/edit-before_restrict_visibility.html.tmpl
@@ +7,5 @@
> +  #%]
> +
> +[% RETURN IF
> +  bug.in_group(bug.product_obj.default_security_group_obj)
> +  || user.in_group(bug.product_obj.default_security_group) %]

Just noticed in testing that all of this fails if the user != reporter of the bug so add that to the above.

@@ +12,5 @@
> +
> +<div class="bz_group_visibility_section">
> +  <input type="checkbox" name="groups"
> +         value="[% bug.product_obj.default_security_group FILTER none %]"
> +         id="group_[% bug.product_obj.default_security_group FILTER html %]"

nit pick for consistency with other group checkboxes: 
        id="group_[% bug.product_obj.default_security_group_obj.id FILTER html %]"

@@ +14,5 @@
> +  <input type="checkbox" name="groups"
> +         value="[% bug.product_obj.default_security_group FILTER none %]"
> +         id="group_[% bug.product_obj.default_security_group FILTER html %]"
> +  >
> +  <label for="group_[% bug.product_obj.default_security_group FILTER html %]"

same here: bug.product_obj.default_security_group_obj.id

Comment 9

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #8)
> > +[% RETURN IF
> > +  bug.in_group(bug.product_obj.default_security_group_obj)
> > +  || user.in_group(bug.product_obj.default_security_group) %]
> 
> Just noticed in testing that all of this fails if the user != reporter of
> the bug so add that to the above.

ah, good catch.

the goal of this bug is for everyone* to be able to mark a bug as security-sensitive, so i'll rework the code a little and attach an updated patch.

* my plan is to allow it for the bug's reporter, or a user with editbugs (not strictly anyone).

Comment 10

[:glob ✱]

Attachment: 504461_3.patch

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 8401114 [details] [diff] [review]
504461_3.patch

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

Also sorry for not finding this before, but now when I am *not* the reporter but I am in 'editbugs', I can successfully add the bug
to the default security group. But if I am *not* in the default security group myself, I am therefore locked out of the bug right after.

Should we also add the user setting the security group to the cc list automatically and if so how do we handle cases where the
bug is not cclist_accessible?

dkl

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 8401114 [details] [diff] [review]
504461_3.patch

r- per comment 11

Comment 13

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #11)
> Should we also add the user setting the security group to the cc list automatically

yes, that sounds like the right thing to do

> and if so how do we handle cases where the bug is not cclist_accessible?

that's very much an edge case; in this instance i think it would be ok for the user to be immediately locked out of the bug.  (only 0.14% of our bugs have cclist_accessible = 0).

Comment 14

[:glob ✱]

 > and if so how do we handle cases where the bug is not cclist_accessible?

ah, cclist_accessible only applies to bugs that are not public, so we don't need to do anything special here.

Comment 15

[:glob ✱]

Attachment: 504461_4.patch

i experimented with a few different ways of addressing the CC issue.

it's currently possible to make a change to a bug and immediately be locked out, so it isn't really a show-stopper when this happens (eg. remove yourself from the cc list of a secure bug), however i agree it is a bit jarring.

i also wanted to somehow honour the user's "automatically add me to the cc list of bugs i change" user preference, or at least let the user mark a bug as secure and not add themselves to the cc list.

what i ended up with is a simple js onchange, which checks the "add me to the cc list" checkbox when "restrict access to this bug" is checked.  this makes the default path less jarring, and also gives users an 'opt-out' of the automatic cc.

Comment 16

[David Lawrence [:dkl]]

Comment on attachment 8419214 [details] [diff] [review]
504461_4.patch

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

r=dkl

Comment 17

[:glob ✱]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   9365f5f..5929fb9  master -> master