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

[Nicolas Bessi - Camptocamp]

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

[Holger Brunn (Therp) (hbrunn)]

review: Approve (code review)

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

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)

[lp:~camptocamp/openobject-addons/7.0-fix_1196847]

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

[FIX] force close of the url

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Added url link object close

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

LGTM

review: Approve (code review, no test)

[lp:~camptocamp/openobject-addons/7.0-fix_1196847]

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

[IMP] add loggin of waranty server call

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Just added some login statement

[Olivier Dony (Odoo) (odo-openerp)]

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

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

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.

[lp:~camptocamp/openobject-addons/7.0-fix_1196847]

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

[FIX] log messages should not be translated

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Pushed correction log messages should not be translated

[Olivier Dony (Odoo) (odo-openerp)]

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

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

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

=== modified file 'mail/data/mail_data.xml'
--- mail/data/mail_data.xml	2013-03-07 13:25:17 +0000
+++ mail/data/mail_data.xml	2013-07-02 11:53:28 +0000
@@ -15,7 +15,7 @@
 
         <record id="ir_cron_module_update_notification" model="ir.cron">
             <field name="name">Update Notification</field>
-            <field eval="True" name="active" />
+            <field eval="False" name="active" />
             <field name="user_id" ref="base.user_root" />
             <field name="interval_number">1</field>
             <field name="interval_type">weeks</field>

=== modified file 'mail/update.py'
--- mail/update.py	2013-02-05 22:29:22 +0000
+++ mail/update.py	2013-07-02 11:53:28 +0000
@@ -62,14 +62,16 @@
     arguments_raw = urllib.urlencode(arguments)
 
     url = config.get("publisher_warranty_url")
-
-    uo = urllib2.urlopen(url, arguments_raw, **add_arg)
-    result = {}
     try:
+        uo = urllib2.urlopen(url, arguments_raw, **add_arg)  # Does it support context manager?
         submit_result = uo.read()
-        result = safe_eval(submit_result)
+        _logger.info("Call warranty server with args: %s \n  result: %s" %
+                     (add_arg, submit_result))
+    except (urllib2.URLError, urllib2.HTTPError) as exc:
+        _logger.error("Unable to query warranty server: %s" % repr(exc))
     finally:
-        uo.close()
+        uo.close()  # We let any close exception be risen
+    result = {}
     return result
 
 class publisher_warranty_contract(osv.osv):
@@ -113,4 +115,3 @@
         return True
 
 # vim:expandtab:smartindent:tabstop=4:softtabstop=4:shiftwidth=4:
-