507389 - [SECURITY] Users can see all products when editing bugs

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[Sergej Pupykin]

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

Comment 1

[Frédéric Buclin]

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.

Comment 2

[Frédéric Buclin]

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.

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

We need to pass the list of products to use.

Comment 4

[Frédéric Buclin]

The problem related to charts is different, as charts do not see categories as products.

Comment 5

[Max Kanat-Alexander]

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.

Comment 6

[Frédéric Buclin]

(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).

Comment 7

[Max Kanat-Alexander]

Um, I'd be okay with having the extra product at the *beginning*, I think.

Comment 8

[Frédéric Buclin]

(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.

Comment 9

[Frédéric Buclin]

Attachment: 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.

Comment 10

[Max Kanat-Alexander]

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 %]"

Comment 11

[Max Kanat-Alexander]

We should release ASAP to get out a fix for this issue.

Comment 12

[Frédéric Buclin]

Attachment: patch, v2.1

I just realized my patch doesn't pass 008filter.t, because unshift() is not in the whitelist. Fixed.

Comment 13

[Frédéric Buclin]

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

Comment 14

[Frédéric Buclin]

Attachment: 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.

Comment 15

[Frédéric Buclin]

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

Comment 16

[Max Kanat-Alexander]

Security advisory sent, unlocking bug.