Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

python27: fix CVE-2021-23336 #118403

Merged
merged 1 commit into from
Apr 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
390 changes: 390 additions & 0 deletions pkgs/development/interpreters/python/cpython/2.7/CVE-2021-23336.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,390 @@
From e7b005c05dbdbce967a409abd71641281a8604bf Mon Sep 17 00:00:00 2001
From: Senthil Kumaran <senthil@uthcode.com>
Date: Mon, 15 Feb 2021 11:16:43 -0800
Subject: [PATCH 24/26] [3.6] bpo-42967: only use '&' as a query string
separator (GH-24297) (GH-24532)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

bpo-42967: [security] Address a web cache-poisoning issue reported in
urllib.parse.parse_qsl().

urllib.parse will only us "&" as query string separator by default
instead of both ";" and "&" as allowed in earlier versions. An optional
argument seperator with default value "&" is added to specify the
separator.

Co-authored-by: Éric Araujo <merwok@netwok.org>
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
Co-authored-by: Adam Goldschmidt <adamgold7@gmail.com>

Rebased for Python 2.7 by Michał Górny
---
Doc/library/cgi.rst | 7 +++-
Doc/library/urlparse.rst | 23 ++++++++++-
Lib/cgi.py | 20 +++++++---
Lib/test/test_cgi.py | 29 +++++++++++---
Lib/test/test_urlparse.py | 38 +++++++++----------
Lib/urlparse.py | 22 ++++++++---
.../2021-02-14-15-59-16.bpo-42967.YApqDS.rst | 1 +
7 files changed, 100 insertions(+), 40 deletions(-)
create mode 100644 Misc/NEWS.d/next/Security/2021-02-14-15-59-16.bpo-42967.YApqDS.rst

diff --git a/Doc/library/cgi.rst b/Doc/library/cgi.rst
index ecd62c8c01..b85cdd8b61 100644
--- a/Doc/library/cgi.rst
+++ b/Doc/library/cgi.rst
@@ -285,10 +285,10 @@ These are useful if you want more control, or if you want to employ some of the
algorithms implemented in this module in other circumstances.


-.. function:: parse(fp[, environ[, keep_blank_values[, strict_parsing]]])
+.. function:: parse(fp[, environ[, keep_blank_values[, strict_parsing]]], separator="&")

Parse a query in the environment or from a file (the file defaults to
- ``sys.stdin`` and environment defaults to ``os.environ``). The *keep_blank_values* and *strict_parsing* parameters are
+ ``sys.stdin`` and environment defaults to ``os.environ``). The *keep_blank_values*, *strict_parsing* and *separator* parameters are
passed to :func:`urlparse.parse_qs` unchanged.


@@ -316,6 +316,9 @@ algorithms implemented in this module in other circumstances.
Note that this does not parse nested multipart parts --- use
:class:`FieldStorage` for that.

+ .. versionchanged:: 3.6.13
+ Added the *separator* parameter.
+

.. function:: parse_header(string)

diff --git a/Doc/library/urlparse.rst b/Doc/library/urlparse.rst
index 0989c88c30..2f8e4c5a44 100644
--- a/Doc/library/urlparse.rst
+++ b/Doc/library/urlparse.rst
@@ -136,7 +136,7 @@ The :mod:`urlparse` module defines the following functions:
now raise :exc:`ValueError`.


-.. function:: parse_qs(qs[, keep_blank_values[, strict_parsing[, max_num_fields]]])
+.. function:: parse_qs(qs[, keep_blank_values[, strict_parsing[, max_num_fields]]], separator='&')

Parse a query string given as a string argument (data of type
:mimetype:`application/x-www-form-urlencoded`). Data are returned as a
@@ -157,6 +157,9 @@ The :mod:`urlparse` module defines the following functions:
read. If set, then throws a :exc:`ValueError` if there are more than
*max_num_fields* fields read.

+ The optional argument *separator* is the symbol to use for separating the
+ query arguments. It defaults to ``&``.
+
Use the :func:`urllib.urlencode` function to convert such dictionaries into
query strings.

@@ -166,7 +169,14 @@ The :mod:`urlparse` module defines the following functions:
.. versionchanged:: 2.7.16
Added *max_num_fields* parameter.

-.. function:: parse_qsl(qs[, keep_blank_values[, strict_parsing[, max_num_fields]]])
+ .. versionchanged:: 2.7.18-gentoo
+ Added *separator* parameter with the default value of ``&``. Earlier
+ Python versions allowed using both ``;`` and ``&`` as query parameter
+ separator. This has been changed to allow only a single separator key,
+ with ``&`` as the default separator.
+
+
+.. function:: parse_qsl(qs[, keep_blank_values[, strict_parsing[, max_num_fields]]], separator='&')

Parse a query string given as a string argument (data of type
:mimetype:`application/x-www-form-urlencoded`). Data are returned as a list of
@@ -186,6 +196,9 @@ The :mod:`urlparse` module defines the following functions:
read. If set, then throws a :exc:`ValueError` if there are more than
*max_num_fields* fields read.

+ The optional argument *separator* is the symbol to use for separating the
+ query arguments. It defaults to ``&``.
+
Use the :func:`urllib.urlencode` function to convert such lists of pairs into
query strings.

@@ -195,6 +208,12 @@ The :mod:`urlparse` module defines the following functions:
.. versionchanged:: 2.7.16
Added *max_num_fields* parameter.

+ .. versionchanged:: 2.7.18-gentoo
+ Added *separator* parameter with the default value of ``&``. Earlier
+ Python versions allowed using both ``;`` and ``&`` as query parameter
+ separator. This has been changed to allow only a single separator key,
+ with ``&`` as the default separator.
+
.. function:: urlunparse(parts)

Construct a URL from a tuple as returned by ``urlparse()``. The *parts* argument
diff --git a/Lib/cgi.py b/Lib/cgi.py
index 5b903e0347..9d0848b6b1 100755
--- a/Lib/cgi.py
+++ b/Lib/cgi.py
@@ -121,7 +121,8 @@ log = initlog # The current logging function
# 0 ==> unlimited input
maxlen = 0

-def parse(fp=None, environ=os.environ, keep_blank_values=0, strict_parsing=0):
+def parse(fp=None, environ=os.environ, keep_blank_values=0,
+ strict_parsing=0, separator='&'):
"""Parse a query in the environment or from a file (default stdin)

Arguments, all optional:
@@ -140,6 +141,9 @@ def parse(fp=None, environ=os.environ, keep_blank_values=0, strict_parsing=0):
strict_parsing: flag indicating what to do with parsing errors.
If false (the default), errors are silently ignored.
If true, errors raise a ValueError exception.
+
+ separator: str. The symbol to use for separating the query arguments.
+ Defaults to &.
"""
if fp is None:
fp = sys.stdin
@@ -171,7 +175,8 @@ def parse(fp=None, environ=os.environ, keep_blank_values=0, strict_parsing=0):
else:
qs = ""
environ['QUERY_STRING'] = qs # XXX Shouldn't, really
- return urlparse.parse_qs(qs, keep_blank_values, strict_parsing)
+ return urlparse.parse_qs(qs, keep_blank_values, strict_parsing,
+ separator=separator)


# parse query string function called from urlparse,
@@ -395,7 +400,7 @@ class FieldStorage:

def __init__(self, fp=None, headers=None, outerboundary="",
environ=os.environ, keep_blank_values=0, strict_parsing=0,
- max_num_fields=None):
+ max_num_fields=None, separator='&'):
"""Constructor. Read multipart/* until last part.

Arguments, all optional:
@@ -430,6 +435,7 @@ class FieldStorage:
self.keep_blank_values = keep_blank_values
self.strict_parsing = strict_parsing
self.max_num_fields = max_num_fields
+ self.separator = separator
if 'REQUEST_METHOD' in environ:
method = environ['REQUEST_METHOD'].upper()
self.qs_on_post = None
@@ -613,7 +619,8 @@ class FieldStorage:
if self.qs_on_post:
qs += '&' + self.qs_on_post
query = urlparse.parse_qsl(qs, self.keep_blank_values,
- self.strict_parsing, self.max_num_fields)
+ self.strict_parsing, self.max_num_fields,
+ separator=self.separator)
self.list = [MiniFieldStorage(key, value) for key, value in query]
self.skip_lines()

@@ -629,7 +636,8 @@ class FieldStorage:
query = urlparse.parse_qsl(self.qs_on_post,
self.keep_blank_values,
self.strict_parsing,
- self.max_num_fields)
+ self.max_num_fields,
+ separator=self.separator)
self.list.extend(MiniFieldStorage(key, value)
for key, value in query)
FieldStorageClass = None
@@ -649,7 +657,7 @@ class FieldStorage:
headers = rfc822.Message(self.fp)
part = klass(self.fp, headers, ib,
environ, keep_blank_values, strict_parsing,
- max_num_fields)
+ max_num_fields, separator=self.separator)

if max_num_fields is not None:
max_num_fields -= 1
diff --git a/Lib/test/test_cgi.py b/Lib/test/test_cgi.py
index 743c2afbd4..f414faa23b 100644
--- a/Lib/test/test_cgi.py
+++ b/Lib/test/test_cgi.py
@@ -61,12 +61,9 @@ parse_strict_test_cases = [
("", ValueError("bad query field: ''")),
("&", ValueError("bad query field: ''")),
("&&", ValueError("bad query field: ''")),
- (";", ValueError("bad query field: ''")),
- (";&;", ValueError("bad query field: ''")),
# Should the next few really be valid?
("=", {}),
("=&=", {}),
- ("=;=", {}),
# This rest seem to make sense
("=a", {'': ['a']}),
("&=a", ValueError("bad query field: ''")),
@@ -81,8 +78,6 @@ parse_strict_test_cases = [
("a=a+b&b=b+c", {'a': ['a b'], 'b': ['b c']}),
("a=a+b&a=b+a", {'a': ['a b', 'b a']}),
("x=1&y=2.0&z=2-3.%2b0", {'x': ['1'], 'y': ['2.0'], 'z': ['2-3.+0']}),
- ("x=1;y=2.0&z=2-3.%2b0", {'x': ['1'], 'y': ['2.0'], 'z': ['2-3.+0']}),
- ("x=1;y=2.0;z=2-3.%2b0", {'x': ['1'], 'y': ['2.0'], 'z': ['2-3.+0']}),
("Hbc5161168c542333633315dee1182227:key_store_seqid=400006&cuyer=r&view=bustomer&order_id=0bb2e248638833d48cb7fed300000f1b&expire=964546263&lobale=en-US&kid=130003.300038&ss=env",
{'Hbc5161168c542333633315dee1182227:key_store_seqid': ['400006'],
'cuyer': ['r'],
@@ -188,6 +183,30 @@ class CgiTests(unittest.TestCase):
self.assertEqual(expect[k], v)
self.assertItemsEqual(expect.values(), d.values())

+ def test_separator(self):
+ parse_semicolon = [
+ ("x=1;y=2.0", {'x': ['1'], 'y': ['2.0']}),
+ ("x=1;y=2.0;z=2-3.%2b0", {'x': ['1'], 'y': ['2.0'], 'z': ['2-3.+0']}),
+ (";", ValueError("bad query field: ''")),
+ (";;", ValueError("bad query field: ''")),
+ ("=;a", ValueError("bad query field: 'a'")),
+ (";b=a", ValueError("bad query field: ''")),
+ ("b;=a", ValueError("bad query field: 'b'")),
+ ("a=a+b;b=b+c", {'a': ['a b'], 'b': ['b c']}),
+ ("a=a+b;a=b+a", {'a': ['a b', 'b a']}),
+ ]
+ for orig, expect in parse_semicolon:
+ env = {'QUERY_STRING': orig}
+ fs = cgi.FieldStorage(separator=';', environ=env)
+ if isinstance(expect, dict):
+ for key in expect.keys():
+ expect_val = expect[key]
+ self.assertIn(key, fs)
+ if len(expect_val) > 1:
+ self.assertEqual(fs.getvalue(key), expect_val)
+ else:
+ self.assertEqual(fs.getvalue(key), expect_val[0])
+
def test_log(self):
cgi.log("Testing")

diff --git a/Lib/test/test_urlparse.py b/Lib/test/test_urlparse.py
index 86c4a0595c..0b2107339a 100644
--- a/Lib/test/test_urlparse.py
+++ b/Lib/test/test_urlparse.py
@@ -24,16 +24,20 @@ parse_qsl_test_cases = [
("&a=b", [('a', 'b')]),
("a=a+b&b=b+c", [('a', 'a b'), ('b', 'b c')]),
("a=1&a=2", [('a', '1'), ('a', '2')]),
- (";", []),
- (";;", []),
- (";a=b", [('a', 'b')]),
- ("a=a+b;b=b+c", [('a', 'a b'), ('b', 'b c')]),
- ("a=1;a=2", [('a', '1'), ('a', '2')]),
- (b";", []),
- (b";;", []),
- (b";a=b", [(b'a', b'b')]),
- (b"a=a+b;b=b+c", [(b'a', b'a b'), (b'b', b'b c')]),
- (b"a=1;a=2", [(b'a', b'1'), (b'a', b'2')]),
+ (b"", []),
+ (b"&", []),
+ (b"&&", []),
+ (b"=", [(b'', b'')]),
+ (b"=a", [(b'', b'a')]),
+ (b"a", [(b'a', b'')]),
+ (b"a=", [(b'a', b'')]),
+ (b"&a=b", [(b'a', b'b')]),
+ (b"a=a+b&b=b+c", [(b'a', b'a b'), (b'b', b'b c')]),
+ (b"a=1&a=2", [(b'a', b'1'), (b'a', b'2')]),
+ (";a=b", [(';a', 'b')]),
+ ("a=a+b;b=b+c", [('a', 'a b;b=b c')]),
+ (b";a=b", [(b';a', b'b')]),
+ (b"a=a+b;b=b+c", [(b'a', b'a b;b=b c')]),
]

parse_qs_test_cases = [
@@ -57,16 +61,10 @@ parse_qs_test_cases = [
(b"&a=b", {b'a': [b'b']}),
(b"a=a+b&b=b+c", {b'a': [b'a b'], b'b': [b'b c']}),
(b"a=1&a=2", {b'a': [b'1', b'2']}),
- (";", {}),
- (";;", {}),
- (";a=b", {'a': ['b']}),
- ("a=a+b;b=b+c", {'a': ['a b'], 'b': ['b c']}),
- ("a=1;a=2", {'a': ['1', '2']}),
- (b";", {}),
- (b";;", {}),
- (b";a=b", {b'a': [b'b']}),
- (b"a=a+b;b=b+c", {b'a': [b'a b'], b'b': [b'b c']}),
- (b"a=1;a=2", {b'a': [b'1', b'2']}),
+ (";a=b", {';a': ['b']}),
+ ("a=a+b;b=b+c", {'a': ['a b;b=b c']}),
+ (b";a=b", {b';a': [b'b']}),
+ (b"a=a+b;b=b+c", {b'a':[ b'a b;b=b c']}),
]

class UrlParseTestCase(unittest.TestCase):
diff --git a/Lib/urlparse.py b/Lib/urlparse.py
index 798b467b60..6c32727fce 100644
--- a/Lib/urlparse.py
+++ b/Lib/urlparse.py
@@ -382,7 +382,8 @@ def unquote(s):
append(item)
return ''.join(res)

-def parse_qs(qs, keep_blank_values=0, strict_parsing=0, max_num_fields=None):
+def parse_qs(qs, keep_blank_values=0, strict_parsing=0, max_num_fields=None,
+ separator='&'):
"""Parse a query given as a string argument.

Arguments:
@@ -402,17 +403,22 @@ def parse_qs(qs, keep_blank_values=0, strict_parsing=0, max_num_fields=None):

max_num_fields: int. If set, then throws a ValueError if there
are more than n fields read by parse_qsl().
+
+ separator: str. The symbol to use for separating the query arguments.
+ Defaults to &.
+
"""
dict = {}
for name, value in parse_qsl(qs, keep_blank_values, strict_parsing,
- max_num_fields):
+ max_num_fields, separator=separator):
if name in dict:
dict[name].append(value)
else:
dict[name] = [value]
return dict

-def parse_qsl(qs, keep_blank_values=0, strict_parsing=0, max_num_fields=None):
+def parse_qsl(qs, keep_blank_values=0, strict_parsing=0, max_num_fields=None,
+ separator='&'):
"""Parse a query given as a string argument.

Arguments:
@@ -432,17 +438,23 @@ def parse_qsl(qs, keep_blank_values=0, strict_parsing=0, max_num_fields=None):
max_num_fields: int. If set, then throws a ValueError if there
are more than n fields read by parse_qsl().

+ separator: str. The symbol to use for separating the query arguments.
+ Defaults to &.
+
Returns a list, as G-d intended.
"""
+ if not separator or (not isinstance(separator, (str, bytes))):
+ raise ValueError("Separator must be of type string or bytes.")
+
# If max_num_fields is defined then check that the number of fields
# is less than max_num_fields. This prevents a memory exhaustion DOS
# attack via post bodies with many fields.
if max_num_fields is not None:
- num_fields = 1 + qs.count('&') + qs.count(';')
+ num_fields = 1 + qs.count(separator)
if max_num_fields < num_fields:
raise ValueError('Max number of fields exceeded')

- pairs = [s2 for s1 in qs.split('&') for s2 in s1.split(';')]
+ pairs = [s1 for s1 in qs.split(separator)]
r = []
for name_value in pairs:
if not name_value and not strict_parsing:
diff --git a/Misc/NEWS.d/next/Security/2021-02-14-15-59-16.bpo-42967.YApqDS.rst b/Misc/NEWS.d/next/Security/2021-02-14-15-59-16.bpo-42967.YApqDS.rst
new file mode 100644
index 0000000000..f08489b414
--- /dev/null
+++ b/Misc/NEWS.d/next/Security/2021-02-14-15-59-16.bpo-42967.YApqDS.rst
@@ -0,0 +1 @@
+Fix web cache poisoning vulnerability by defaulting the query args separator to ``&``, and allowing the user to choose a custom separator.
--
2.31.1

2 changes: 2 additions & 0 deletions pkgs/development/interpreters/python/cpython/2.7/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ let

./CVE-2021-3177.patch

./CVE-2021-23336.patch

# The workaround is for unittests on Win64, which we don't support.
# It does break aarch64-darwin, which we do support. See:
# * https://bugs.python.org/issue35523
Expand Down