[SECURITY] Users can see all products when editing bugs

RESOLVED FIXED in Bugzilla 3.4

Status

()

P1
critical
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: pupykin.s, Assigned: LpSolit)

Tracking

({regression})

3.3.4
Bugzilla 3.4
regression
Dependency tree / graph
Bug Flags:
approval +
approval3.4 +
blocking3.4.1 +
testcase ?

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1) Gecko/20090718 Shiretoko/3.5.1
Build Identifier: bugzilla-3.4

User can see all products even he has no access to them.

Reproducible: Always

Steps to Reproduce:
1. create 2 products: prod1 and prod2
2. restrict user1 access only to prod1
3. try to edit own bug and see all products in combobox
Actual Results:  
user can see full product list

Expected Results:  
user can see allowed product list

product list page works ok.

problem affects only combo and listboxes on new chart and editbug pages
(Assignee)

Comment 1

10 years ago
I can reproduce on tip and 3.4. I cannot reproduce on 3.2.4 and 3.0.8, so this is a regression.

To be exact: a product must be totally hidden if group settings are Mandatory/Mandatory + the Entry bit is set.
Group: bugzilla-security
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking3.4.1+
Keywords: regression
OS: Linux → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → Bugzilla 3.4
Version: unspecified → 3.4
(Assignee)

Updated

10 years ago
Severity: critical → major
(Assignee)

Comment 2

10 years ago
This is a regression due to bug 371995, which replaced bug.choices.product (which correctly calls $user->get_enterable_products) by Bugzilla::Product->get_all.

Bugzilla 3.3.4 and newer are affected.
Depends on: 371995
Version: 3.4 → 3.3.4
(Assignee)

Comment 3

10 years ago
Created attachment 391655 [details] [diff] [review]
patch, v1

We need to pass the list of products to use.
Assignee: create-and-change → LpSolit
Status: NEW → ASSIGNED
Attachment #391655 - Flags: review?(mkanat)
(Assignee)

Comment 4

10 years ago
The problem related to charts is different, as charts do not see categories as products.
Summary: user can see all products if he edits bug or creates new chart → [SECURITY] Users can see all products when editing bugs

Updated

10 years ago
Attachment #391655 - Flags: review?(mkanat) → review-
Comment on attachment 391655 [details] [diff] [review]
patch, v1

>Index: template/en/default/bug/edit.html.tmpl
>     <tr>
>+       [% prod_list = user.get_enterable_products %]
>+       [% IF NOT user.can_enter_product(bug.product) %]
>+         [% prod_list.push(bug.product_obj) %]
>+         [% prod_list = prod_list.sort('name') %]

  You shouldn't need to sort them by name again--Perl will sort them differently than the database will, so it's best to just let the DB sort them.

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

>+  #   list (optional): The list of legal values, for select fields.

  Hmm. In other patches/customizations, I've called this "override_legal_values", which I think is better, because "list" could be a very common variable name.

Updated

10 years ago
Severity: major → critical
(Assignee)

Comment 6

10 years ago
(In reply to comment #5)
>   You shouldn't need to sort them by name again--Perl will sort them
> differently than the database will, so it's best to just let the DB sort them.

I have to sort the list here because the added product shouldn't be at the end (unless we are fine with this, but this is confusing).
Um, I'd be okay with having the extra product at the *beginning*, I think.
(Assignee)

Comment 8

10 years ago
(In reply to comment #7)
> Um, I'd be okay with having the extra product at the *beginning*, I think.

OK, sure, easy to do. Patch coming.
(Assignee)

Comment 9

10 years ago
Created attachment 391843 [details] [diff] [review]
patch, v2

I now only set the override_... list if the user can edit the product field, else we don't waste time generating the list as the field would be read-only.
Attachment #391655 - Attachment is obsolete: true
Attachment #391843 - Flags: review?(mkanat)

Updated

10 years ago
Attachment #391843 - Flags: review?(mkanat) → review+
Comment on attachment 391843 [details] [diff] [review]
patch, v2

>Index: template/en/default/bug/edit.html.tmpl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v
>retrieving revision 1.160
>diff -3 -p -u -r1.160 edit.html.tmpl
>--- template/en/default/bug/edit.html.tmpl	23 Jul 2009 06:36:10 -0000	1.160
>+++ template/en/default/bug/edit.html.tmpl	31 Jul 2009 10:16:52 -0000
>@@ -375,8 +375,16 @@
>     [%#############%]
>     
>     <tr>
>+       [% IF bug.check_can_change_field('product', 0, 1) %]
>+         [% prod_list = user.get_enterable_products %]
>+         [% IF NOT user.can_enter_product(bug.product) %]
>+           [% prod_list.unshift(bug.product_obj) %]
>+         [% END %]
>+       [% END %]
>+
>        [% INCLUDE bug/field.html.tmpl
>             bug = bug, field = select_fields.product,
>+            override_legal_values = prod_list
>             desc_url = 'describecomponents.cgi', value = bug.product
>             editable = bug.check_can_change_field('product', 0, 1) %]
>     </tr>
>Index: template/en/default/bug/field.html.tmpl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v
>retrieving revision 1.29
>diff -3 -p -u -r1.29 field.html.tmpl
>--- template/en/default/bug/field.html.tmpl	17 Jul 2009 22:40:15 -0000	1.29
>+++ template/en/default/bug/field.html.tmpl	31 Jul 2009 10:16:52 -0000
>@@ -23,6 +23,7 @@
> [%# INTERFACE:
>   #   field: a Bugzilla::Field object
>   #   value: The value of the field for this bug.
>+  #   override_legal_values (optional): The list of legal values, for select fields.
>   #   editable: Whether the field should be displayed as an editable
>   #             <input> or as just the plain text of its value.
>   #   allow_dont_change: display the --do_not_change-- option for select fields.
>@@ -130,7 +131,10 @@
>               [% dontchange FILTER html %]
>             </option>
>           [% END %]
>-          [% FOREACH legal_value = field.legal_values %]
>+          [% IF NOT override_legal_values %]
>+            [% override_legal_values = field.legal_values %]
>+          [% END %]
>+          [% FOREACH legal_value = override_legal_values %]
>             [% SET control_value = legal_value.visibility_value %]
>             [% SET control_field = field.value_field %]
>             <option value="[% legal_value.name FILTER html %]"
We should release ASAP to get out a fix for this issue.
Flags: approval?
Flags: approval3.4?

Updated

10 years ago
Blocks: 507801
(Assignee)

Comment 12

10 years ago
Created attachment 392078 [details] [diff] [review]
patch, v2.1

I just realized my patch doesn't pass 008filter.t, because unshift() is not in the whitelist. Fixed.
Attachment #391843 - Attachment is obsolete: true
Attachment #392078 - Flags: review?(mkanat)

Updated

10 years ago
Attachment #392078 - Flags: review?(mkanat) → review+
(Assignee)

Updated

10 years ago
Flags: approval?
Flags: approval3.4?
Flags: approval3.4+
Flags: approval+
(Assignee)

Comment 13

10 years ago
tip:

Checking in t/008filter.t;
/cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v  <--  008filter.t
new revision: 1.30; previous revision: 1.29
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.161; previous revision: 1.160
done
Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.30; previous revision: 1.29
done


3.4:

Checking in t/008filter.t;
/cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v  <--  008filter.t
new revision: 1.29.2.1; previous revision: 1.29
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.156.2.2; previous revision: 1.156.2.1
done
Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.24.2.4; previous revision: 1.24.2.3
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: testcase?
Flags: approval?
Flags: approval3.4?
Flags: approval3.4+
Flags: approval+

Updated

10 years ago
Flags: approval?
Flags: approval3.4?
Flags: approval3.4+
Flags: approval+
(Assignee)

Comment 14

10 years ago
Created attachment 392086 [details] [diff] [review]
full patch, v3

The previous patch has a bug where values can be overriden by other fields. This is a full patch, with 2.1 being backed out.
Attachment #392078 - Attachment is obsolete: true
Attachment #392086 - Flags: review?(mkanat)

Updated

10 years ago
Attachment #392086 - Flags: review?(mkanat) → review+
(Assignee)

Comment 15

10 years ago
tip:

Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.31; previous revision: 1.30
done

3.4:

Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.24.2.5; previous revision: 1.24.2.4
done
Security advisory sent, unlocking bug.
Group: bugzilla-security
You need to log in before you can comment on or make changes to this bug.