Closed Bug 504461 Opened 10 years ago Closed 5 years ago

Allow everyone to make bugs security-sensitive retroactively

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: glob)

References

Details

(Keywords: sec-want, Whiteboard: [sg:want P3])

Attachments

(1 file, 3 obsolete files)

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.)
Whiteboard: [sg:want P3]
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
Duplicate of this bug: 565043
Component: Bugzilla: Other b.m.o Issues → General
Product: mozilla.org → bugzilla.mozilla.org
Duplicate of this bug: 987948
Attached patch 504461_1.patch (obsolete) — Splinter Review
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
Assignee: nobody → glob
Status: NEW → ASSIGNED
Attachment #8396938 - Flags: review?(dkl)
(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 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/ :)
Attachment #8396938 - Flags: review?(dkl) → review-
Attached patch 504461_2.patch (obsolete) — Splinter 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.

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.
Attachment #8396938 - Attachment is obsolete: true
Attachment #8400443 - Flags: review?(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
Attachment #8400443 - Flags: review?(dkl) → review+
(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).
Attached patch 504461_3.patch (obsolete) — Splinter Review
Attachment #8400443 - Attachment is obsolete: true
Attachment #8401114 - Flags: review?(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 on attachment 8401114 [details] [diff] [review]
504461_3.patch

r- per comment 11
Attachment #8401114 - Flags: review?(dkl) → review-
(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).
 > 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.
Attached patch 504461_4.patchSplinter Review
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.
Attachment #8401114 - Attachment is obsolete: true
Attachment #8419214 - Flags: review?(dkl)
Comment on attachment 8419214 [details] [diff] [review]
504461_4.patch

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

r=dkl
Attachment #8419214 - Flags: review?(dkl) → review+
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   9365f5f..5929fb9  master -> master
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.