Merge lp:~camptocamp/openobject-addons/7.0-fix_1196847 into lp:openobject-addons/7.0

Proposed by Nicolas Bessi - Camptocamp
Status: Needs review
Proposed branch: lp:~camptocamp/openobject-addons/7.0-fix_1196847
Merge into: lp:openobject-addons/7.0
Diff against target: 43 lines (+8/-7)
2 files modified
mail/data/mail_data.xml (+1/-1)
mail/update.py (+7/-6)
To merge this branch: bzr merge lp:~camptocamp/openobject-addons/7.0-fix_1196847
Reviewer Review Type Date Requested Status
Olivier Dony (Odoo) Needs Fixing
Alexandre Fayolle - camptocamp (community) code review, no test Approve
Holger Brunn (Therp) (community) code review Approve
Review via email: mp+172491@code.launchpad.net

Description of the change

Fix of bug 1196847 That may allows arbitrary code execution if safe_eval is tricked.

To post a comment you must log in.
Revision history for this message
Holger Brunn (Therp) (hbrunn) :
review: Approve (code review)
Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

I recommend keeping the call to uo.close() in order to have a clean termination of the HTTP session.

And I'd be glad to have a trace of the sent parameters and of the value of submit_result in the logs

review: Needs Fixing (code review, no test)
9280. By Nicolas Bessi - Camptocamp on 2013-07-02

[FIX] force close of the url

Revision history for this message
Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount) wrote :

Added url link object close

Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

LGTM

review: Approve (code review, no test)
9281. By Nicolas Bessi - Camptocamp on 2013-07-02

[IMP] add loggin of waranty server call

Revision history for this message
Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount) wrote :

Just added some login statement

Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote :

I think disabling the cron altogether is a wrong move. The level of criticality of this alleged vulnerability is quite low, while the mechanism to send system messages to OpenERP administrators is important and should not need to be manually turned on. It was used in the past to notify all users (not just OE/OPW customers!) about a critical vulnerability in OpenERP 6.0 that needed to be patched urgently [1], and it was levels of magnitude more serious than the potential exploit of this notification mechanism.

It seems to me that replacing safe_eval() with ast.literal_eval() and forcing an HTTPS URL would reduce the possibilities of misuse to an acceptable level. The handling of the return values is the actual mechanism used to notify system administrators of important messages.

BTW if you think safe_eval is not safe you should post another bug report with the issues you've found with it, so that we can fix them - as this would have a much bigger impact!

Also, log messages are not be translated, and this particular extra logging seems a bit superfluous. The mechanism is meant to be fault-tolerant on purpose.

[1] bug 832601

review: Needs Fixing
Revision history for this message
Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount) wrote :

IMO the concept of executing arbitrary distant code is defective by design and should be simply removed. There is other better patterns to notify users.

Try simply to do in a Python consol:

import ast
ast.literal_eval(82173821737213782173821739921**881230980921832173821732132323798321) # Via Alexandre Fayolle

If you are not in multi process your instance will froze as long the GIL is not released.

It never will be safe to directly execute code from external sources.

Even if we use SSL we have to ensure it is not only used for encryption but also for source authentication and reject any action if authentication fails. That's may probably means some infrastructure work for OpenERP SA (please confirm). I do not want so send user info to undue people. As long that I have not more guaranty that this stats will arrive to OpenERP SA, I do not think I want to activate the cron.

This kind of authentication/security should also be required by the internal "app store" of OpenERP.

9282. By Nicolas Bessi - Camptocamp on 2013-07-02

[FIX] log messages should not be translated

Revision history for this message
Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount) wrote :

Pushed correction log messages should not be translated

Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote :

On 07/02/2013 01:48 PM, Nicolas Bessi - Camptocamp wrote:
> Even if we use SSL we have to ensure it is not only used for encryption but
> also for source authentication and reject any action if authentication
> fails.

Yes, server-side certificate chain validation is a key part of the TLS
protocol[1], and it is enabled by default on all HTTPS stacks.
It does not require any extra work on OpenERP Publisher Warranty servers
because they are already deployed with valid SSL certificates.

> This kind of authentication/security should also be required by the internal
> "app store" of OpenERP.

The OpenERP Apps integration in OpenERP 7.0 is done using HTTPS.

[1] https://tools.ietf.org/html/rfc5246#section-7.4.2

Revision history for this message
Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount) wrote :

My point with SSL was more to know if OpenERP have self signed certificat, forme those services or use a registered ca

Unmerged revisions

9282. By Nicolas Bessi - Camptocamp on 2013-07-02

[FIX] log messages should not be translated

9281. By Nicolas Bessi - Camptocamp on 2013-07-02

[IMP] add loggin of waranty server call

9280. By Nicolas Bessi - Camptocamp on 2013-07-02

[FIX] force close of the url

9279. By Nicolas Bessi - Camptocamp on 2013-07-02

[FIX] arbitrary code execution

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'mail/data/mail_data.xml'
2--- mail/data/mail_data.xml 2013-03-07 13:25:17 +0000
3+++ mail/data/mail_data.xml 2013-07-02 11:53:28 +0000
4@@ -15,7 +15,7 @@
5
6 <record id="ir_cron_module_update_notification" model="ir.cron">
7 <field name="name">Update Notification</field>
8- <field eval="True" name="active" />
9+ <field eval="False" name="active" />
10 <field name="user_id" ref="base.user_root" />
11 <field name="interval_number">1</field>
12 <field name="interval_type">weeks</field>
13
14=== modified file 'mail/update.py'
15--- mail/update.py 2013-02-05 22:29:22 +0000
16+++ mail/update.py 2013-07-02 11:53:28 +0000
17@@ -62,14 +62,16 @@
18 arguments_raw = urllib.urlencode(arguments)
19
20 url = config.get("publisher_warranty_url")
21-
22- uo = urllib2.urlopen(url, arguments_raw, **add_arg)
23- result = {}
24 try:
25+ uo = urllib2.urlopen(url, arguments_raw, **add_arg) # Does it support context manager?
26 submit_result = uo.read()
27- result = safe_eval(submit_result)
28+ _logger.info("Call warranty server with args: %s \n result: %s" %
29+ (add_arg, submit_result))
30+ except (urllib2.URLError, urllib2.HTTPError) as exc:
31+ _logger.error("Unable to query warranty server: %s" % repr(exc))
32 finally:
33- uo.close()
34+ uo.close() # We let any close exception be risen
35+ result = {}
36 return result
37
38 class publisher_warranty_contract(osv.osv):
39@@ -113,4 +115,3 @@
40 return True
41
42 # vim:expandtab:smartindent:tabstop=4:softtabstop=4:shiftwidth=4:
43-