diff --git a/docs/faq.rst b/docs/faq.rst index 20dd814df..657802fd3 100644 --- a/docs/faq.rst +++ b/docs/faq.rst @@ -297,9 +297,13 @@ build the DOM of the entire feed in memory, and this can be quite slow and consume a lot of memory. In order to avoid parsing all the entire feed at once in memory, you can use -the functions ``xmliter`` and ``csviter`` from ``scrapy.utils.iterators`` -module. In fact, this is what the feed spiders (see :ref:`topics-spiders`) use -under the cover. +the :func:`~scrapy.utils.iterators.xmliter_lxml` and +:func:`~scrapy.utils.iterators.csviter` functions. In fact, this is what +:class:`~scrapy.spiders.XMLFeedSpider` uses. + +.. autofunction:: scrapy.utils.iterators.xmliter_lxml + +.. autofunction:: scrapy.utils.iterators.csviter Does Scrapy manage cookies automatically? ----------------------------------------- diff --git a/docs/news.rst b/docs/news.rst index ad931d6db..16e7e79a7 100644 --- a/docs/news.rst +++ b/docs/news.rst @@ -19,6 +19,25 @@ Highlights: Security bug fixes ~~~~~~~~~~~~~~~~~~ +- Addressed `ReDoS vulnerabilities`_: + + - ``scrapy.utils.iterators.xmliter`` is now deprecated in favor of + :func:`~scrapy.utils.iterators.xmliter_lxml`, which + :class:`~scrapy.spiders.XMLFeedSpider` now uses. + + To minimize the impact of this change on existing code, + :func:`~scrapy.utils.iterators.xmliter_lxml` now supports indicating + the node namespace with a prefix in the node name, and big files with + highly nested trees when using libxml2 2.7+. + + - Fixed regular expressions in the implementation of the + :func:`~scrapy.utils.response.open_in_browser` function. + + Please, see the `cc65-xxvf-f7r9 security advisory`_ for more information. + + .. _ReDoS vulnerabilities: https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS + .. _cc65-xxvf-f7r9 security advisory: https://github.com/scrapy/scrapy/security/advisories/GHSA-cc65-xxvf-f7r9 + - :setting:`DOWNLOAD_MAXSIZE` and :setting:`DOWNLOAD_WARNSIZE` now also apply to the decompressed response body. Please, see the `7j7m-v7m3-jqm7 security advisory`_ for more information. @@ -2951,7 +2970,6 @@ affect subclasses: (:issue:`3884`) - .. _release-1.8.4: Scrapy 1.8.4 (unreleased) @@ -2959,6 +2977,17 @@ Scrapy 1.8.4 (unreleased) **Security bug fixes:** +- Due to its `ReDoS vulnerabilities`_, ``scrapy.utils.iterators.xmliter`` is + now deprecated in favor of :func:`~scrapy.utils.iterators.xmliter_lxml`, + which :class:`~scrapy.spiders.XMLFeedSpider` now uses. + + To minimize the impact of this change on existing code, + :func:`~scrapy.utils.iterators.xmliter_lxml` now supports indicating + the node namespace as a prefix in the node name, and big files with highly + nested trees when using libxml2 2.7+. + + Please, see the `cc65-xxvf-f7r9 security advisory`_ for more information. + - :setting:`DOWNLOAD_MAXSIZE` and :setting:`DOWNLOAD_WARNSIZE` now also apply to the decompressed response body. Please, see the `7j7m-v7m3-jqm7 security advisory`_ for more information. diff --git a/docs/topics/debug.rst b/docs/topics/debug.rst index 49c5b0410..988e37bbd 100644 --- a/docs/topics/debug.rst +++ b/docs/topics/debug.rst @@ -125,25 +125,15 @@ Fortunately, the :command:`shell` is your bread and butter in this case (see See also: :ref:`topics-shell-inspect-response`. + Open in browser =============== Sometimes you just want to see how a certain response looks in a browser, you -can use the ``open_in_browser`` function for that. Here is an example of how -you would use it: +can use the :func:`~scrapy.utils.response.open_in_browser` function for that: -.. code-block:: python +.. autofunction:: scrapy.utils.response.open_in_browser - from scrapy.utils.response import open_in_browser - - - def parse_details(self, response): - if "item name" not in response.body: - open_in_browser(response) - -``open_in_browser`` will open a browser with the response received by Scrapy at -that point, adjusting the `base tag`_ so that images and styles are displayed -properly. Logging ======= @@ -163,8 +153,6 @@ available in all future runs should they be necessary again: For more information, check the :ref:`topics-logging` section. -.. _base tag: https://www.w3schools.com/tags/tag_base.asp - .. _debug-vscode: Visual Studio Code diff --git a/scrapy/spiders/feed.py b/scrapy/spiders/feed.py index 6afadc577..42675c76a 100644 --- a/scrapy/spiders/feed.py +++ b/scrapy/spiders/feed.py @@ -7,7 +7,7 @@ See documentation in docs/topics/spiders.rst from scrapy.exceptions import NotConfigured, NotSupported from scrapy.selector import Selector from scrapy.spiders import Spider -from scrapy.utils.iterators import csviter, xmliter +from scrapy.utils.iterators import csviter, xmliter_lxml from scrapy.utils.spider import iterate_spider_output @@ -84,7 +84,7 @@ class XMLFeedSpider(Spider): return self.parse_nodes(response, nodes) def _iternodes(self, response): - for node in xmliter(response, self.itertag): + for node in xmliter_lxml(response, self.itertag): self._register_namespaces(node) yield node diff --git a/scrapy/utils/iterators.py b/scrapy/utils/iterators.py index 03d779afb..ab48e525f 100644 --- a/scrapy/utils/iterators.py +++ b/scrapy/utils/iterators.py @@ -16,7 +16,11 @@ from typing import ( cast, overload, ) +from warnings import warn +from lxml import etree + +from scrapy.exceptions import ScrapyDeprecationWarning from scrapy.http import Response, TextResponse from scrapy.selector import Selector from scrapy.utils.python import re_rsearch, to_unicode @@ -38,6 +42,16 @@ def xmliter( - a unicode string - a string encoded as utf-8 """ + warn( + ( + "xmliter is deprecated and its use strongly discouraged because " + "it is vulnerable to ReDoS attacks. Use xmliter_lxml instead. See " + "https://github.com/scrapy/scrapy/security/advisories/GHSA-cc65-xxvf-f7r9" + ), + ScrapyDeprecationWarning, + stacklevel=2, + ) + nodename_patt = re.escape(nodename) DOCUMENT_HEADER_RE = re.compile(r"<\?xml[^>]+>\s*", re.S) @@ -81,15 +95,34 @@ def xmliter_lxml( namespace: Optional[str] = None, prefix: str = "x", ) -> Generator[Selector, Any, None]: - from lxml import etree - reader = _StreamReader(obj) tag = f"{{{namespace}}}{nodename}" if namespace else nodename iterable = etree.iterparse( - cast("SupportsReadClose[bytes]", reader), tag=tag, encoding=reader.encoding + cast("SupportsReadClose[bytes]", reader), + encoding=reader.encoding, + events=("end", "start-ns"), + huge_tree=True, ) selxpath = "//" + (f"{prefix}:{nodename}" if namespace else nodename) - for _, node in iterable: + needs_namespace_resolution = not namespace and ":" in nodename + if needs_namespace_resolution: + prefix, nodename = nodename.split(":", maxsplit=1) + for event, data in iterable: + if event == "start-ns": + assert isinstance(data, tuple) + if needs_namespace_resolution: + _prefix, _namespace = data + if _prefix != prefix: + continue + namespace = _namespace + needs_namespace_resolution = False + selxpath = f"//{prefix}:{nodename}" + tag = f"{{{namespace}}}{nodename}" + continue + assert isinstance(data, etree._Element) + node = data + if node.tag != tag: + continue nodetext = etree.tostring(node, encoding="unicode") node.clear() xs = Selector(text=nodetext, type="xml") diff --git a/scrapy/utils/response.py b/scrapy/utils/response.py index c540d6278..fabfb1167 100644 --- a/scrapy/utils/response.py +++ b/scrapy/utils/response.py @@ -74,6 +74,18 @@ def response_httprepr(response: Response) -> bytes: return b"".join(values) +def _remove_html_comments(body): + start = body.find(b"", start + 1) + if end == -1: + return body[:start] + else: + body = body[:start] + body[end + 3 :] + start = body.find(b"", b"", body, flags=re.DOTALL) - body = re.sub(rb"(|\s.*?>))", to_bytes(repl), body) + _remove_html_comments(body) + repl = rf'\0' + body = re.sub(rb"]*?>)", to_bytes(repl), body, count=1) ext = ".html" elif isinstance(response, TextResponse): ext = ".txt" diff --git a/tests/test_spider.py b/tests/test_spider.py index 3f595cc93..3653e2d73 100644 --- a/tests/test_spider.py +++ b/tests/test_spider.py @@ -151,10 +151,10 @@ class XMLFeedSpiderTest(SpiderTest): body = b""" - http://www.example.com/Special-Offers.html2009-08-16 + http://www.example.com/Special-Offers.html2009-08-16 - http://www.example.com/2009-08-16 + http://www.example.com/2009-08-16 """ response = XmlResponse(url="http://example.com/sitemap.xml", body=body) diff --git a/tests/test_utils_iterators.py b/tests/test_utils_iterators.py index 3598fa0bb..505cc276c 100644 --- a/tests/test_utils_iterators.py +++ b/tests/test_utils_iterators.py @@ -1,14 +1,14 @@ -from pytest import mark +import pytest from twisted.trial import unittest +from scrapy.exceptions import ScrapyDeprecationWarning from scrapy.http import Response, TextResponse, XmlResponse from scrapy.utils.iterators import _body_or_str, csviter, xmliter, xmliter_lxml from tests import get_testdata -class XmliterTestCase(unittest.TestCase): - xmliter = staticmethod(xmliter) - +class XmliterBaseTestCase: + @pytest.mark.filterwarnings("ignore::scrapy.exceptions.ScrapyDeprecationWarning") def test_xmliter(self): body = b""" @@ -40,6 +40,7 @@ class XmliterTestCase(unittest.TestCase): attrs, [("001", ["Name 1"], ["Type 1"]), ("002", ["Name 2"], ["Type 2"])] ) + @pytest.mark.filterwarnings("ignore::scrapy.exceptions.ScrapyDeprecationWarning") def test_xmliter_unusual_node(self): body = b""" @@ -53,6 +54,7 @@ class XmliterTestCase(unittest.TestCase): ] self.assertEqual(nodenames, [["matchme..."]]) + @pytest.mark.filterwarnings("ignore::scrapy.exceptions.ScrapyDeprecationWarning") def test_xmliter_unicode(self): # example taken from https://github.com/scrapy/scrapy/issues/1665 body = """ @@ -112,6 +114,7 @@ class XmliterTestCase(unittest.TestCase): [("26", ["-"], ["80"]), ("21", ["Ab"], ["76"]), ("27", ["A"], ["27"])], ) + @pytest.mark.filterwarnings("ignore::scrapy.exceptions.ScrapyDeprecationWarning") def test_xmliter_text(self): body = ( '' @@ -123,6 +126,7 @@ class XmliterTestCase(unittest.TestCase): [["one"], ["two"]], ) + @pytest.mark.filterwarnings("ignore::scrapy.exceptions.ScrapyDeprecationWarning") def test_xmliter_namespaces(self): body = b""" @@ -162,6 +166,7 @@ class XmliterTestCase(unittest.TestCase): self.assertEqual(node.xpath("id/text()").getall(), []) self.assertEqual(node.xpath("price/text()").getall(), []) + @pytest.mark.filterwarnings("ignore::scrapy.exceptions.ScrapyDeprecationWarning") def test_xmliter_namespaced_nodename(self): body = b""" @@ -190,6 +195,7 @@ class XmliterTestCase(unittest.TestCase): ["http://www.mydummycompany.com/images/item1.jpg"], ) + @pytest.mark.filterwarnings("ignore::scrapy.exceptions.ScrapyDeprecationWarning") def test_xmliter_namespaced_nodename_missing(self): body = b""" @@ -214,6 +220,7 @@ class XmliterTestCase(unittest.TestCase): with self.assertRaises(StopIteration): next(my_iter) + @pytest.mark.filterwarnings("ignore::scrapy.exceptions.ScrapyDeprecationWarning") def test_xmliter_exception(self): body = ( '' @@ -226,10 +233,12 @@ class XmliterTestCase(unittest.TestCase): self.assertRaises(StopIteration, next, iter) + @pytest.mark.filterwarnings("ignore::scrapy.exceptions.ScrapyDeprecationWarning") def test_xmliter_objtype_exception(self): i = self.xmliter(42, "product") self.assertRaises(TypeError, next, i) + @pytest.mark.filterwarnings("ignore::scrapy.exceptions.ScrapyDeprecationWarning") def test_xmliter_encoding(self): body = ( b'\n' @@ -244,12 +253,25 @@ class XmliterTestCase(unittest.TestCase): ) -class LxmlXmliterTestCase(XmliterTestCase): - xmliter = staticmethod(xmliter_lxml) +class XmliterTestCase(XmliterBaseTestCase, unittest.TestCase): + xmliter = staticmethod(xmliter) - @mark.xfail(reason="known bug of the current implementation") - def test_xmliter_namespaced_nodename(self): - super().test_xmliter_namespaced_nodename() + def test_deprecation(self): + body = b""" + + + + + """ + with pytest.warns( + ScrapyDeprecationWarning, + match="xmliter", + ): + next(self.xmliter(body, "product")) + + +class LxmlXmliterTestCase(XmliterBaseTestCase, unittest.TestCase): + xmliter = staticmethod(xmliter_lxml) def test_xmliter_iterate_namespace(self): body = b""" diff --git a/tests/test_utils_response.py b/tests/test_utils_response.py index 80e15a60f..db3c31b89 100644 --- a/tests/test_utils_response.py +++ b/tests/test_utils_response.py @@ -1,12 +1,16 @@ import unittest import warnings from pathlib import Path +from time import process_time from urllib.parse import urlparse +import pytest + from scrapy.exceptions import ScrapyDeprecationWarning from scrapy.http import HtmlResponse, Response, TextResponse from scrapy.utils.python import to_bytes from scrapy.utils.response import ( + _remove_html_comments, get_base_url, get_meta_refresh, open_in_browser, @@ -198,3 +202,76 @@ class ResponseUtilsTest(unittest.TestCase): assert open_in_browser( r5, _openfunc=check_base_url ), "Inject unique base url with conditional comment" + + def test_open_in_browser_redos_comment(self): + MAX_CPU_TIME = 0.001 + + # Exploit input from + # https://makenowjust-labs.github.io/recheck/playground/ + # for // (old pattern to remove comments). + body = b"->" + + response = HtmlResponse("https://example.com", body=body) + + start_time = process_time() + + open_in_browser(response, lambda url: True) + + end_time = process_time() + self.assertLess(end_time - start_time, MAX_CPU_TIME) + + def test_open_in_browser_redos_head(self): + MAX_CPU_TIME = 0.001 + + # Exploit input from + # https://makenowjust-labs.github.io/recheck/playground/ + # for /(|\s.*?>))/ (old pattern to find the head element). + body = b"b", + b"ab", + ), + ( + b"ac", + b"ac", + ), + ( + b"acccd", + b"acd", + ), + ( + b"ad", + b"ad", + ), + ), +) +def test_remove_html_comments(input_body, output_body): + assert ( + _remove_html_comments(input_body) == output_body + ), f"{_remove_html_comments(input_body)=} == {output_body=}"