299646 - potential missing entries in bug_group_map related to mandatory groups

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Frédéric Buclin]

Now that bug 298024 is fixed, sanitycheck.cgi finds several bugs on my local
installation which have missing entries in bug_group_map, despite of the fact
that these bugs are in a product which has mandatory groups associated with it.

Here is how to reproduce:

1) In the Edit Group Control page, mark a group as Mandatory/Mandatory => all
bugs in this product have now an entry in bug_group_map related to this group,
as expected.

2) In editgroups.cgi, uncheck the "Use for bugs" checkbox for this group => all
restrictions relative to this group still apply. It's not a problem for me, but
we have to discuss this point below. Note that the Edit Group Control page now
displays this group as "Disabled", see also bug 285424, meaning that you cannot
change settings for this group anymore.

3) Create a new bug in the product above => as the group in question is
"disabled", no entry related to this group is added in bug_group_map, as
expected, meaning that anyone can see this bug, assuming no other group
restrictions apply.

4) Re-enable the group again => as an admin, I thought that all bugs in the
product above would be protected by this group again, meaning that I never
checked if it was true or not. As this check in sanitycheck.cgi was broken until
recently, I never took care of that point. And here is the problem: *ALL* bugs
added to this product while this group was "disabled" have no entry in
bug_group_map, meaning that these bugs are public (assuming no other group
restrictions apply)! Indeed, User::can_see_bug(), called by ValidateBugID(),
only check for entries in bug_group_map, not group_control_map!

As I said in 4), I expected that re-enabling this group would add missing
entries in bug_group_map, which did not happen. :(


Workarounds:

A) Go to the Edit Group Control page and click "Submit" => missing entries in
bug_group_map are added.

*or*

B) Visit bugs reported by sanitycheck.cgi and do nothing else except to click
"Commit" => missing entries in bug_group_map are added (for the visited bugs only).


I discussed this problem with joel last night and we came to the conclusion that
some cleanup may be required, especially about the role of the "Use for bugs"
bit. At least, we should make clear to the admin that re-enabling a group does
not automatically add missing entries in bug_group_map *or* this action should
do it for us, automatically!

I mark this bug as a security bug till we come to a clear conclusion about the
behavior Bugzilla should have relative to these "disabled" groups, in agreement
with joel.

Comment 1

[Joel Peshkin]

Hmm...

This is not really exploitable.  It is a case where an unaware administrator
could leave bugs exposed.  My suggestion is that we roll out both some
protective code and documentation for admins but that we not treat it like an
exploit.

  Probably, we should force the admin to take groups out of group controls
before yanking the use-for-bugs flag and we should warn the admin on the reverse
transition.

Comment 2

[Frédéric Buclin]

Removing the security flag per joel's comment.

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

When reactivating a mandatory group, add missing entries in bug_group_map (for bugs created while the group was inactive).

Comment 4

[Joel Peshkin]

Comment on attachment 206302 [details] [diff] [review]
patch, v1

This looks good.  I don't currently have a good databse to test this, but LpSolit has tested it and is reliable about such things.

Comment 5

[Frédéric Buclin]

Attachment: patch, v1.1

Well, I wasn't so reliable this time... I forgot to lock some tables. I don't know how it went through my testing the first time. Maybe I forgot to redo 'cvs diff' after locking them.

Comment 6

[Frédéric Buclin]

Attachment: patch for 2.20, v1

same patch as for the tip. And yes, this time I fully tested it. :(

Comment 7

[Teemu Mannermaa (:wicked)]

Comment on attachment 206546 [details] [diff] [review]
patch, v1.1

Looks like this fixes the problem and has no visible errors. Also, joel reviewed this visually earlier and only changes are locks.

Comment 8

[Frédéric Buclin]

tip:

Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.63; previous revision: 1.62
done

2.20:

Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.52.4.2; previous revision: 1.52.4.1
done