From 1c3d28979852aa8370ffdb9187f5864a5dfb2b1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Chaves?= Date: Wed, 22 Nov 2023 15:52:00 +0100 Subject: [PATCH 1/4] Protect against compression bombs --- docs/news.rst | 12 + docs/topics/request-response.rst | 31 +- docs/topics/settings.rst | 36 +- .../downloadermiddlewares/httpcompression.py | 117 +++++-- scrapy/spiders/sitemap.py | 45 ++- scrapy/utils/_compression.py | 100 ++++++ scrapy/utils/gz.py | 23 +- tests/sample_data/compressed/bomb-br.bin | 2 + tests/sample_data/compressed/bomb-deflate.bin | Bin 0 -> 27968 bytes tests/sample_data/compressed/bomb-gzip.bin | Bin 0 -> 27988 bytes tests/sample_data/compressed/bomb-zstd.bin | Bin 0 -> 1096 bytes tests/test_downloader_handlers.py | 9 +- ...st_downloadermiddleware_httpcompression.py | 317 ++++++++++++++++-- tests/test_spider.py | 151 ++++++++- 14 files changed, 734 insertions(+), 109 deletions(-) create mode 100644 scrapy/utils/_compression.py create mode 100644 tests/sample_data/compressed/bomb-br.bin create mode 100644 tests/sample_data/compressed/bomb-deflate.bin create mode 100644 tests/sample_data/compressed/bomb-gzip.bin create mode 100644 tests/sample_data/compressed/bomb-zstd.bin diff --git a/docs/news.rst b/docs/news.rst index 4c4110306..4de283ddf 100644 --- a/docs/news.rst +++ b/docs/news.rst @@ -6,6 +6,18 @@ Release notes .. note:: Scrapy 1.x is the last series supporting Python 2. Scrapy 2.x supports **Python 3 only**. +.. _release-1.8.4: + +Scrapy 1.8.4 (unreleased) +------------------------- + +**Security bug fix:** + +- :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. + + .. _release-1.8.3: Scrapy 1.8.3 (2022-07-25) diff --git a/docs/topics/request-response.rst b/docs/topics/request-response.rst index 727c67482..d0f8381c3 100644 --- a/docs/topics/request-response.rst +++ b/docs/topics/request-response.rst @@ -318,26 +318,27 @@ are some special keys recognized by Scrapy and its built-in extensions. Those are: -* :reqmeta:`dont_redirect` -* :reqmeta:`dont_retry` -* :reqmeta:`handle_httpstatus_list` -* :reqmeta:`handle_httpstatus_all` -* :reqmeta:`dont_merge_cookies` +* :reqmeta:`bindaddress` * :reqmeta:`cookiejar` * :reqmeta:`dont_cache` +* :reqmeta:`dont_merge_cookies` +* :reqmeta:`dont_obey_robotstxt` +* :reqmeta:`dont_redirect` +* :reqmeta:`dont_retry` +* :reqmeta:`download_fail_on_dataloss` +* :reqmeta:`download_latency` +* :reqmeta:`download_maxsize` +* :reqmeta:`download_warnsize` +* :reqmeta:`download_timeout` +* ``ftp_password`` (See :setting:`FTP_PASSWORD` for more info) +* ``ftp_user`` (See :setting:`FTP_USER` for more info) +* :reqmeta:`handle_httpstatus_all` +* :reqmeta:`handle_httpstatus_list` +* :reqmeta:`max_retry_times` +* :reqmeta:`proxy` * :reqmeta:`redirect_reasons` * :reqmeta:`redirect_urls` -* :reqmeta:`bindaddress` -* :reqmeta:`dont_obey_robotstxt` -* :reqmeta:`download_timeout` -* :reqmeta:`download_maxsize` -* :reqmeta:`download_latency` -* :reqmeta:`download_fail_on_dataloss` -* :reqmeta:`proxy` -* ``ftp_user`` (See :setting:`FTP_USER` for more info) -* ``ftp_password`` (See :setting:`FTP_PASSWORD` for more info) * :reqmeta:`referrer_policy` -* :reqmeta:`max_retry_times` .. reqmeta:: bindaddress diff --git a/docs/topics/settings.rst b/docs/topics/settings.rst index 4374d8042..cfbc0193c 100644 --- a/docs/topics/settings.rst +++ b/docs/topics/settings.rst @@ -632,42 +632,44 @@ The amount of time (in secs) that the downloader will wait before timing out. Request.meta key. .. setting:: DOWNLOAD_MAXSIZE +.. reqmeta:: download_maxsize DOWNLOAD_MAXSIZE ---------------- -Default: ``1073741824`` (1024MB) +Default: ``1073741824`` (1 GiB) -The maximum response size (in bytes) that downloader will download. +The maximum response body size (in bytes) allowed. Bigger responses are +aborted and ignored. -If you want to disable it set to 0. +This applies both before and after compression. If decompressing a response +body would exceed this limit, decompression is aborted and the response is +ignored. -.. reqmeta:: download_maxsize +Use ``0`` to disable this limit. -.. note:: - - This size can be set per spider using :attr:`download_maxsize` - spider attribute and per-request using :reqmeta:`download_maxsize` - Request.meta key. +This limit can be set per spider using the :attr:`download_maxsize` spider +attribute and per request using the :reqmeta:`download_maxsize` Request.meta +key. This feature needs Twisted >= 11.1. .. setting:: DOWNLOAD_WARNSIZE +.. reqmeta:: download_warnsize DOWNLOAD_WARNSIZE ----------------- -Default: ``33554432`` (32MB) +Default: ``33554432`` (32 MiB) -The response size (in bytes) that downloader will start to warn. +If the size of a response exceeds this value, before or after compression, a +warning will be logged about it. -If you want to disable it set to 0. +Use ``0`` to disable this limit. -.. note:: - - This size can be set per spider using :attr:`download_warnsize` - spider attribute and per-request using :reqmeta:`download_warnsize` - Request.meta key. +This limit can be set per spider using the :attr:`download_warnsize` spider +attribute and per request using the :reqmeta:`download_warnsize` Request.meta +key. This feature needs Twisted >= 11.1. diff --git a/scrapy/downloadermiddlewares/httpcompression.py b/scrapy/downloadermiddlewares/httpcompression.py index 203dee42d..6db817ffe 100644 --- a/scrapy/downloadermiddlewares/httpcompression.py +++ b/scrapy/downloadermiddlewares/httpcompression.py @@ -1,28 +1,73 @@ -import zlib +import warnings +from logging import getLogger -from scrapy.utils.gz import gunzip +from scrapy import signals +from scrapy.exceptions import IgnoreRequest, NotConfigured from scrapy.http import Response, TextResponse from scrapy.responsetypes import responsetypes -from scrapy.exceptions import NotConfigured +from scrapy.utils._compression import ( + _DecompressionMaxSizeExceeded, + _inflate, + _unbrotli, + _unzstd, +) +from scrapy.utils.deprecate import ScrapyDeprecationWarning +from scrapy.utils.gz import gunzip +logger = getLogger(__name__) -ACCEPTED_ENCODINGS = [b'gzip', b'deflate'] +ACCEPTED_ENCODINGS = [b"gzip", b"deflate"] try: - import brotli - ACCEPTED_ENCODINGS.append(b'br') + import brotli # noqa: F401 except ImportError: pass +else: + ACCEPTED_ENCODINGS.append(b"br") + +try: + import zstandard # noqa: F401 +except ImportError: + pass +else: + ACCEPTED_ENCODINGS.append(b"zstd") class HttpCompressionMiddleware(object): """This middleware allows compressed (gzip, deflate) traffic to be sent/received from web sites""" + + def __init__(self, crawler=None): + if not crawler: + return + self._max_size = crawler.settings.getint("DOWNLOAD_MAXSIZE") + self._warn_size = crawler.settings.getint("DOWNLOAD_WARNSIZE") + crawler.signals.connect(self.open_spider, signals.spider_opened) + @classmethod def from_crawler(cls, crawler): if not crawler.settings.getbool('COMPRESSION_ENABLED'): raise NotConfigured - return cls() + try: + return cls(crawler=crawler) + except TypeError: + warnings.warn( + "HttpCompressionMiddleware subclasses must either modify " + "their '__init__' method to support a 'crawler' parameter or " + "reimplement their 'from_crawler' method.", + ScrapyDeprecationWarning, + ) + spider = cls() + spider._max_size = crawler.settings.getint("DOWNLOAD_MAXSIZE") + spider._warn_size = crawler.settings.getint("DOWNLOAD_WARNSIZE") + crawler.signals.connect(spider.open_spider, signals.spider_opened) + return spider + + def open_spider(self, spider): + if hasattr(spider, "download_maxsize"): + self._max_size = spider.download_maxsize + if hasattr(spider, "download_warnsize"): + self._warn_size = spider.download_warnsize def process_request(self, request, spider): request.headers.setdefault('Accept-Encoding', @@ -36,9 +81,36 @@ class HttpCompressionMiddleware(object): content_encoding = response.headers.getlist('Content-Encoding') if content_encoding: encoding = content_encoding.pop() - decoded_body = self._decode(response.body, encoding.lower()) - respcls = responsetypes.from_args(headers=response.headers, \ - url=response.url, body=decoded_body) + max_size = request.meta.get("download_maxsize", self._max_size) + warn_size = request.meta.get("download_warnsize", self._warn_size) + try: + decoded_body = self._decode( + response.body, encoding.lower(), max_size + ) + except _DecompressionMaxSizeExceeded: + raise IgnoreRequest( + "Ignored response {response} because its body " + "({body_size} B) exceeded DOWNLOAD_MAXSIZE " + "({max_size} B) during decompression.".format( + response=response, + body_size=len(response.body), + max_size=max_size, + ) + ) + if len(response.body) < warn_size <= len(decoded_body): + logger.warning( + "%(response)s body size after decompression " + "(%(body_size)s B) is larger than the " + "download warning size (%(warn_size)s B).", + { + "response": response, + "body_size": len(decoded_body), + "warn_size": warn_size, + }, + ) + respcls = responsetypes.from_args( + headers=response.headers, url=response.url, body=decoded_body + ) kwargs = dict(cls=respcls, body=decoded_body) if issubclass(respcls, TextResponse): # force recalculating the encoding until we make sure the @@ -50,20 +122,13 @@ class HttpCompressionMiddleware(object): return response - def _decode(self, body, encoding): - if encoding == b'gzip' or encoding == b'x-gzip': - body = gunzip(body) - - if encoding == b'deflate': - try: - body = zlib.decompress(body) - except zlib.error: - # ugly hack to work with raw deflate content that may - # be sent by microsoft servers. For more information, see: - # http://carsten.codimi.de/gzip.yaws/ - # http://www.port80software.com/200ok/archive/2005/10/31/868.aspx - # http://www.gzip.org/zlib/zlib_faq.html#faq38 - body = zlib.decompress(body, -15) - if encoding == b'br' and b'br' in ACCEPTED_ENCODINGS: - body = brotli.decompress(body) + def _decode(self, body, encoding, max_size): + if encoding == b"gzip" or encoding == b"x-gzip": + return gunzip(body, max_size=max_size) + if encoding == b"deflate": + return _inflate(body, max_size=max_size) + if encoding == b"br" and b"br" in ACCEPTED_ENCODINGS: + return _unbrotli(body, max_size=max_size) + if encoding == b"zstd" and b"zstd" in ACCEPTED_ENCODINGS: + return _unzstd(body, max_size=max_size) return body diff --git a/scrapy/spiders/sitemap.py b/scrapy/spiders/sitemap.py index 534c45c70..e27131f61 100644 --- a/scrapy/spiders/sitemap.py +++ b/scrapy/spiders/sitemap.py @@ -1,12 +1,13 @@ -import re import logging +import re + import six -from scrapy.spiders import Spider -from scrapy.http import Request, XmlResponse -from scrapy.utils.sitemap import Sitemap, sitemap_urls_from_robots +from scrapy.http.response.xml import XmlResponse +from scrapy.spiders import Request, Spider +from scrapy.utils._compression import _DecompressionMaxSizeExceeded from scrapy.utils.gz import gunzip, gzip_magic_number - +from scrapy.utils.sitemap import Sitemap, sitemap_urls_from_robots logger = logging.getLogger(__name__) @@ -18,6 +19,17 @@ class SitemapSpider(Spider): sitemap_follow = [''] sitemap_alternate_links = False + @classmethod + def from_crawler(cls, crawler, *args, **kwargs): + spider = super(SitemapSpider, cls).from_crawler(crawler, *args, **kwargs) + spider._max_size = getattr( + spider, "download_maxsize", spider.settings.getint("DOWNLOAD_MAXSIZE") + ) + spider._warn_size = getattr( + spider, "download_warnsize", spider.settings.getint("DOWNLOAD_WARNSIZE") + ) + return spider + def __init__(self, *a, **kw): super(SitemapSpider, self).__init__(*a, **kw) self._cbs = [] @@ -70,8 +82,25 @@ class SitemapSpider(Spider): """ if isinstance(response, XmlResponse): return response.body - elif gzip_magic_number(response): - return gunzip(response.body) + if gzip_magic_number(response): + uncompressed_size = len(response.body) + max_size = response.meta.get("download_maxsize", self._max_size) + warn_size = response.meta.get("download_warnsize", self._warn_size) + try: + body = gunzip(response.body, max_size=max_size) + except _DecompressionMaxSizeExceeded: + return None + if uncompressed_size < warn_size <= len(body): + logger.warning( + "%(response)s body size after decompression (%(body_length)s B) " + "is larger than the download warning size (%(warn_size)s B).", + { + "response": response, + "body_length": len(body), + "warn_size": warn_size, + }, + ) + return body # actual gzipped sitemap files are decompressed above ; # if we are here (response body is not gzipped) # and have a response for .xml.gz, @@ -81,7 +110,7 @@ class SitemapSpider(Spider): # without actually being a .xml.gz file in the first place, # merely XML gzip-compressed on the fly, # in other word, here, we have plain XML - elif response.url.endswith('.xml') or response.url.endswith('.xml.gz'): + if response.url.endswith('.xml') or response.url.endswith('.xml.gz'): return response.body diff --git a/scrapy/utils/_compression.py b/scrapy/utils/_compression.py new file mode 100644 index 000000000..2260c55a8 --- /dev/null +++ b/scrapy/utils/_compression.py @@ -0,0 +1,100 @@ +import zlib +from io import BytesIO + +try: + import brotli +except ImportError: + pass + +try: + import zstandard +except ImportError: + pass + + +class _DecompressionMaxSizeExceeded(ValueError): + pass + + +def _inflate(data, max_size=0): + decompressor = zlib.decompressobj() + raw_decompressor = zlib.decompressobj(-15) + input_stream = BytesIO(data) + output_list = [] + output_chunk = b"." + decompressed_size = 0 + CHUNK_SIZE = 8196 + while output_chunk: + input_chunk = input_stream.read(CHUNK_SIZE) + try: + output_chunk = decompressor.decompress(input_chunk) + except zlib.error: + if decompressor != raw_decompressor: + # ugly hack to work with raw deflate content that may + # be sent by microsoft servers. For more information, see: + # http://carsten.codimi.de/gzip.yaws/ + # http://www.port80software.com/200ok/archive/2005/10/31/868.aspx + # http://www.gzip.org/zlib/zlib_faq.html#faq38 + decompressor = raw_decompressor + output_chunk = decompressor.decompress(input_chunk) + else: + raise + decompressed_size += len(output_chunk) + if max_size and decompressed_size > max_size: + raise _DecompressionMaxSizeExceeded( + "The number of bytes decompressed so far " + "({decompressed_size} B) exceed the specified maximum " + "({max_size} B).".format( + decompressed_size=decompressed_size, + max_size=max_size, + ) + ) + output_list.append(output_chunk) + return b"".join(output_list) + + +def _unbrotli(data, max_size=0): + decompressor = brotli.Decompressor() + input_stream = BytesIO(data) + output_list = [] + output_chunk = b"." + decompressed_size = 0 + CHUNK_SIZE = 8196 + while output_chunk: + input_chunk = input_stream.read(CHUNK_SIZE) + output_chunk = decompressor.decompress(input_chunk) + decompressed_size += len(output_chunk) + if max_size and decompressed_size > max_size: + raise _DecompressionMaxSizeExceeded( + "The number of bytes decompressed so far " + "({decompressed_size} B) exceed the specified maximum " + "({max_size} B).".format( + decompressed_size=decompressed_size, + max_size=max_size, + ) + ) + output_list.append(output_chunk) + return b"".join(output_list) + + +def _unzstd(data, max_size=0): + decompressor = zstandard.ZstdDecompressor() + stream_reader = decompressor.stream_reader(BytesIO(data)) + output_list = [] + output_chunk = b"." + decompressed_size = 0 + CHUNK_SIZE = 8196 + while output_chunk: + output_chunk = stream_reader.read(CHUNK_SIZE) + decompressed_size += len(output_chunk) + if max_size and decompressed_size > max_size: + raise _DecompressionMaxSizeExceeded( + "The number of bytes decompressed so far " + "({decompressed_size} B) exceed the specified maximum " + "({max_size} B).".format( + decompressed_size=decompressed_size, + max_size=max_size, + ) + ) + output_list.append(output_chunk) + return b"".join(output_list) diff --git a/scrapy/utils/gz.py b/scrapy/utils/gz.py index b3fb16b1e..9f28cfb77 100644 --- a/scrapy/utils/gz.py +++ b/scrapy/utils/gz.py @@ -4,13 +4,15 @@ try: from cStringIO import StringIO as BytesIO except ImportError: from io import BytesIO + +import re from gzip import GzipFile import six -import re from scrapy.utils.decorators import deprecated +from ._compression import _DecompressionMaxSizeExceeded # - Python>=3.5 GzipFile's read() has issues returning leftover # uncompressed data when input is corrupted @@ -27,18 +29,18 @@ else: return gzf.read1(size) -def gunzip(data): +def gunzip(data, max_size=0): """Gunzip the given data and return as much data as possible. This is resilient to CRC checksum errors. """ f = GzipFile(fileobj=BytesIO(data)) output_list = [] - chunk = b'.' + chunk = b"." + decompressed_size = 0 while chunk: try: chunk = read1(f, 8196) - output_list.append(chunk) except (IOError, EOFError, struct.error): # complete only if there is some data, otherwise re-raise # see issue 87 about catching struct.error @@ -51,7 +53,18 @@ def gunzip(data): break else: raise - return b''.join(output_list) + decompressed_size += len(chunk) + if max_size and decompressed_size > max_size: + raise _DecompressionMaxSizeExceeded( + "The number of bytes decompressed so far " + "({decompressed_size} B) exceed the specified maximum " + "({max_size} B).".format( + decompressed_size=decompressed_size, + max_size=max_size, + ) + ) + output_list.append(chunk) + return b"".join(output_list) _is_gzipped = re.compile(br'^application/(x-)?gzip\b', re.I).search _is_octetstream = re.compile(br'^(application|binary)/octet-stream\b', re.I).search diff --git a/tests/sample_data/compressed/bomb-br.bin b/tests/sample_data/compressed/bomb-br.bin new file mode 100644 index 000000000..50059866f --- /dev/null +++ b/tests/sample_data/compressed/bomb-br.bin @@ -0,0 +1,2 @@ +;nުVp SmoY2 +()-д=_o \ No newline at end of file diff --git a/tests/sample_data/compressed/bomb-deflate.bin b/tests/sample_data/compressed/bomb-deflate.bin new file mode 100644 index 0000000000000000000000000000000000000000..3598aca0777ec2511a721e9655cabb8c21c658bd GIT binary patch literal 27968 zcmeI&u}VS#6b9f+=-?6}`2-Gb=^8GEdrPzhZ7q%$M35jzaB^}JadC@=dVsh@OD>AI zMF>rSC{CdeWcdhg3f~#dd^nu*O@FmB>%Sy!^U2^bI{%2BjpGkTvtGCQb9b0}%gx*A zC^NVm7VfVnqwxEtJUIF4gqj_=18;x=5|WUFBqSjTNk~Exl8}TXBq0e&NJ0{lkc1>8 zAqh!HLK2dYgd`*(2}wvo5|VI-C0ssb8?oTOioa2%K7C$JY75N{+<`Yh0SQS+LK2dY zgd`*(2}wvo5|WUFBqSjTNk~Exl8}TXBq0e&NJ0{lkc1>8Aqh#iPZGYjN(Y-T;OY9R z_8O#jIJamt;d0?};d0?}5|WUFBqSjTNk~Exl8}TXBq0e&NJ0{lkc1>8Aqh#im4waX I@bhBz2V-bE-2eap literal 0 HcmV?d00001 diff --git a/tests/sample_data/compressed/bomb-gzip.bin b/tests/sample_data/compressed/bomb-gzip.bin new file mode 100644 index 0000000000000000000000000000000000000000..64aa0c3696cc1c6d86d218c70635d88f2035ec9e GIT binary patch literal 27988 zcmeIyElY!86b9f&oiK|G(Y#;~27Xl0-yq1UE0YN_<|{r6TM$G+ga1HfWkC=@i}}T- zAQQ0;tA;J=f*|@M2A1oDJDzYj_mw}*X4m_rN*LQrYP)-t7`Kz1`EpV#FVq|L(0ja} zI9SSs+qBrtti6t3Pxsob#`n?W)Wc%`Y$l!UY&@@AZN0t3&;4p=`TZgaH}D5)fC3Vd zkc1>8Aqh!HLK2dYgd`*(2}wvo5|WUFBqSjTNk~Exl8}TXBq0e&NJ0{lkc1>8Aqh!H zLK2dYgd`*(2}wvo5|WUFBqSjTNk~Exl8}TXBq0e&NJ0{lkc1>8Aqh!HLK2d2h!PI& z=1wx8 S;eSfl{TO{ZZ^qTjoA3)j<4WiN literal 0 HcmV?d00001 diff --git a/tests/sample_data/compressed/bomb-zstd.bin b/tests/sample_data/compressed/bomb-zstd.bin new file mode 100644 index 0000000000000000000000000000000000000000..4b0efa8a41c88a38dffe7cea9e4a6726bdb137c4 GIT binary patch literal 1096 zcmdPcs{gko!e;q;1{Fqz2O$}m#R@=_sF0kWTTql*T%4Jor;wDNo219Z$k6h?-fpfB z0|SQwBg3EnmI6#5b|Mlx7l~br#Lh!vBdZBP5+5~lG&~1uT5@4vU|?kU`+vT_!f29* VWPM*?)(2)~i{-##pxebr9RRD(6-595 literal 0 HcmV?d00001 diff --git a/tests/test_downloader_handlers.py b/tests/test_downloader_handlers.py index 109469503..1f42c77c3 100644 --- a/tests/test_downloader_handlers.py +++ b/tests/test_downloader_handlers.py @@ -37,7 +37,7 @@ from scrapy.responsetypes import responsetypes from scrapy.settings import Settings from scrapy.utils.test import get_crawler, skip_if_no_boto from scrapy.utils.python import to_bytes -from scrapy.exceptions import NotConfigured +from scrapy.exceptions import IgnoreRequest, NotConfigured from tests.mockserver import MockServer, ssl_context_factory, Echo from tests.spiders import SingleRequestSpider @@ -637,11 +637,10 @@ class Http11MockServerTestCase(unittest.TestCase): request.headers.setdefault(b'Accept-Encoding', b'gzip,deflate') request = request.replace(url=self.mockserver.url('/xpayload')) yield crawler.crawl(seed=request) - # download_maxsize = 50 is enough for the gzipped response + # The gzipped response passes the download_maxsize = 50 during + # download, but fails during decompression. failure = crawler.spider.meta.get('failure') - self.assertTrue(failure == None) - reason = crawler.spider.meta['close_reason'] - self.assertTrue(reason, 'finished') + self.assertIsInstance(failure.value, IgnoreRequest) else: # See issue https://twistedmatrix.com/trac/ticket/8175 raise unittest.SkipTest("xpayload only enabled for PY2") diff --git a/tests/test_downloadermiddleware_httpcompression.py b/tests/test_downloadermiddleware_httpcompression.py index 0745c8dd3..73e90bd1f 100644 --- a/tests/test_downloadermiddleware_httpcompression.py +++ b/tests/test_downloadermiddleware_httpcompression.py @@ -1,34 +1,61 @@ -from io import BytesIO -from unittest import TestCase, SkipTest -from os.path import join from gzip import GzipFile +from io import BytesIO +from logging import WARNING +from os.path import join +from unittest import SkipTest, TestCase -from scrapy.spiders import Spider -from scrapy.http import Response, Request, HtmlResponse -from scrapy.downloadermiddlewares.httpcompression import HttpCompressionMiddleware, \ - ACCEPTED_ENCODINGS -from scrapy.responsetypes import responsetypes -from scrapy.utils.gz import gunzip -from tests import tests_datadir +from testfixtures import LogCapture from w3lib.encoding import resolve_encoding +from scrapy.downloadermiddlewares.httpcompression import ( + ACCEPTED_ENCODINGS, + HttpCompressionMiddleware, +) +from scrapy.exceptions import IgnoreRequest +from scrapy.http import HtmlResponse, Request, Response +from scrapy.responsetypes import responsetypes +from scrapy.spiders import Spider +from scrapy.utils.gz import gunzip +from scrapy.utils.test import get_crawler +from tests import tests_datadir SAMPLEDIR = join(tests_datadir, 'compressed') FORMAT = { - 'gzip': ('html-gzip.bin', 'gzip'), - 'x-gzip': ('html-gzip.bin', 'gzip'), - 'rawdeflate': ('html-rawdeflate.bin', 'deflate'), - 'zlibdeflate': ('html-zlibdeflate.bin', 'deflate'), - 'br': ('html-br.bin', 'br') - } + "gzip": ("html-gzip.bin", "gzip"), + "x-gzip": ("html-gzip.bin", "gzip"), + "rawdeflate": ("html-rawdeflate.bin", "deflate"), + "zlibdeflate": ("html-zlibdeflate.bin", "deflate"), + "br": ("html-br.bin", "br"), + # $ zstd raw.html --content-size -o html-zstd-static-content-size.bin + "zstd-static-content-size": ("html-zstd-static-content-size.bin", "zstd"), + # $ zstd raw.html --no-content-size -o html-zstd-static-no-content-size.bin + "zstd-static-no-content-size": ("html-zstd-static-no-content-size.bin", "zstd"), + # $ cat raw.html | zstd -o html-zstd-streaming-no-content-size.bin + "zstd-streaming-no-content-size": ( + "html-zstd-streaming-no-content-size.bin", + "zstd", + ), +} +FORMAT.update( + { + "bomb-{format_id}".format(format_id=format_id): ("bomb-{format_id}.bin".format(format_id=format_id), format_id) + for format_id in ( + "br", # 34 -> 11 511 612 + "deflate", # 27 968 -> 11 511 612 + "gzip", # 27 988 -> 11 511 612 + "zstd", # 1 096 -> 11 511 612 + ) + } +) class HttpCompressionTest(TestCase): def setUp(self): + crawler = get_crawler() self.spider = Spider('foo') - self.mw = HttpCompressionMiddleware() + self.mw = HttpCompressionMiddleware.from_crawler(crawler) def _getresponse(self, coding): if coding not in FORMAT: @@ -65,8 +92,8 @@ class HttpCompressionTest(TestCase): self.assertEqual(response.headers['Content-Encoding'], b'gzip') newresponse = self.mw.process_response(request, response, self.spider) assert newresponse is not response - assert newresponse.body.startswith(b' body size after " + "decompression (11511612 B) is larger than the download " + "warning size (10000000 B)." + ), + ), + ) + + def test_download_warnsize_setting_br(self): + try: + import brotli # noqa: F401 + except ImportError: + raise SkipTest("no brotli") + self._test_download_warnsize_setting("br") + + def test_download_warnsize_setting_deflate(self): + self._test_download_warnsize_setting("deflate") + + def test_download_warnsize_setting_gzip(self): + self._test_download_warnsize_setting("gzip") + + def test_download_warnsize_setting_zstd(self): + try: + import zstandard + except ImportError: + raise SkipTest("no zstandard") + self._test_download_warnsize_setting("zstd") + + def _test_download_warnsize_spider_attr(self, compression_id): + class DownloadWarnSizeSpider(Spider): + download_warnsize = 10000000 + + crawler = get_crawler(DownloadWarnSizeSpider) + spider = crawler._create_spider("scrapytest.org") + mw = HttpCompressionMiddleware.from_crawler(crawler) + mw.open_spider(spider) + response = self._getresponse("bomb-{compression_id}".format(compression_id=compression_id)) + + with LogCapture( + "scrapy.downloadermiddlewares.httpcompression", + propagate=False, + level=WARNING, + ) as log: + mw.process_response(response.request, response, spider) + log.check( + ( + "scrapy.downloadermiddlewares.httpcompression", + "WARNING", + ( + "<200 http://scrapytest.org/> body size after " + "decompression (11511612 B) is larger than the download " + "warning size (10000000 B)." + ), + ), + ) + + def test_download_warnsize_spider_attr_br(self): + try: + import brotli # noqa: F401 + except ImportError: + raise SkipTest("no brotli") + self._test_download_warnsize_spider_attr("br") + + def test_download_warnsize_spider_attr_deflate(self): + self._test_download_warnsize_spider_attr("deflate") + + def test_download_warnsize_spider_attr_gzip(self): + self._test_download_warnsize_spider_attr("gzip") + + def test_download_warnsize_spider_attr_zstd(self): + try: + import zstandard + except ImportError: + raise SkipTest("no zstandard") + self._test_download_warnsize_spider_attr("zstd") + + def _test_download_warnsize_request_meta(self, compression_id): + crawler = get_crawler(Spider) + spider = crawler._create_spider("scrapytest.org") + mw = HttpCompressionMiddleware.from_crawler(crawler) + mw.open_spider(spider) + response = self._getresponse("bomb-{compression_id}".format(compression_id=compression_id)) + response.meta["download_warnsize"] = 10000000 + + with LogCapture( + "scrapy.downloadermiddlewares.httpcompression", + propagate=False, + level=WARNING, + ) as log: + mw.process_response(response.request, response, spider) + log.check( + ( + "scrapy.downloadermiddlewares.httpcompression", + "WARNING", + ( + "<200 http://scrapytest.org/> body size after " + "decompression (11511612 B) is larger than the download " + "warning size (10000000 B)." + ), + ), + ) + + def test_download_warnsize_request_meta_br(self): + try: + import brotli # noqa: F401 + except ImportError: + raise SkipTest("no brotli") + self._test_download_warnsize_request_meta("br") + + def test_download_warnsize_request_meta_deflate(self): + self._test_download_warnsize_request_meta("deflate") + + def test_download_warnsize_request_meta_gzip(self): + self._test_download_warnsize_request_meta("gzip") + + def test_download_warnsize_request_meta_zstd(self): + try: + import zstandard + except ImportError: + raise SkipTest("no zstandard") + self._test_download_warnsize_request_meta("zstd") diff --git a/tests/test_spider.py b/tests/test_spider.py index 2220b8ffc..dab7095c4 100644 --- a/tests/test_spider.py +++ b/tests/test_spider.py @@ -1,23 +1,29 @@ import gzip import inspect +import os import warnings from io import BytesIO +from logging import WARNING from testfixtures import LogCapture from twisted.trial import unittest from scrapy import signals -from scrapy.settings import Settings -from scrapy.http import Request, Response, TextResponse, XmlResponse, HtmlResponse -from scrapy.spiders.init import InitSpider -from scrapy.spiders import Spider, CrawlSpider, Rule, XMLFeedSpider, \ - CSVFeedSpider, SitemapSpider -from scrapy.linkextractors import LinkExtractor from scrapy.exceptions import ScrapyDeprecationWarning -from scrapy.utils.trackref import object_ref +from scrapy.http import HtmlResponse, Request, Response, TextResponse, XmlResponse +from scrapy.linkextractors import LinkExtractor +from scrapy.settings import Settings +from scrapy.spiders import ( + CrawlSpider, + CSVFeedSpider, + Rule, + SitemapSpider, + Spider, + XMLFeedSpider, +) +from scrapy.spiders.init import InitSpider from scrapy.utils.test import get_crawler - -from tests import mock +from tests import mock, tests_datadir class SpiderTest(unittest.TestCase): @@ -399,7 +405,8 @@ class SitemapSpiderTest(SpiderTest): GZBODY = f.getvalue() def assertSitemapBody(self, response, body): - spider = self.spider_class("example.com") + crawler = get_crawler() + spider = self.spider_class.from_crawler(crawler, "example.com") self.assertEqual(spider._get_sitemap_body(response), body) def test_get_sitemap_body(self): @@ -413,8 +420,12 @@ class SitemapSpiderTest(SpiderTest): self.assertSitemapBody(r, None) def test_get_sitemap_body_gzip_headers(self): - r = Response(url="http://www.example.com/sitemap", body=self.GZBODY, - headers={"content-type": "application/gzip"}) + r = Response( + url="http://www.example.com/sitemap", + body=self.GZBODY, + headers={"content-type": "application/gzip"}, + request=Request("http://www.example.com/sitemap"), + ) self.assertSitemapBody(r, self.BODY) def test_get_sitemap_body_xml_url(self): @@ -422,7 +433,11 @@ class SitemapSpiderTest(SpiderTest): self.assertSitemapBody(r, self.BODY) def test_get_sitemap_body_xml_url_compressed(self): - r = Response(url="http://www.example.com/sitemap.xml.gz", body=self.GZBODY) + r = Response( + url="http://www.example.com/sitemap.xml.gz", + body=self.GZBODY, + request=Request("http://www.example.com/sitemap"), + ) self.assertSitemapBody(r, self.BODY) # .xml.gz but body decoded by HttpCompression middleware already @@ -570,6 +585,116 @@ Sitemap: /sitemap-relative-url.xml self.assertEqual([req.url for req in spider._parse_sitemap(r)], ['http://www.example.com/sitemap2.xml']) + def test_compression_bomb_setting(self): + settings = {"DOWNLOAD_MAXSIZE": 10000000} + crawler = get_crawler(settings_dict=settings) + spider = self.spider_class.from_crawler(crawler, "example.com") + body_path = os.path.join(tests_datadir, "compressed", "bomb-gzip.bin") + body = open(body_path, "rb").read() + request = Request(url="https://example.com") + response = Response(url="https://example.com", body=body, request=request) + self.assertIsNone(spider._get_sitemap_body(response)) + + def test_compression_bomb_spider_attr(self): + class DownloadMaxSizeSpider(self.spider_class): + download_maxsize = 10000000 + + crawler = get_crawler() + spider = DownloadMaxSizeSpider.from_crawler(crawler, "example.com") + body_path = os.path.join(tests_datadir, "compressed", "bomb-gzip.bin") + body = open(body_path, "rb").read() + request = Request(url="https://example.com") + response = Response(url="https://example.com", body=body, request=request) + self.assertIsNone(spider._get_sitemap_body(response)) + + def test_compression_bomb_request_meta(self): + crawler = get_crawler() + spider = self.spider_class.from_crawler(crawler, "example.com") + body_path = os.path.join(tests_datadir, "compressed", "bomb-gzip.bin") + body = open(body_path, "rb").read() + request = Request( + url="https://example.com", meta={"download_maxsize": 10000000} + ) + response = Response(url="https://example.com", body=body, request=request) + self.assertIsNone(spider._get_sitemap_body(response)) + + def test_download_warnsize_setting(self): + settings = {"DOWNLOAD_WARNSIZE": 10000000} + crawler = get_crawler(settings_dict=settings) + spider = self.spider_class.from_crawler(crawler, "example.com") + body_path = os.path.join(tests_datadir, "compressed", "bomb-gzip.bin") + body = open(body_path, "rb").read() + request = Request(url="https://example.com") + response = Response(url="https://example.com", body=body, request=request) + with LogCapture( + "scrapy.spiders.sitemap", propagate=False, level=WARNING + ) as log: + spider._get_sitemap_body(response) + log.check( + ( + "scrapy.spiders.sitemap", + "WARNING", + ( + "<200 https://example.com> body size after decompression " + "(11511612 B) is larger than the download warning size " + "(10000000 B)." + ), + ), + ) + + def test_download_warnsize_spider_attr(self): + class DownloadWarnSizeSpider(self.spider_class): + download_warnsize = 10000000 + + crawler = get_crawler() + spider = DownloadWarnSizeSpider.from_crawler(crawler, "example.com") + body_path = os.path.join(tests_datadir, "compressed", "bomb-gzip.bin") + body = open(body_path, "rb").read() + request = Request( + url="https://example.com", meta={"download_warnsize": 10000000} + ) + response = Response(url="https://example.com", body=body, request=request) + with LogCapture( + "scrapy.spiders.sitemap", propagate=False, level=WARNING + ) as log: + spider._get_sitemap_body(response) + log.check( + ( + "scrapy.spiders.sitemap", + "WARNING", + ( + "<200 https://example.com> body size after decompression " + "(11511612 B) is larger than the download warning size " + "(10000000 B)." + ), + ), + ) + + def test_download_warnsize_request_meta(self): + crawler = get_crawler() + spider = self.spider_class.from_crawler(crawler, "example.com") + body_path = os.path.join(tests_datadir, "compressed", "bomb-gzip.bin") + body = open(body_path, "rb").read() + request = Request( + url="https://example.com", meta={"download_warnsize": 10000000} + ) + response = Response(url="https://example.com", body=body, request=request) + with LogCapture( + "scrapy.spiders.sitemap", propagate=False, level=WARNING + ) as log: + spider._get_sitemap_body(response) + log.check( + ( + "scrapy.spiders.sitemap", + "WARNING", + ( + "<200 https://example.com> body size after decompression " + "(11511612 B) is larger than the download warning size " + "(10000000 B)." + ), + ), + ) + class DeprecationTest(unittest.TestCase): From 06c9692769902fc2a8b551ace41dc2b273b41a55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Chaves?= Date: Fri, 24 Nov 2023 14:10:59 +0100 Subject: [PATCH 2/4] Backport improvements --- .../downloadermiddlewares/httpcompression.py | 2 ++ scrapy/utils/_compression.py | 33 ++++++++++--------- scrapy/utils/gz.py | 21 ++++++------ 3 files changed, 31 insertions(+), 25 deletions(-) diff --git a/scrapy/downloadermiddlewares/httpcompression.py b/scrapy/downloadermiddlewares/httpcompression.py index 6db817ffe..f495f99a9 100644 --- a/scrapy/downloadermiddlewares/httpcompression.py +++ b/scrapy/downloadermiddlewares/httpcompression.py @@ -39,6 +39,8 @@ class HttpCompressionMiddleware(object): def __init__(self, crawler=None): if not crawler: + self._max_size = 1073741824 + self._warn_size = 33554432 return self._max_size = crawler.settings.getint("DOWNLOAD_MAXSIZE") self._warn_size = crawler.settings.getint("DOWNLOAD_WARNSIZE") diff --git a/scrapy/utils/_compression.py b/scrapy/utils/_compression.py index 2260c55a8..8b1f073bb 100644 --- a/scrapy/utils/_compression.py +++ b/scrapy/utils/_compression.py @@ -12,6 +12,9 @@ except ImportError: pass +_CHUNK_SIZE = 65536 # 64 KiB + + class _DecompressionMaxSizeExceeded(ValueError): pass @@ -20,12 +23,11 @@ def _inflate(data, max_size=0): decompressor = zlib.decompressobj() raw_decompressor = zlib.decompressobj(-15) input_stream = BytesIO(data) - output_list = [] + output_stream = BytesIO() output_chunk = b"." decompressed_size = 0 - CHUNK_SIZE = 8196 while output_chunk: - input_chunk = input_stream.read(CHUNK_SIZE) + input_chunk = input_stream.read(_CHUNK_SIZE) try: output_chunk = decompressor.decompress(input_chunk) except zlib.error: @@ -49,19 +51,19 @@ def _inflate(data, max_size=0): max_size=max_size, ) ) - output_list.append(output_chunk) - return b"".join(output_list) + output_stream.write(output_chunk) + output_stream.seek(0) + return output_stream.read() def _unbrotli(data, max_size=0): decompressor = brotli.Decompressor() input_stream = BytesIO(data) - output_list = [] + output_stream = BytesIO() output_chunk = b"." decompressed_size = 0 - CHUNK_SIZE = 8196 while output_chunk: - input_chunk = input_stream.read(CHUNK_SIZE) + input_chunk = input_stream.read(_CHUNK_SIZE) output_chunk = decompressor.decompress(input_chunk) decompressed_size += len(output_chunk) if max_size and decompressed_size > max_size: @@ -73,19 +75,19 @@ def _unbrotli(data, max_size=0): max_size=max_size, ) ) - output_list.append(output_chunk) - return b"".join(output_list) + output_stream.write(output_chunk) + output_stream.seek(0) + return output_stream.read() def _unzstd(data, max_size=0): decompressor = zstandard.ZstdDecompressor() stream_reader = decompressor.stream_reader(BytesIO(data)) - output_list = [] + output_stream = BytesIO() output_chunk = b"." decompressed_size = 0 - CHUNK_SIZE = 8196 while output_chunk: - output_chunk = stream_reader.read(CHUNK_SIZE) + output_chunk = stream_reader.read(_CHUNK_SIZE) decompressed_size += len(output_chunk) if max_size and decompressed_size > max_size: raise _DecompressionMaxSizeExceeded( @@ -96,5 +98,6 @@ def _unzstd(data, max_size=0): max_size=max_size, ) ) - output_list.append(output_chunk) - return b"".join(output_list) + output_stream.write(output_chunk) + output_stream.seek(0) + return output_stream.read() diff --git a/scrapy/utils/gz.py b/scrapy/utils/gz.py index 9f28cfb77..15a1ecfe6 100644 --- a/scrapy/utils/gz.py +++ b/scrapy/utils/gz.py @@ -12,7 +12,7 @@ import six from scrapy.utils.decorators import deprecated -from ._compression import _DecompressionMaxSizeExceeded +from ._compression import _CHUNK_SIZE, _DecompressionMaxSizeExceeded # - Python>=3.5 GzipFile's read() has issues returning leftover # uncompressed data when input is corrupted @@ -35,25 +35,25 @@ def gunzip(data, max_size=0): This is resilient to CRC checksum errors. """ f = GzipFile(fileobj=BytesIO(data)) - output_list = [] - chunk = b"." + output_stream = BytesIO() + output_chunk = b"." decompressed_size = 0 - while chunk: + while output_chunk: try: - chunk = read1(f, 8196) + output_chunk = read1(f, _CHUNK_SIZE) except (IOError, EOFError, struct.error): # complete only if there is some data, otherwise re-raise # see issue 87 about catching struct.error # some pages are quite small so output_list is empty and f.extrabuf # contains the whole page content - if output_list or getattr(f, 'extrabuf', None): + if decompressed_size or getattr(f, 'extrabuf', None): try: - output_list.append(f.extrabuf[-f.extrasize:]) + output_stream.write(f.extrabuf[-f.extrasize:]) finally: break else: raise - decompressed_size += len(chunk) + decompressed_size += len(output_chunk) if max_size and decompressed_size > max_size: raise _DecompressionMaxSizeExceeded( "The number of bytes decompressed so far " @@ -63,8 +63,9 @@ def gunzip(data, max_size=0): max_size=max_size, ) ) - output_list.append(chunk) - return b"".join(output_list) + output_stream.write(output_chunk) + output_stream.seek(0) + return output_stream.read() _is_gzipped = re.compile(br'^application/(x-)?gzip\b', re.I).search _is_octetstream = re.compile(br'^(application|binary)/octet-stream\b', re.I).search From 54604823197c0c8066acc28fa0e2b8566aeb5ff8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Chaves?= Date: Mon, 11 Dec 2023 17:39:55 +0100 Subject: [PATCH 3/4] =?UTF-8?q?spider=20=E2=86=92=20mw?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- scrapy/downloadermiddlewares/httpcompression.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/scrapy/downloadermiddlewares/httpcompression.py b/scrapy/downloadermiddlewares/httpcompression.py index f495f99a9..eb53607e4 100644 --- a/scrapy/downloadermiddlewares/httpcompression.py +++ b/scrapy/downloadermiddlewares/httpcompression.py @@ -59,11 +59,11 @@ class HttpCompressionMiddleware(object): "reimplement their 'from_crawler' method.", ScrapyDeprecationWarning, ) - spider = cls() - spider._max_size = crawler.settings.getint("DOWNLOAD_MAXSIZE") - spider._warn_size = crawler.settings.getint("DOWNLOAD_WARNSIZE") - crawler.signals.connect(spider.open_spider, signals.spider_opened) - return spider + mw = cls() + mw._max_size = crawler.settings.getint("DOWNLOAD_MAXSIZE") + mw._warn_size = crawler.settings.getint("DOWNLOAD_WARNSIZE") + crawler.signals.connect(mw.open_spider, signals.spider_opened) + return mw def open_spider(self, spider): if hasattr(spider, "download_maxsize"): From 978407c938057b2e7d1bad3186fa7d102fffc9c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Chaves?= Date: Wed, 13 Dec 2023 13:42:37 +0100 Subject: [PATCH 4/4] Warn against using scrapy.downloadermiddlewares.decompression --- docs/news.rst | 6 ++++++ scrapy/downloadermiddlewares/decompression.py | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/docs/news.rst b/docs/news.rst index 4de283ddf..1ecc81ae7 100644 --- a/docs/news.rst +++ b/docs/news.rst @@ -17,6 +17,12 @@ Scrapy 1.8.4 (unreleased) to the decompressed response body. Please, see the `7j7m-v7m3-jqm7 security advisory`_ for more information. + .. _7j7m-v7m3-jqm7 security advisory: https://github.com/scrapy/scrapy/security/advisories/GHSA-7j7m-v7m3-jqm7 + +- Also in relation with the `7j7m-v7m3-jqm7 security advisory`_, use of the + ``scrapy.downloadermiddlewares.decompression`` module is discouraged and + will trigger a warning. + .. _release-1.8.3: diff --git a/scrapy/downloadermiddlewares/decompression.py b/scrapy/downloadermiddlewares/decompression.py index 49313cc04..5e9e9bc48 100644 --- a/scrapy/downloadermiddlewares/decompression.py +++ b/scrapy/downloadermiddlewares/decompression.py @@ -8,6 +8,7 @@ import zipfile import tarfile import logging from tempfile import mktemp +from warnings import warn import six @@ -16,8 +17,18 @@ try: except ImportError: from io import BytesIO +from scrapy.exceptions import ScrapyDeprecationWarning from scrapy.responsetypes import responsetypes +warn( + "Use of the scrapy.downloadermiddlewares.decompression module is " + "discouraged, as it is susceptible to decompression bomb attacks. For " + "details, see " + "https://github.com/scrapy/scrapy/security/advisories/GHSA-7j7m-v7m3-jqm7", + ScrapyDeprecationWarning, + stacklevel=2, +) + logger = logging.getLogger(__name__)