Issue #22796: HTTP cookie parsing is now stricter, in order to protect against potential injection attacks.

3 files changed, 48 insertions, 23 deletions

diff --git a/Lib/http/cookies.py b/Lib/http/cookies.py
index a6de6d5..7ec9274 100644
--- a/Lib/http/cookies.py
+++ b/Lib/http/cookies.py
@@ -533,10 +533,17 @@ class BaseCookie(dict):
         return
 
     def __parse_string(self, str, patt=_CookiePattern):
-        i = 0            # Our starting point
-        n = len(str)     # Length of string
-        M = None         # current morsel
+        i = 0                 # Our starting point
+        n = len(str)          # Length of string
+        parsed_items = []     # Parsed (type, key, value) triples
+        morsel_seen = False   # A key=value pair was previously encountered
+
+        TYPE_ATTRIBUTE = 1
+        TYPE_KEYVALUE = 2
 
+        # We first parse the whole cookie string and reject it if it's
+        # syntactically invalid (this helps avoid some classes of injection
+        # attacks).
         while 0 <= i < n:
             # Start looking for a cookie
             match = patt.match(str, i)
@@ -547,22 +554,41 @@ class BaseCookie(dict):
             key, value = match.group("key"), match.group("val")
             i = match.end(0)
 
-            # Parse the key, value in case it's metainfo
             if key[0] == "$":
-                # We ignore attributes which pertain to the cookie
-                # mechanism as a whole.  See RFC 2109.
-                # (Does anyone care?)
-                if M:
-                    M[key[1:]] = value
+                if not morsel_seen:
+                    # We ignore attributes which pertain to the cookie
+                    # mechanism as a whole, such as "$Version".
+                    # See RFC 2965. (Does anyone care?)
+                    continue
+                parsed_items.append((TYPE_ATTRIBUTE, key[1:], value))
             elif key.lower() in Morsel._reserved:
-                if M:
-                    if value is None:
-                        if key.lower() in Morsel._flags:
-                            M[key] = True
+                if not morsel_seen:
+                    # Invalid cookie string
+                    return
+                if value is None:
+                    if key.lower() in Morsel._flags:
+                        parsed_items.append((TYPE_ATTRIBUTE, key, True))
                     else:
-                        M[key] = _unquote(value)
+                        # Invalid cookie string
+                        return
+                else:
+                    parsed_items.append((TYPE_ATTRIBUTE, key, _unquote(value)))
             elif value is not None:
-                rval, cval = self.value_decode(value)
+                parsed_items.append((TYPE_KEYVALUE, key, self.value_decode(value)))
+                morsel_seen = True
+            else:
+                # Invalid cookie string
+                return
+
+        # The cookie string is valid, apply it.
+        M = None         # current morsel
+        for tp, key, value in parsed_items:
+            if tp == TYPE_ATTRIBUTE:
+                assert M is not None
+                M[key] = value
+            else:
+                assert tp == TYPE_KEYVALUE
+                rval, cval = value
                 self.__set(key, rval, cval)
                 M = self[key]
 
diff --git a/Lib/test/test_http_cookies.py b/Lib/test/test_http_cookies.py
index 2b0281e..7c0e01b 100644
--- a/Lib/test/test_http_cookies.py
+++ b/Lib/test/test_http_cookies.py
@@ -141,13 +141,6 @@ class CookieTests(unittest.TestCase):
         self.assertEqual(C['eggs']['httponly'], 'foo')
         self.assertEqual(C['eggs']['secure'], 'bar')
 
-    def test_bad_attrs(self):
-        # issue 16611: make sure we don't break backward compatibility.
-        C = cookies.SimpleCookie()
-        C.load('cookie=with; invalid; version; second=cookie;')
-        self.assertEqual(C.output(),
-            'Set-Cookie: cookie=with\r\nSet-Cookie: second=cookie')
-
     def test_extra_spaces(self):
         C = cookies.SimpleCookie()
         C.load('eggs  =  scrambled  ;  secure  ;  path  =  bar   ; foo=foo   ')
@@ -182,7 +175,10 @@ class CookieTests(unittest.TestCase):
     def test_invalid_cookies(self):
         # Accepting these could be a security issue
         C = cookies.SimpleCookie()
-        for s in (']foo=x', '[foo=x', 'blah]foo=x', 'blah[foo=x'):
+        for s in (']foo=x', '[foo=x', 'blah]foo=x', 'blah[foo=x',
+                  'Set-Cookie: foo=bar', 'Set-Cookie: foo',
+                  'foo=bar; baz', 'baz; foo=bar',
+                  'secure;foo=bar', 'Version=1;foo=bar'):
             C.load(s)
             self.assertEqual(dict(C), {})
             self.assertEqual(C.output(), '')
diff --git a/Misc/NEWS b/Misc/NEWS
index 7d97ae6..127efc6 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -188,6 +188,9 @@ Core and Builtins
 Library
 -------
 
+- Issue #22796: HTTP cookie parsing is now stricter, in order to protect
+  against potential injection attacks.
+
 - Issue #22370: Windows detection in pathlib is now more robust.
 
 - Issue #22841: Reject coroutines in asyncio add_signal_handler().