o2m and m2m validator class have an attribute initialized to a mutable value!

Bug #906449 reported by Christophe Combelles
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Odoo Web Client
Confirmed
Low
OpenERP Publisher's Warranty Team

Bug Description

The code below is dangerous and I've come into a nasty bug because of it. It can even silently add unwanted values in other fields (that's why I've checked the security box).

openerp-web-6.0.3/addons/openerp/validators.py:242 :

class one2many(formencode.validators.FancyValidator):
    if_empty = []

and openerp-web-6.0.3/addons/openerp/validators.py:209 :

class many2many(BaseValidator):
    if_empty = [(6, 0, [])]

If you just get the default empty values in your code and append elements into it, you'll get the appended elements in all the other fields of the same type (m2m or o2m).

The correct fix is to replace :

    if_empty = []

with :

    @property
    def if_empty(self):
        return []

and :

    if_empty = [(6, 0, [])]

with :

    @property
    def if_empty(self):
        return [(6, 0, [])]

(or alternatively you can initialize the value in the __init__ method).

Related branches

visibility: private → public
Revision history for this message
Christophe Combelles (ccomb) wrote :
Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote :

Hi,

Thanks for reporting and providing a patch. I'm confirming this with only "Low" priority because it is not a security vulnerability but rather a bad programming choice that could lead to errors when writing web addons.
When a web addon is installed (which requires an admin), it gets unlimited access to data on the client-side, just like a normal OpenERP addon can do anything with data on the server-side. We'd talk about a security issue if this was possible without having administrative privileges.

Normally we don't handle Low priority bugs via Launchpad anymore for the 6.0 web client (as explained in our bug management policy[1]), but as you provided a patch, I'll assign it to the OpenERP Enterprise (maintenance) team, so they can review your branch and merge it. BTW, you should create a merge proposal for your bugfix branch towards the lp:openobject-client/6.0 branch if you want it to be reviewed. This process is explained in the documentation too[2].

Thanks!

[1] http://bit.ly/openerp-bug-policy (See the FAQ)
[2] http://bit.ly/openerp-contrib-mp (See the guidelines)

Changed in openobject-client-web:
assignee: nobody → OpenERP Publisher's Warranty Team (openerp-opw)
importance: Undecided → Low
milestone: none → 6.0.4
status: New → Confirmed
security vulnerability: yes → no
Revision history for this message
Christophe Combelles (ccomb) wrote :

The problem I found is that this bug is not limited to the web client, but it is propagated to the server code :

The web client creates a "vals" dictionary with form data, serializes it, send it through NetRPC. But all the empty fields (either [] for o2m or [(6,0,[])] for m2m) are the exact same object in memory! So when the server unserializes the dict, the problem appears on the server side as well : the received dict contains several times the same mutable value (same python reference).

Then, any server-side code that will try to append an id in an empty one2many field will modify *all other* empty one2many fields at the same time.

In the best case (what happend to me), the id does not exist in the unwanted field and you get an error which is very difficult to understand.
In the worst case you silently modify an unwanted field, which I find dangerous and can lead to a clear security flaw.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.