diff --git a/docs/news.rst b/docs/news.rst index 6966e7763..582c543ef 100644 --- a/docs/news.rst +++ b/docs/news.rst @@ -10,10 +10,30 @@ Scrapy 2.17.0 (unreleased) Highlights: +- Security bug fixes + - HTTP/2 and SOCKS proxy support for ``HttpxDownloadHandler`` - Improved settings for changing allowed TLS versions +Security bug fixes +~~~~~~~~~~~~~~~~~~ + +- ``s3://`` requests now use HTTPS by default, instead of plaintext HTTP. + + Previously, :class:`~scrapy.core.downloader.handlers.s3.S3DownloadHandler` + sent signed S3 requests over plaintext HTTP unless + ``request.meta["is_secure"]`` was set to a true value, exposing the request + path, the AWS ``Authorization`` header, the ``X-Amz-Security-Token`` header + (when using temporary credentials), and the response contents to network + attackers, who could also tamper with responses. See the `76g3-c3x4-crvx`_ + security advisory for details. + + To restore the previous behavior for a given request, set + ``request.meta["is_secure"]`` to ``False``. + + .. _76g3-c3x4-crvx: https://github.com/scrapy/scrapy/security/advisories/GHSA-76g3-c3x4-crvx + Deprecations ~~~~~~~~~~~~ diff --git a/scrapy/core/downloader/handlers/s3.py b/scrapy/core/downloader/handlers/s3.py index c6660ff48..9b7ceeb4c 100644 --- a/scrapy/core/downloader/handlers/s3.py +++ b/scrapy/core/downloader/handlers/s3.py @@ -49,7 +49,7 @@ class S3DownloadHandler(BaseDownloadHandler): async def download_request(self, request: Request) -> Response: p = urlparse_cached(request) - scheme = "https" if request.meta.get("is_secure") else "http" + scheme = "http" if request.meta.get("is_secure") is False else "https" bucket = p.hostname path = p.path + "?" + p.query if p.query else p.path url = f"{scheme}://{bucket}.s3.amazonaws.com{path}" diff --git a/tests/test_downloader_handlers.py b/tests/test_downloader_handlers.py index e685f607a..275199a76 100644 --- a/tests/test_downloader_handlers.py +++ b/tests/test_downloader_handlers.py @@ -170,6 +170,12 @@ class TestS3Anon: httpreq = await self.download_request(req) assert hasattr(self.s3reqh, "anon") assert self.s3reqh.anon + assert httpreq.url == "https://aws-publicdatasets.s3.amazonaws.com/" + + @coroutine_test + async def test_anon_request_insecure(self): + req = Request("s3://aws-publicdatasets/", meta={"is_secure": False}) + httpreq = await self.download_request(req) assert httpreq.url == "http://aws-publicdatasets.s3.amazonaws.com/" @@ -200,6 +206,18 @@ class TestS3: mock_formatdate.return_value = date yield + @coroutine_test + async def test_secure_by_default(self): + req = Request("s3://johnsmith/photos/puppy.jpg") + httpreq = await self.download_request(req) + assert httpreq.url == "https://johnsmith.s3.amazonaws.com/photos/puppy.jpg" + + @coroutine_test + async def test_insecure_opt_out(self): + req = Request("s3://johnsmith/photos/puppy.jpg", meta={"is_secure": False}) + httpreq = await self.download_request(req) + assert httpreq.url == "http://johnsmith.s3.amazonaws.com/photos/puppy.jpg" + @coroutine_test async def test_request_signing1(self): # gets an object from the johnsmith bucket.