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)

2.17.6

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)

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.
Group: webtools-security
OS: Windows 2000 → All
Hardware: PC → All
Version: unspecified → 2.17.6
Yep
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.18
Does this affect 2.16 or did this get accidently broken by the groups rewrite?
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.
Attached patch Bleah! - the patch (obsolete) — Splinter Review
We should also do an audit of all the places where the fool ::legal_whatever
variables are used.
Assignee: myk → bugreport
Status: NEW → ASSIGNED
Attachment #142339 - Flags: review?(justdave)
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]
hmm....

should we use EnterableProducts??
Yeah, Enterable sounds good.  In theory, moving a bug to a different product
implies entry privs to put it there.
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-
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*?
Attached patch kiko_v1: ugly but functional (obsolete) — Splinter Review
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
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.
We can use this lame-o function in duplicates.cgi too if the same issue exists
there.
Whiteboard: [wanted for 2.16.5] [wanted for 2.17.7] → [wanted for 2.16.6] [wanted for 2.18rc1]
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+
Attachment #142792 - Flags: review?(justdave)
For the record, I did test this, and it did work for me at my local installation.
(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. :-)
(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.

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.
(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.
Should I be requesting approval here?
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)
Whiteboard: [wanted for 2.16.6] [wanted for 2.18rc1] → [wanted for 2.16.6] [ready for 2.18rc1]
Here's the 2.16-specific version
Attachment #142339 - Attachment is obsolete: true
Attachment #143589 - Flags: review?(justdave)
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."


Attachment #143589 - Flags: review?(kiko)
Attachment #143589 - Flags: review?(myk)
Attachment #143589 - Flags: review?(kiko)
Attachment #143589 - Flags: review?(justdave)
Attachment #143589 - Flags: review+
Flags: blocking2.18+
Flags: blocking2.16.6+
Flags: approval?
Comment on attachment 143589 [details] [diff] [review]
2.16-specific patch

Looks good, works.
Attachment #143589 - Flags: review?(myk)
Whiteboard: [wanted for 2.16.6] [ready for 2.18rc1] → [ready for 2.16.6] [ready for 2.18rc1]
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.
Flags: approval2.16?
Flags: approval2.16? → approval2.16+
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
checked in on both branches
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Whiteboard: [ready for 2.16.6] [ready for 2.18rc1] → [fixed in 2.16.6] [fixed in 2.18rc1]
Clearing the security flag on disclosed bugs
Group: webtools-security
Target Milestone: Bugzilla 2.18 → Bugzilla 2.16
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: