Odoo Server (MOVED TO GITHUB)

SQL injection vulnerability in ir.sequence

Bug #512682 reported by Albert Cervera i Areny - http://www.NaN-tic.com on 2010-01-26

258

This bug affects 1 person

Affects		Status	Importance	Assigned to	Milestone
	Odoo Server (MOVED TO GITHUB)	Fix Released	Critical	Christophe Simonis (OpenERP)	Odoo Server (MOVED TO GITHUB) 5.0.7

Bug Description

In ir_sequence.py:73:

def get_id(self, cr, uid, sequence_id, test='id=%s', context=None):
cr.execute('SELECT id, number_next, prefix, suffix, padding FROM ir_sequence WHERE '+test+' AND active=%s FOR UPDATE', (sequence_id, True))

Revising the recently commited patch to ir_sequence.py I realized there's an obvious SQL injection vulnerability. The bug requires changes in several modules that use this functionality. More precisely:

account/sequence.py
account_renumber/wizard/wizard_renumber.py
base_module_quality/speed_test/speed_test.py
base_module_quality/workflow_test/workflow_test.py

Related branches

lp:openobject-server/5.0

lp:openobject-addons/5.0

Revision history for this message

Albert Cervera i Areny - http://www.NaN-tic.com (albert-nan) wrote on 2010-01-26:

ir_sequence.py.diff Edit (1.0 KiB, text/plain)

The attached patch fixes the problem in ir_sequence.py

Raphaël Valyi - http://www.akretion.com (rvalyi) on 2010-01-26

Changed in openobject-server:
importance:	Undecided → Critical
milestone:	none → 5.0.7

Christophe Simonis (OpenERP) (kangol) on 2010-01-26

Changed in openobject-server:
status:	New → Confirmed
assignee:	nobody → Christophe (OpenERP) (kangol)

Revision history for this message

Christophe Simonis (OpenERP) (kangol) wrote on 2010-01-28: Re: [Bug 512682] Re: SQL injection vulnerability in ir.sequence

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 26/01/10 09:06, Albert Cervera i Areny wrote:
> The attached patch fixes the problem in ir_sequence.py
>
> ** Attachment added: "ir_sequence.py.diff"
> http://launchpadlibrarian.net/38393670/ir_sequence.py.diff
>

In order to not break any existing addons, I will apply your patch on
stable if you check if test.starswith('id=') instead of comparing to 'id'.

For trunk, I propose to redefine the method as
def get_id(self, cr, uid, sequence_id=None, code=None, context=None):

and then pass either the sequence_id either the code...

- --
Simonis Christophe
Quality & Support Manager
Open ERP - Tiny SPRL
Chaussée de Namur, 40
B-1367 Gérompont
Tel: +32.81.81.37.00
Web: http://openerp.com
Blog: http://christophe-simonis-at-tiny.blogspot.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkthg5IACgkQYeXc6dw+UFrZ4gCfYN7R2yXRAUpH4JEXn/WlWzWb
dpMAn0aI9ggwTMFN3ETQ3TedQ3m8bEnV
=FmBa
-----END PGP SIGNATURE-----

Revision history for this message

Albert Cervera i Areny - http://www.NaN-tic.com (albert-nan) wrote on 2010-01-28:

A Dijous, 28 de gener de 2010, Christophe (OpenERP) va escriure:
> In order to not break any existing addons, I will apply your patch on
> stable if you check if test.starswith('id=') instead of comparing to 'id'.
>

I'm ok with it, as long as you ensure that after "id=" there's a number only.

--
Albert Cervera i Areny
http://www.NaN-tic.com
Mòbil: +34 669 40 40 18

Revision history for this message

Christophe Simonis (OpenERP) (kangol) wrote on 2010-01-28:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 28/01/10 15:16, Albert Cervera i Areny wrote:
> A Dijous, 28 de gener de 2010, Christophe (OpenERP) va escriure:
>> In order to not break any existing addons, I will apply your patch on
>> stable if you check if test.starswith('id=') instead of comparing to 'id'.
>>
>
> I'm ok with it, as long as you ensure that after "id=" there's a number
> only.
>
Yes, as in your current patch...