330892 - Turn on insider/private comment/private attachments at BMO

Status: RESOLVED FIXED

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

Description

[Daniel Veditz [:dveditz]]

The security group needs the private attachment/private comment feature of insidergroup turned on at BMO. When we announce security vulnerabilities we're under a lot of cultural pressure to open the bug, yet revealing working exploit code even with a fixed version available can be problematic.

For some contributors there are potential legal liability issues in their home countries (France, Italy) for having the stuff public, even if it was a non-malicious demonstration responsibly reported. We sometimes "solve" this problem by creating a dummy public bug to describe the issue, but that doesn't carry the patch or any important design conversations.

Sometimes a comment references exploit information and it would be much better to be able to hide the comment than to have to hide the entire bug, which is what we do now.

I understand there can only be one insider group due to bug 299895, but until that can be fixed I lobby that "security" should be the one.

Comment 1

[Dave Miller [:justdave]]

This is going to be a policy decision at this point.  Send it back to me if we decide to implement.

I personally cringe at the thought of enabling this, but if we really need it I'll do it.  My basic objection (besides the implementation being somewhat sketchy?) is that it's too easy to use, and with the current conversations about trying to keep things open, this seems exactly counter to that.  If we turn this on, we're going to have to be really vigilant about what we let it get used for, and publicly humiliate people that abuse it. :)

If we implement this, I am going to request that we wait until we've upgraded Bugzilla to 2.22 before turning it on.  The feature is relatively new in Bugzilla, and it's had more time to bake in 2.22.

Comment 2

[Gervase Markham [:gerv]]

When this feature was implemented, I predicted the day would come that someone would want it on b.m.o. The question is: are we using it to enable greater transparency (e.g. to open bugs that would otherwise be completely closed) or less transparency?

justdave is right - if we do this, we need to a) work out exactly how it would work in practice, and make sure the implementation supports that method of working, and b) document some rules for hiding and unhiding comments. Like secure bugs, insider-comments should not remain insider for ever.

For example, one rule might be that just as the reporter of a security bug can make it public if they choose, they should also be able to un-insider any comment. However, I'm not sure if reporters can even see insider-comments at the moment - which is an implementation point we'd need to check.

Gerv

Comment 3

[Dave Miller [:justdave]]

(In reply to comment #2)
> However, I'm not sure if reporters can even see insider-comments at
> the moment - which is an implementation point we'd need to check.

The feature was originally created with the express purpose of tech-support type people being able to put comments on the bug that the reporter couldn't see.  So, no, the reporter wouldn't be able to see them.

Comment 4

[Gervase Markham [:gerv]]

OK. To make this work we would need to:

- Upgrade b.m.o. to 2.22 or the tip

- Turn on the insidergroup feature, with value "security".

- Make a b.m.o.-specific patch so that reporters and CC list members of a particular bug are counted as part of the insidergroup for that bug.

(That's the mechanism)

- Write some rules about how we use this feature responsibly in line with our commitment to openness.

(That's the policy)

I will look into making such a patch. 

Dave: do we have an ETA for upgrading b.m.o., now that 2.22 is out?

Gerv

Comment 5

[Gervase Markham [:gerv]]

> - Make a b.m.o.-specific patch so that reporters and CC list members of a
> particular bug are counted as part of the insidergroup for that bug.

Of course that won't work, because anyone can add themselves to the CC list :-) But we can make it so the reporter is counted as part of the insidergroup. Unfortunately, there's no way to do this centrally - UserInGroup() doesn't know what the bug ID is and so doesn't know if the user is a reporter. So we have to do it at each callsite.

Therefore, I've made a patch which allows the reporter to at least _view_ the insider stuff, even if they can't change the insider status of it.

Gerv

Comment 6

[Gervase Markham [:gerv]]

Grr. The Commit button is too close to the knob.

Gerv

Comment 7

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Here's the patch.

Gerv

Comment 8

[Gervase Markham [:gerv]]

Moving to Bugzilla product so I can use the review flags.

Gerv

Comment 9

[Gervase Markham [:gerv]]

Comment on attachment 221594 [details] [diff] [review]
Patch v.1

Asking justdave for review, for eventual placing on b.m.o.

Gerv

Comment 10

[Frédéric Buclin]

Comment on attachment 221594 [details] [diff] [review]
Patch v.1

>Index: template/en/default/bug/comments.html.tmpl

>-[% isinsider = Param("insidergroup") && UserInGroup(Param("insidergroup")) %]
>+[% isinsider = Param("insidergroup") && 
>+               (UserInGroup(Param("insidergroup")) ||
>+                bug.user.isreporter) %]

Setting isinsider to 1 for the reporter will show the 'Private' checkboxes. If the reporter plays with them, process_bug.cgi will complain that he is not allowed to change this attribute, which is confusing.



>Index: template/en/default/bug/edit.html.tmpl

>   [% PROCESS attachment/list.html.tmpl
>              attachments = bug.attachments
>-             bugid       = bug.bug_id
>+             bug         = bug
>              num_attachment_flag_types = bug.num_attachment_flag_types
>              show_attachment_flags = bug.show_attachment_flags
>    %]

This doesn't make sense to pass the bug object and still pass bug.foo at the same time.



>Index: attachment.cgi

>     if ($isprivate && Param("insidergroup")) {
>-        UserInGroup(Param("insidergroup"))
>+        UserInGroup(Param("insidergroup")) 
>+          || $bug->user->{'isreporter'}
>           || ThrowUserError("auth_failure", {action => "access",
>                                              object => "attachment"});
>     }

This change will let the reporter to turn off the private attribute of attachments, including other people attachments if the reporter is in the editbugs group. That's too critical to accept this change.

Comment 11

[Gervase Markham [:gerv]]

(In reply to comment #10)
> Setting isinsider to 1 for the reporter will show the 'Private' checkboxes. If
> the reporter plays with them, process_bug.cgi will complain that he is not
> allowed to change this attribute, which is confusing.

True; I was attempting to keep the patch small. The fewer changes b.m.o. has from a stock install, the better. (You are aware that this is a b.m.o.-specific patch, aren't you?)

> This doesn't make sense to pass the bug object and still pass bug.foo at the
> same time.

Indeed; but see above. I could rewrite large chunks of the template, but there's no need for a local hack. There's no harm in what it does now.

> This change will let the reporter to turn off the private attribute of
> attachments, including other people attachments if the reporter is in the
> editbugs group. That's too critical to accept this change.

That's by design. If the reporter can view this information, they can publicise it by other methods anyway. This is similar to our current rule that a reporter can make an entire bug public whenever he wants to.

We could do this differently, I suppose, but it wouldn't add any extra security, only inconvenience for a reporter who wanted to publicise the bug's contents.

Gerv

Comment 12

[Frédéric Buclin]

(In reply to comment #11)
> True; I was attempting to keep the patch small. The fewer changes b.m.o. has
> from a stock install, the better. (You are aware that this is a b.m.o.-specific
> patch, aren't you?)

Confusing the reporter is not a good thing, even on b.m.o.


> Indeed; but see above. I could rewrite large chunks of the template, but
> there's no need for a local hack. There's no harm in what it does now.

An even less intrusive patch would pass "is_reporter => bug.user.isreporter" to the template, and you could then use is_reporter in the template.


> That's by design. If the reporter can view this information, they can publicise
> it by other methods anyway. This is similar to our current rule that a reporter
> can make an entire bug public whenever he wants to.

Wrong! A reporter cannot make a bug public. Checkboxes are disabled.


If you want this patch for b.m.o only, please do not move this bug in the Bugzilla product. Much better would be to add the review flag to its previous product.

Comment 13

[Gervase Markham [:gerv]]

(In reply to comment #12)
> Wrong! A reporter cannot make a bug public. Checkboxes are disabled.

http://www.mozilla.org/projects/security/security-bugs-policy.html says:
"The person reporting a bug... has the power to open the bug report for public scrutiny."

Are you saying that this is, in fact, incorrect?

> If you want this patch for b.m.o only, please do not move this bug in the
> Bugzilla product. Much better would be to add the review flag to its previous
> product.

OK; done.

Gerv

Comment 14

[Frédéric Buclin]

(In reply to comment #13)
> "The person reporting a bug... has the power to open the bug report for public
> scrutiny."
> 
> Are you saying that this is, in fact, incorrect?

It's incorrect, yes. Unless the reporter has editbugs privs, in which case he can move the bug to another product (which will remove group restrictions) and then move the bug back to the initial product (now with no group restriction).

Comment 15

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Try this for size; it should address all your comments.

Gerv

Comment 16

[Frédéric Buclin]

Comment on attachment 223176 [details] [diff] [review]
Patch v.2

>Index: attachment.cgi
>@@ -164,12 +164,14 @@ sub validateID
>         unless $bugid;
> 
>     # Make sure the user is authorized to access this attachment's bug.
> 
>     ValidateBugID($bugid);
>+    my $bug = new Bugzilla::Bug($bugid, Bugzilla->user->id);
>     if ($isprivate && Param("insidergroup")) {
>-        UserInGroup(Param("insidergroup"))
>+        UserInGroup(Param("insidergroup")) 
>+          || $bug->user->{'isreporter'}
>           || ThrowUserError("auth_failure", {action => "access",
>                                              object => "attachment"});

The patch applies cleanly on tip, but not on the 2.22 branch. As b.m.o is going to upgrade to 2.22, you should fix attachment.cgi accordingly.

Moreover, this patch will let reporters to make private attachments public. Is that really the desired effect?

Else your patch looks good.

Comment 17

[timeless]

why are we letting reporters see insider comments at all? the whole idea of insider is to allow the security group to chatter on bugs without others seeing them. if we wanted someone who was the reporter to see the insider comment, we should let that person into the group.

Comment 18

[Daniel Veditz [:dveditz]]

> This is similar to our current rule that a reporter
> can make an entire bug public whenever he wants to.

That is our security-bug policy, implemented manually since Bugzilla doesn't support it. For generic groups I agree with the Bugzilla implementation -- non-members don't necessarily have the knowledge or wisdom to know when the group flag is appropriate, especially since each group could have different (and unpublished) policies. We can continue to implement the security-bug policy on a manual basis.

Note that the reporter can be blocked from viewing the bug (in bugzilla, not by security-group policy) and in those cases could only publicize the original issue.

I disagree with your design premise that a reporter should be able even to see insidergroup comments or attachments, let alone remove the insidergroup status from them. If by "reporter" you mean "person who added the comment or attachment in question" rather than the one-and-only original bug reporter then I could be swayed to agree with you, but it's not worth much extra work to me.

An enhancement request might be for an "allow reporter to see this" checkbox next to the insidergroup checkbox, but for those cases we can just as easily make the whole bug security-confidential if we need to. The main use here is opening up most of previously closed bugs, or temporarily hiding comments amongst developers about security aspects of a bug not otherwise hidden.

(In reply to comment #14)
> he can move the bug to another product (which will remove group
> restrictions) and then move the bug back to the initial product
> (now with no group restriction).

Horrible, but that's a bug, yes? Bugzilla should not allow anyone to move a bug if it will remove a group set -- either the move should be blocked or the group should remain set.

Comment 19

[Frédéric Buclin]

(In reply to comment #18)
> Horrible, but that's a bug, yes? Bugzilla should not allow anyone to move a bug
> if it will remove a group set -- either the move should be blocked or the group
> should remain set.

Makes sense, partially. The move should be allowed only if the user is in the group in question, meaning he could remove the group restriction even without moving the bug elsewhere (2 steps in 1). What about the assignee if he isn't is this group? Should he be able to move the bug anyway? Anyway, that's another bug.

About gerv's patch, if you don't want to let reporters see private comments and attachments, then there is no need to hack b.m.o. All you have to do is to set the 'insidergroup' parameter to 'security'; that's all. I can only review these patches from a technical point of view. The policy is your decision.

Comment 20

[Frédéric Buclin]

(In reply to comment #19)
> Makes sense, partially. The move should be allowed only if the user is in the
> group in question, meaning he could remove the group restriction even without
> moving the bug elsewhere (2 steps in 1). What about the assignee if he isn't is
> this group? Should he be able to move the bug anyway? Anyway, that's another
> bug.

See bug 90477 about this issue.

Comment 21

[Daniel Veditz [:dveditz]]

(In reply to comment #19)
> The move should be allowed only if the user is in the group in question,
> meaning he could remove the group restriction even without moving the bug
> elsewhere (2 steps in 1).

Convenient, except they almost surely didn't mean to remove the group and just didn't know the new product didn't support it (which may be a mistake in the configuration of that bugzilla instance).

> Anyway, that's another bug.

Appears to be bug 303183

Comment 22

[Gervase Markham [:gerv]]

Dan Veditz said:
> I disagree with your design premise that a reporter should be able even to see
> insidergroup comments or attachments, let alone remove the insidergroup status
> from them.

Er, OK; I was under the impression that this was the change that needed to be made before we could start using insidergroup!

So, in the end, it appears that the way Bugzilla works now may be sufficient. So I suggest we set insidergroup to "security" and start using the mechanism; if we find some policy issues in the implementation, we can provide a patch at a later date.

If no-one objects in a few days, I'll reassign this bug to Dave to do it.

We also need a policy about what should be restricted. How about the following:

"The insidergroup flag should be set on comments and attachments which reveal information about security-confidential bugs other than the current one, or security group activities which are not public. 

The person who opens a security bug is responsible for scanning it to see if there are any remaining unrestricted comments which should be restricted.

To keep the information thus restricted to a minimum, security group members are asked to segregate this type of information into separate comments from other relevant things they have to say."

Gerv

Comment 23

[Gervase Markham [:gerv]]

Dave: please turn on insidergroup for b.m.o. with value "security". Alternatively, I'm happy to do it if you are busy.

[Sorry for not doing this before now; it slipped through the cracks.]

Gerv

Comment 24

[Frédéric Buclin]

(In reply to comment #23)
> Dave: please turn on insidergroup for b.m.o. with value "security".
> Alternatively, I'm happy to do it if you are busy.

Before doing this, please read bug 346086.

Comment 25

[Daniel Veditz [:dveditz]]

> Before doing this, please read bug 346086.

For BMO use that doesn't bother me in the slightest. If that information needed to be private we would leave the entire bug in the hidden security group because it would no doubt be revealed in various comments anyway.

Comment 26

[Dave Miller [:justdave]]

OK, discussing this with dveditz on IRC the other day, we've decided we need a two-tier permissions arrangement for insidergroup for it to be useful on bmo.  The stated purpose of enabling it is very much in line with the project goals of being open but not to release exploits to the wild, however the set of people that need to be able to continue seeing items which have been marked private is much larger than the set of people who should be able to mark things as private.  This will need to be a local hack for bmo, as we're upgrading to 3.0 shortly, and 3.0 is already past the feature freeze, so it won't be upstream until 3.2, if it goes upstream.

Comment 27

[Gervase Markham [:gerv]]

> however the set of people that need to be able to continue seeing items which 
> have been marked private is much larger than the set of people who should be 
> able to mark things as private.

Given that we presumably trust both sets of people, can we not just do this as a social control for the time being? That is to say, tell everyone in the first group "Don't mark stuff as private unless we tell you you can", and then give explicit permission to the second group.

Gerv

Comment 28

[Dave Miller [:justdave]]

Yeah, that's probably what we'll do for now.  The problem is there's no notification by email when someone toggles that flag on a comment, so it's hard to catch someone if they do abuse it.  We're still going to need to hack it so that the reporter can still see them.

Comment 29

[Gervase Markham [:gerv]]

(In reply to comment #28)
> Yeah, that's probably what we'll do for now.  The problem is there's no
> notification by email when someone toggles that flag on a comment, so it's hard
> to catch someone if they do abuse it. 

Do we really need that? We'll find out if something's been bogusly hidden when people complain they can't see it. Remember, it's obvious something is missing because comment numbers are non-contiguous.

> We're still going to need to hack it so
> that the reporter can still see them.

That should be a two-line fix. Is this bug an appropriate place for a patch?

Gerv

Comment 30

[Dave Miller [:justdave]]

(In reply to comment #29)
> > We're still going to need to hack it so
> > that the reporter can still see them.
> 
> That should be a two-line fix. Is this bug an appropriate place for a patch?

sure.

Comment 31

[Daniel Veditz [:dveditz]]

(In reply to comment #26)
> OK, discussing this with dveditz on IRC the other day, we've decided we need a
> two-tier permissions arrangement for insidergroup for it to be useful on bmo.

I don't actually agree with that statement, I acquiesced if that's what it takes to get the feature turned on even a little bit. I trust the folks on the security group not to abuse this any more than we abuse hiding entire bugs, and we're already trusting them with knowledge of horrible exploits.

It is ironic that fear of people hiding too much has us stuck in the status quo of having far more hidden than we need.

I think you're just signing yourself up for unnecessary work with the two-tier scheme, but as I said I'll take it if that's the only way to more of the old security bugs mostly opened up.

We can police attachment abuse by searching for "attachment is private" just as we periodically sift through old "group is security" bugs. A similar ability to search for "Comment is private" seems both sufficient to detect abuse as well simpler to implement than a two-tier insidergroup hack.

Comment 32

[Gervase Markham [:gerv]]

I say we should turn this on as soon as there are no known ways that _too_much_ information could leak (e.g. a security hole). After all, presumably every bug is checked before it is opened, so turning this on isn't going to lead to accidental revelations that way. The only risk is accidental disclosure via a bug in Bugzilla.

Aside from bug 346086, which dveditz is not concerned about, are there any other known security bugs in insidergroup which would prevent us from turning it on?

Gerv

Comment 33

[Reed Loden [:reed]]

The insidergroup feature is now active on bmo. If you wish for a change in its function (such as allowing reporters to see insidergroup stuff), please file a new bug.