diff --git a/changes/3045.bugfix.md b/changes/3045.bugfix.md new file mode 100644 index 0000000000..4a435a6ee9 --- /dev/null +++ b/changes/3045.bugfix.md @@ -0,0 +1 @@ +Downloads that are already present in the download cache are now re-verified against their expected hash, rather than being trusted unconditionally. This provides protection against cache poisoning as a potential attack vector. diff --git a/src/briefcase/integrations/file.py b/src/briefcase/integrations/file.py index 0bb51a7942..868cee8dda 100644 --- a/src/briefcase/integrations/file.py +++ b/src/briefcase/integrations/file.py @@ -282,6 +282,13 @@ def download( doesn't match, a `CorruptContentError` is raised. Any fixed-length hash algorithm provided by hashlib can be used. + If the file is already present in the download cache, it is *not* + re-downloaded; however, its content is still re-hashed and compared + against `expected_hash` every time, so a corrupted or tampered cache + entry will still raise `CorruptContentError`. A console message + confirms when a hash has been checked and verified, for both cached + and freshly downloaded files. + :param url: The URL to download :param download_path: The path to the download cache folder. This path will be created if it doesn't exist. @@ -331,14 +338,31 @@ def download( cache_name = cache_full_name.split("/")[-1] filename = download_path / cache_name + if expected_hash is None: + self.tools.console.warning( + f"The integrity of {cache_name} will not be verified " + "as no reference hash been provided." + ) + if filename.exists(): - self.tools.console.info(f"{cache_name} already downloaded") - else: - if expected_hash is None: - self.tools.console.warning( - f"The integrity of {cache_name} will not be verified " - "as no reference hash been provided." + if algorithm is None: + self.tools.console.info(f"{cache_name} already downloaded.") + else: + with filename.open("rb") as f: + hash = hashlib.file_digest(f, algorithm) + + actual_digest = hash.hexdigest() + if actual_digest.lower() != digest.lower(): + raise CorruptContentError( + role=role or filename.name, + expected_hash=f"{algorithm}:{digest}", + actual_hash=f"{algorithm}:{actual_digest}", + ) + self.tools.console.info( + f"{cache_name} already downloaded; " + f"hash verified ({algorithm})." ) + else: self.tools.console.info(f"Downloading {cache_name}...") self._fetch_and_write_content( response, @@ -488,11 +512,15 @@ def _fetch_and_write_content( hasher.update(data) progress_bar.update(task_id, advance=len(data)) - if hasher is not None and hasher.hexdigest().lower() != digest.lower(): - raise CorruptContentError( - role=role or filename.name, - expected_hash=f"{hasher.name}:{digest}", - actual_hash=f"{hasher.name}:{hasher.hexdigest()}", + if hasher is not None: + if hasher.hexdigest().lower() != digest.lower(): + raise CorruptContentError( + role=role or filename.name, + expected_hash=f"{hasher.name}:{digest}", + actual_hash=f"{hasher.name}:{hasher.hexdigest()}", + ) + self.tools.console.info( + f"{filename.name} hash verified ({hasher.name})." ) # This file move short circuits to a file rename when the source and diff --git a/tests/integrations/file/test_File__download.py b/tests/integrations/file/test_File__download.py index f67c00dc28..32d39b0bcb 100644 --- a/tests/integrations/file/test_File__download.py +++ b/tests/integrations/file/test_File__download.py @@ -221,11 +221,17 @@ def test_new_download_oneshot( with (mock_tools.base_path / "downloads/something.zip").open(encoding="utf-8") as f: assert f.read() == "all content" - # Verify that the expected warnings are output + # Verify that the expected warnings/confirmations are output + output = capsys.readouterr().out if hash_algorithm is None: - assert "will not be verified" in capsys.readouterr().out + assert "will not be verified" in output + assert "hash verified" not in output + elif hash_algorithm == "unverified": + assert "will not be verified" not in output + assert "hash verified" not in output else: - assert "will not be verified" not in capsys.readouterr().out + assert "will not be verified" not in output + assert f"hash verified ({hash_algorithm})" in output @pytest.mark.parametrize( @@ -299,16 +305,23 @@ def test_new_download_chunked(mock_tools, file_perms, hash_algorithm, capsys): with (mock_tools.base_path / "something.zip").open(encoding="utf-8") as f: assert f.read() == "chunk-1;chunk-2;chunk-3;" - # Verify that the expected warnings are output + # Verify that the expected warnings/confirmations are output + output = capsys.readouterr().out if hash_algorithm is None: - assert "will not be verified" in capsys.readouterr().out + assert "will not be verified" in output + assert "hash verified" not in output + elif hash_algorithm == "unverified": + assert "will not be verified" not in output + assert "hash verified" not in output else: - assert "will not be verified" not in capsys.readouterr().out + assert "will not be verified" not in output + assert f"hash verified ({hash_algorithm})" in output @pytest.mark.parametrize("hash_algorithm", [None, "unverified", "sha256"]) def test_already_downloaded(mock_tools, hash_algorithm, capsys): - """If the file already exists on disk, it isn't re-downloaded. + """If the file already exists on disk, it isn't re-downloaded, but its hash is re- + verified against expected_hash. The request is still made to derive the filename, but the content is never streamed. """ @@ -367,8 +380,73 @@ def test_already_downloaded(mock_tools, hash_algorithm, capsys): mock_tools.os.chmod.assert_not_called() mock_tools.os.remove.assert_not_called() - # No mention of verification - assert "will not be verified" not in capsys.readouterr().out + output = capsys.readouterr().out + if hash_algorithm is None: + # No hash was provided, so a warning is logged, and there's nothing to + # confirm as verified. + assert "will not be verified" in output + assert "already downloaded." in output + assert "already downloaded; hash verified" not in output + elif hash_algorithm == "unverified": + # Verification was deliberately skipped; no warning, nothing verified. + assert "will not be verified" not in output + assert "already downloaded." in output + assert "already downloaded; hash verified" not in output + else: + # A real hash was provided and matched; confirm it was verified. + assert "will not be verified" not in output + assert "already downloaded." not in output + assert f"already downloaded; hash verified ({hash_algorithm})" in output + + +def test_already_downloaded_hash_mismatch(mock_tools, capsys): + """If the cached file's content doesn't match expected_hash, CorruptContentError is + raised, even though the file is already on disk.""" + content = b"existing content" + + # Create an existing file whose content doesn't match the expected hash + existing_file = mock_tools.base_path / "something.zip" + with existing_file.open("w", encoding="utf-8") as f: + f.write(content.decode()) + + url = "https://example.com/path/to/something.zip" + + response = _make_httpx_response( + status_code=200, + url=url, + headers={"content-length": "100", "content-encoding": "gzip"}, + stream=[b"definitely not gzip content"], + ) + mock_tools.httpx.stream.return_value.__enter__.return_value = response + + expected_hash = f"sha256:{'0' * 64}" + + with pytest.raises(CorruptContentError) as exc_info: + mock_tools.file.download( + url=url, + download_path=mock_tools.base_path, + role="something", + expected_hash=expected_hash, + ) + + assert exc_info.value.role == "something" + assert exc_info.value.expected_hash == expected_hash + assert exc_info.value.actual_hash == ( + f"sha256:{hashlib.sha256(content).hexdigest()}" + ) + + # The cached file was not touched + assert existing_file.exists() + with existing_file.open(encoding="utf-8") as f: + assert f.read() == content.decode() + + # Temporary file was not created, moved, or deleted + mock_tools.shutil.move.assert_not_called() + mock_tools.os.chmod.assert_not_called() + mock_tools.os.remove.assert_not_called() + + # No success confirmation was logged + assert "hash verified" not in capsys.readouterr().out def test_missing_resource(mock_tools):