Merge lp:~openerp-dev/openobject-client/trunk-bug-671926-nch into lp:openobject-client

[Naresh(OpenERP)]

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

Hi Naresh,

May I suggest some additional improvements:
- please include the module and name in the exceptions message, for diagnostics purpose, e.g:
   raise cPickle.UnpicklingError('Unsafe pickled class instance: %s.%s' % (module,name))

- different kinds of exceptions may be passed over netrpc apparently, so until we patch the server to wrap them all, we can allow them all like this (need to add import 'exceptions' and 'types'):

  EXCEPTION_CLASSES = [x for x in dir(exceptions) if type(getattr(exceptions,x)) == types.TypeType]
  SAFE_CLASSES = { 'exceptions' : EXCEPTION_CLASSES }

Thanks!

[lp:~openerp-dev/openobject-client/trunk-bug-671926-nch]

1918. By Naresh(OpenERP) on 2011-07-04

[IMP]:included module and name in the exceptions message

[Xavier (Open ERP) (xmo-deactivatedaccount)]

> type(getattr(exceptions,x)) == types.TypeType

No need to go through `types.TypeType`, we can just use `type` here:

    >>> types.TypeType is type
    True

I'd also use `issubclass(getattr(exceptions, x), Exception)` as the filter, instead. Makes what we're interested in clearer, and lets system-stopping exceptions go through unmolested.

On the other hand, are we sure it's not possible for attackers to get new stuff into `exceptions` before this code runs?

[lp:~openerp-dev/openobject-client/trunk-bug-671926-nch]

1919. By Naresh(OpenERP) on 2011-07-12

[IMP]:added all types of exceptions in the safe_classes

[Naresh(OpenERP) (nch-openerp)]

Hello Olivier,

Thanks for the suggestions !

I have made the improvements. Can you please verify them.

Thanks,

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

@Xavier: thanks for the suggestions. I don't think system-stopping exceptions should be sent via RPC to the client, but using issubclass() might be more readable anyhow. As for altering 'exceptions', if an attacker had a way to do it, then he also has a way to directly execute arbitrary code, so this check does not help.

@Naresh: looks good, could you apply Xavier's suggestion with issubclass()?

[Naresh(OpenERP) (nch-openerp)]

getattr will return a 'str' and not a class instance also we can have non Exception type like the built-ins , __doc__ the docstrings etc.

so the type check is appropriate.

=== modified file 'bin/tiny_socket.py'
--- bin/tiny_socket.py	2010-01-12 09:24:17 +0000
+++ bin/tiny_socket.py	2011-07-12 06:51:23 +0000
@@ -1,6 +1,6 @@
 # -*- coding: utf-8 -*-
 ##############################################################################
-#    
+#
 #    OpenERP, Open Source Management Solution
 #    Copyright (C) 2004-2010 Tiny SPRL (<http://tiny.be>).
 #
@@ -15,13 +15,15 @@
 #    GNU Affero General Public License for more details.
 #
 #    You should have received a copy of the GNU Affero General Public License
-#    along with this program.  If not, see <http://www.gnu.org/licenses/>.     
+#    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 ##############################################################################
 
 import socket
 import cPickle
+import cStringIO
 import sys
+import exceptions
 import options
 
 DNS_CACHE = {}
@@ -42,6 +44,17 @@
         self.faultString = faultString
         self.args = (faultCode, faultString)
 
+# Safety class instance loader for unpickling.
+# Inspired by http://nadiana.com/python-pickle-insecure#How_to_Make_Unpickling_Safer
+EXCEPTION_CLASSES = [x for x in dir(exceptions) if type(getattr(exceptions,x)) == type]
+SAFE_CLASSES = { 'exceptions' : EXCEPTION_CLASSES }
+def find_global(module, name):
+    if module not in SAFE_CLASSES or name not in SAFE_CLASSES[module]:
+        raise cPickle.UnpicklingError('Attempting to unpickle unsafe module %s.%s' % (module,name))
+    __import__(module)
+    mod = sys.modules[module]
+    return getattr(mod, name)
+
 class mysocket:
     def __init__(self, sock=None):
         if sock is None:
@@ -84,7 +97,11 @@
         size = int(read(self.sock, 8))
         buf = read(self.sock, 1)
         exception = buf != '0' and buf or False
-        res = cPickle.loads(read(self.sock, size))
+        buf = read(self.sock, size)
+        msgio = cStringIO.StringIO(buf)
+        unpickler = cPickle.Unpickler(msgio)
+        unpickler.find_global = find_global
+        res = unpickler.load()
 
         if isinstance(res[0],Exception):
             if exception: