Closed
Bug 234855
Opened 20 years ago
Closed 20 years ago
product field on edit-multiple includes products the user doesn't have access to
Categories
(Bugzilla :: Creating/Changing Bugs, defect, P1)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.16
People
(Reporter: laran, Assigned: bugreport)
References
()
Details
(Whiteboard: [fixed in 2.16.6] [fixed in 2.18rc1])
Attachments
(2 files, 2 obsolete files)
1.37 KB,
patch
|
bugreport
:
review+
justdave
:
review+
|
Details | Diff | Splinter Review |
867 bytes,
patch
|
justdave
:
review+
myk
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113 all products are configured with group restrictions (one group per product, each user is in exactly 1 group). current user searched for some bugs. the list came up, I clicked "change several bugs at once". when the page loaded, all of the products (including others to which the current user did not have access) showed up in the dropdown. Only those groups to which the user belongs should show up. In this case that would be one group. Reproducible: Always Steps to Reproduce: 1. 2. 3.
Updated•20 years ago
|
Group: webtools-security
OS: Windows 2000 → All
Hardware: PC → All
Reporter | ||
Updated•20 years ago
|
Version: unspecified → 2.17.6
Assignee | ||
Comment 1•20 years ago
|
||
Yep
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.18
Comment 2•20 years ago
|
||
Does this affect 2.16 or did this get accidently broken by the groups rewrite?
Comment 3•20 years ago
|
||
Not only does the product menu on edit-multiple include products that are mandatory to groups you don't have access to, but it also include inactive products (with disallownew set on them). In theory you shouldn't be able to move a bug to a product that's inactive either.
Assignee | ||
Comment 4•20 years ago
|
||
We should also do an audit of all the places where the fool ::legal_whatever variables are used.
Assignee: myk → bugreport
Status: NEW → ASSIGNED
Assignee | ||
Updated•20 years ago
|
Attachment #142339 -
Flags: review?(justdave)
Comment 5•20 years ago
|
||
Just confirmed this on the 2.16 branch. It's a problem there as well. Oof.
Whiteboard: [wanted for 2.16.5] [wanted for 2.17.7]
Assignee | ||
Comment 6•20 years ago
|
||
hmm.... should we use EnterableProducts??
Comment 7•20 years ago
|
||
Yeah, Enterable sounds good. In theory, moving a bug to a different product implies entry privs to put it there.
Comment 8•20 years ago
|
||
Comment on attachment 142339 [details] [diff] [review] Bleah! - the patch This doesn't work. All the products are still returned when testing with a user which is a member of *no* groups at all and product entry is restricted. I think this is a bug in GetSelectableProducts... Additional comments: none of the variables here are in StudlyCaps, and SelectableProduct is a bit confusing.
Attachment #142339 -
Flags: review?(justdave) → review-
Comment 9•20 years ago
|
||
It seems this bit of GetSelectableProducts is odd: if (defined Bugzilla->user && %{Bugzilla->user->groups}) { $query .= "AND group_id NOT IN(" . join(',', values(%{Bugzilla->user->groups})) . ") "; } can somebody tell me why we're checking if the group_id of the product is *not* IN the user's list? Shouldn't it be *IN*?
Comment 10•20 years ago
|
||
Comment 11•20 years ago
|
||
So I added a GetEnterableProducts that nobody wants to look at, and use it in buglist.cgi. Last patch had a chunk that wasn't relevant.
Attachment #142791 -
Attachment is obsolete: true
Assignee | ||
Comment 12•20 years ago
|
||
Response to comment 9: That's the way we make sure that something is restricted to no group of which the user is not a member. We say.... SELECT thing FROM thing_table LEFT JOIN thing_group_map ON thing_table.id = thing_group_map.thing_id AND thing_group_map.group_id NOT IN(your_grouplist) WHERE thing_group_map.thing_id IS NULL; That says that we must find no (NULL) thing_group_map entry that your_grouplist does not cover. The original patch was finding selectable products rather than enterable products. It would offer you the same list as query.cgi would, closing the security hole, but leaving you with a false expectation that you could actually use some of those options. The patch (kiko v1: sigh) looks reasonable... I'll give it a better read when I get a chance.
Comment 13•20 years ago
|
||
We can use this lame-o function in duplicates.cgi too if the same issue exists there.
Updated•20 years ago
|
Whiteboard: [wanted for 2.16.5] [wanted for 2.17.7] → [wanted for 2.16.6] [wanted for 2.18rc1]
Assignee | ||
Comment 14•20 years ago
|
||
Comment on attachment 142792 [details] [diff] [review] kiko_v1: sigh. removed cruft. r=joel by inspection Let's get a second r= as well and also make sure that someone else has tested it.
Attachment #142792 -
Flags: review+
Assignee | ||
Updated•20 years ago
|
Attachment #142792 -
Flags: review?(justdave)
Comment 15•20 years ago
|
||
For the record, I did test this, and it did work for me at my local installation.
Comment 16•20 years ago
|
||
(In reply to comment #12) > That's the way we make sure that something is restricted to no group of which > the user is not a member. That's quite possibly the most confusing phrase I've ever read, btw. :-)
Assignee | ||
Comment 17•20 years ago
|
||
(In reply to comment #16) > That's quite possibly the most confusing phrase I've ever read, btw. :-) OK, "You cannot see something if it requires a group of which you are not a member. You can see something if there is no reason why not." :-) Gerv and I have been re-debating the question of shifting from AND groups to OR groups. If we could find a good way to migrate existing sites, this stuff would all become much simpler.
Assignee | ||
Comment 18•20 years ago
|
||
I'm not sure that we want to change this on the 2.16 branch. The whole notion that there are products that are truly invisible to the unauthorized is a new concept in 2.17 If a 2.16 installation were that sensitive, there are many other things that would have to be changed as well.
Comment 19•20 years ago
|
||
(In reply to comment #18) > I'm not sure that we want to change this on the 2.16 branch. We do, although it may or may not need the same logic in the fix. > The whole notion that there are products that are truly invisible to the > unauthorized is a new concept in 2.17 No it isn't. The groups system in 2.16 had this as well. If there was a group name that matched a product name, only people in the group could see the product. > If a 2.16 installation were that sensitive, there are many other things that > would have to be changed as well. Then those would also be security bugs and should be fixed.
Comment 20•20 years ago
|
||
Should I be requesting approval here?
Comment 21•20 years ago
|
||
Comment on attachment 142792 [details] [diff] [review] kiko_v1: sigh. removed cruft. Since this is the only place calling it, why not have GetEnterableProducts return a reference to the array? Then you can just assign the function call to $vars->{'products'} instead of having to have a temporary assignment in between so you can reference it... r= justdave either way. We still need a fix for this on the 2.16 branch as well.
Attachment #142792 -
Flags: review?(justdave)
Updated•20 years ago
|
Whiteboard: [wanted for 2.16.6] [wanted for 2.18rc1] → [wanted for 2.16.6] [ready for 2.18rc1]
Assignee | ||
Comment 22•20 years ago
|
||
Here's the 2.16-specific version
Attachment #142339 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #143589 -
Flags: review?(justdave)
Assignee | ||
Comment 23•20 years ago
|
||
Suggested description: "A user performing a mass-change operation on a list of bugs would be presented with a list of possible products including those products to which that user has no access."
Assignee | ||
Updated•20 years ago
|
Attachment #143589 -
Flags: review?(kiko)
Updated•20 years ago
|
Attachment #143589 -
Flags: review?(myk)
Attachment #143589 -
Flags: review?(kiko)
Attachment #143589 -
Flags: review?(justdave)
Attachment #143589 -
Flags: review+
Updated•20 years ago
|
Flags: blocking2.18+
Flags: blocking2.16.6+
Flags: approval?
Comment 24•20 years ago
|
||
Comment on attachment 143589 [details] [diff] [review] 2.16-specific patch Looks good, works.
Attachment #143589 -
Flags: review?(myk)
Assignee | ||
Updated•20 years ago
|
Whiteboard: [wanted for 2.16.6] [ready for 2.18rc1] → [ready for 2.16.6] [ready for 2.18rc1]
Assignee | ||
Comment 25•20 years ago
|
||
Proposed wording for release notes: The form for mass-editing a list of bugs divulges the list of product names even if some of those names are supposed to remain unknown to the user.
Updated•20 years ago
|
Flags: approval2.16?
Updated•20 years ago
|
Flags: approval2.16? → approval2.16+
Updated•20 years ago
|
Flags: approval? → approval+
Summary: security bug in product field on edit-multiple → product field on edit-multiple includes products the user doesn't have access to
Assignee | ||
Comment 26•20 years ago
|
||
checked in on both branches
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•20 years ago
|
Whiteboard: [ready for 2.16.6] [ready for 2.18rc1] → [fixed in 2.16.6] [fixed in 2.18rc1]
Updated•19 years ago
|
Target Milestone: Bugzilla 2.18 → Bugzilla 2.16
Updated•11 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•