[Zope-Checkins] CVS: Zope/lib/python/TreeDisplay - TreeTag.py:1.57

Tres Seaver tseaver at zope.com
Thu Jan 15 17:58:26 EST 2004


Update of /cvs-repository/Zope/lib/python/TreeDisplay
In directory cvs.zope.org:/tmp/cvs-serv22550

Modified Files:
	TreeTag.py 
Log Message:


  - Prevent DoS attack against decompression of tree state cookie (merged
    from 2.6 / 2.7 audit).


=== Zope/lib/python/TreeDisplay/TreeTag.py 1.56 => 1.57 ===
--- Zope/lib/python/TreeDisplay/TreeTag.py:1.56	Thu Dec 11 17:49:17 2003
+++ Zope/lib/python/TreeDisplay/TreeTag.py	Thu Jan 15 17:58:25 2004
@@ -18,9 +18,10 @@
 from DocumentTemplate.DT_Util import *
 from DocumentTemplate.DT_String import String
 
+from cPickle import dumps
 from string import translate
 from urllib import quote, unquote
-from zlib import compress, decompress
+from zlib import compress, decompressobj
 from binascii import b2a_base64, a2b_base64
 import re
 
@@ -115,15 +116,15 @@
       ['eagle'], # eagle is open
       ['eagle'], ['jeep', [1983, 1985]]  # eagle, jeep, 1983 jeep and 1985 jeep
 
-    where the items are object ids. The state will be converted to a
+    where the items are object ids. The state will be pickled to a
     compressed and base64ed string that gets unencoded, uncompressed,
-    and evaluated on the other side.
+    and unpickled on the other side.
 
     Note that ids used in state need not be connected to urls, since
     state manipulation is internal to rendering logic.
 
-    Note that to make eval safe, we do not allow the character '*' in
-    the state.
+    Note that to make unpickling safe, we use the MiniPickle module,
+    that only creates safe objects
     """
 
     data=[]
@@ -333,7 +334,7 @@
 
             ####################################
             # Mostly inline encode_seq for speed
-            s=compress(str(diff))
+            s=compress(dumps(diff,1))
             if len(s) > 57: s=encode_str(s)
             else:
                 s=b2a_base64(s)[:-1]
@@ -523,7 +524,7 @@
 
 def encode_seq(state):
     "Convert a sequence to an encoded string"
-    state=compress(str(state))
+    state=compress(dumps(state))
     l=len(state)
 
     if l > 57:
@@ -583,10 +584,20 @@
         state=a2b_base64(state)
 
     state=decompress(state)
-    if state.find('*') >= 0: raise ValueError, 'Illegal State: %s' % state
-    try: return list(eval(state,{'__builtins__':{}}))
+    try: return list(MiniUnpickler(StringIO(state)).load())
     except: return []
 
+def decompress(input,max_size=10240):
+    # This sillyness can go away in python 2.2
+    d = decompressobj()
+    output = ''
+    while input:
+        fragment_size = max(1,(max_size-len(output))/1000)
+        fragment,input = input[:fragment_size],input[fragment_size:]
+        output += d.decompress(fragment)
+        if len(output)>max_size:
+            raise ValueError('Compressed input too large')
+    return output+d.flush()
 
 def tpStateLevel(state, level=0):
     for sub in state:
@@ -632,3 +643,79 @@
 #icoSpace='<IMG SRC="Blank_icon" BORDER="0">'
 #icoPlus ='<IMG SRC="Plus_icon" BORDER="0">'
 #icoMinus='<IMG SRC="Minus_icon" BORDER="0">'
+
+
+
+
+
+###############################################################################
+## Everthing below here should go in a MiniPickle.py module, but keeping it
+## internal makes an easier patch
+
+
+import pickle
+from cStringIO import StringIO
+
+
+if pickle.format_version!="2.0":
+    # Maybe the format changed, and opened a security hole
+    raise 'Invalid pickle version'
+
+
+class MiniUnpickler(pickle.Unpickler):
+    """An unpickler that can only handle simple types.
+    """
+    def refuse_to_unpickle(self):
+        raise pickle.UnpicklingError, 'Refused'
+
+    dispatch = pickle.Unpickler.dispatch.copy()
+
+    for k,v in dispatch.items():
+        if k=='' or k in '().012FGIJKLMNTUVX]adeghjlpqrstu}':
+            # This key is necessary and safe, so leave it in the map
+            pass
+        else:
+            dispatch[k] = refuse_to_unpickle
+            # Anything unnecessary is banned, but here is some logic to explain why
+            if k in [pickle.GLOBAL, pickle.OBJ, pickle.INST, pickle.REDUCE, pickle.BUILD]:
+                # These are definite security holes
+                pass
+            elif k in [pickle.PERSID, pickle.BINPERSID]:
+                # These are just unnecessary
+                pass
+            elif k in [pickle.STRING]:
+                # This one is controversial: A string is harmlessm, but the
+                # implementation of pickle leaks memory (strings may be interned)
+                # The problem can be avoided by using binary pickles.
+                pass
+    del k
+    del v
+
+def _should_succeed(x,binary=1):
+    if x != MiniUnpickler(StringIO(pickle.dumps(x,binary))).load():
+        raise ValueError(x)
+
+def _should_fail(x,binary=1):
+    try:
+        MiniUnpickler(StringIO(pickle.dumps(x,binary))).load()
+        raise ValueError(x)
+    except pickle.UnpicklingError, e:
+        if e[0]!='Refused': raise ValueError(x)
+
+class _junk_class: pass
+
+def _test():
+    _should_fail('hello',0)
+    _should_succeed('hello')
+    _should_succeed(1)
+    _should_succeed(1L)
+    _should_succeed(1.0)
+    _should_succeed((1,2,3))
+    _should_succeed([1,2,3])
+    _should_succeed({1:2,3:4})
+    _should_fail(open)
+    _should_fail(_junk_class)
+    _should_fail(_junk_class())
+
+# Test MiniPickle on every import
+_test()




More information about the Zope-Checkins mailing list