Chris McDonough's patch to defend against certain DoS attacks on FieldStorage.

SF bug #1112549.

Lib/cgi.py

@@ -251,6 +251,10 @@ def parse_multipart(fp, pdict):
 
     XXX This should really be subsumed by FieldStorage altogether -- no
     point in having two implementations of the same parsing algorithm.
+    Also, FieldStorage protects itself better against certain DoS attacks
+    by limiting the size of the data read in one chunk.  The API here
+    does not support that kind of protection.  This also affects parse()
+    since it can call parse_multipart().
 
     """
     boundary = ""
@@ -699,7 +703,7 @@ class FieldStorage:
     def read_lines_to_eof(self):
         """Internal: read lines until EOF."""
         while 1:
-            line = self.fp.readline()
+            line = self.fp.readline(1<<16)
             if not line:
                 self.done = -1
                 break
@@ -710,12 +714,13 @@ class FieldStorage:
         next = "--" + self.outerboundary
         last = next + "--"
         delim = ""
+        last_line_lfend = True
         while 1:
-            line = self.fp.readline()
+            line = self.fp.readline(1<<16)
             if not line:
                 self.done = -1
                 break
-            if line[:2] == "--":
+            if line[:2] == "--" and last_line_lfend:
                 strippedline = line.strip()
                 if strippedline == next:
                     break
@@ -726,11 +731,14 @@ class FieldStorage:
             if line[-2:] == "\r\n":
                 delim = "\r\n"
                 line = line[:-2]
+                last_line_lfend = True
             elif line[-1] == "\n":
                 delim = "\n"
                 line = line[:-1]
+                last_line_lfend = True
             else:
                 delim = ""
+                last_line_lfend = False
             self.__write(odelim + line)
 
     def skip_lines(self):
@@ -739,18 +747,20 @@ class FieldStorage:
             return
         next = "--" + self.outerboundary
         last = next + "--"
+        last_line_lfend = True
         while 1:
-            line = self.fp.readline()
+            line = self.fp.readline(1<<16)
             if not line:
                 self.done = -1
                 break
-            if line[:2] == "--":
+            if line[:2] == "--" and last_line_lfend:
                 strippedline = line.strip()
                 if strippedline == next:
                     break
                 if strippedline == last:
                     self.done = 1
                     break
+            last_line_lfend = line.endswith('\n')
 
     def make_file(self, binary=None):
         """Overridable: return a readable & writable file.

Lib/test/output/test_cgi

@@ -38,3 +38,5 @@ test_cgi
 Testing log
 Testing initlog 1
 Testing log 2
+Test FieldStorage methods that use readline
+Test basic FieldStorage multipart parsing

Lib/test/test_cgi.py

@@ -2,6 +2,8 @@ from test.test_support import verify, verbose
 import cgi
 import os
 import sys
+import tempfile
+from StringIO import StringIO
 
 class HackedSysModule:
     # The regression test will have real values in sys.argv, which
@@ -203,4 +205,71 @@ def main():
         cgi.initlog("%s", "Testing log 3")
         cgi.log("Testing log 4")
 
+    print "Test FieldStorage methods that use readline"
+    # FieldStorage uses readline, which has the capacity to read all
+    # contents of the input file into memory; we use readline's size argument
+    # to prevent that for files that do not contain any newlines in
+    # non-GET/HEAD requests
+    class TestReadlineFile:
+        def __init__(self, file):
+            self.file = file
+            self.numcalls = 0
+
+        def readline(self, size=None):
+            self.numcalls += 1
+            if size:
+                return self.file.readline(size)
+            else:
+                return self.file.readline()
+
+        def __getattr__(self, name):
+            file = self.__dict__['file']
+            a = getattr(file, name)
+            if not isinstance(a, int):
+                setattr(self, name, a)
+            return a
+
+    f = TestReadlineFile(tempfile.TemporaryFile())
+    f.write('x' * 256 * 1024)
+    f.seek(0)
+    env = {'REQUEST_METHOD':'PUT'}
+    fs = cgi.FieldStorage(fp=f, environ=env)
+    # if we're not chunking properly, readline is only called twice
+    # (by read_binary); if we are chunking properly, it will be called 5 times
+    # as long as the chunksize is 1 << 16.
+    verify(f.numcalls > 2)
+
+    print "Test basic FieldStorage multipart parsing"
+    env = {'REQUEST_METHOD':'POST', 'CONTENT_TYPE':'multipart/form-data; boundary=---------------------------721837373350705526688164684', 'CONTENT_LENGTH':'558'}
+    postdata = r"""-----------------------------721837373350705526688164684
+Content-Disposition: form-data; name="id"
+
+1234
+-----------------------------721837373350705526688164684
+Content-Disposition: form-data; name="title"
+
+
+-----------------------------721837373350705526688164684
+Content-Disposition: form-data; name="file"; filename="test.txt"
+Content-Type: text/plain
+
+Testing 123.
+
+-----------------------------721837373350705526688164684
+Content-Disposition: form-data; name="submit"
+
+ Add 
+-----------------------------721837373350705526688164684--
+"""
+    fs = cgi.FieldStorage(fp=StringIO(postdata), environ=env)
+    verify(len(fs.list) == 4)
+    expect = [{'name':'id', 'filename':None, 'value':'1234'},
+              {'name':'title', 'filename':None, 'value':''},
+              {'name':'file', 'filename':'test.txt','value':'Testing 123.\n'},
+              {'name':'submit', 'filename':None, 'value':' Add '}]
+    for x in range(len(fs.list)):
+        for k, exp in expect[x].items():
+            got = getattr(fs.list[x], k)
+            verify(got == exp)
+
 main()