diff --git a/custom_components/hacs/repositories/base.py b/custom_components/hacs/repositories/base.py index a3d1807dc43..d91000d19c4 100644 --- a/custom_components/hacs/repositories/base.py +++ b/custom_components/hacs/repositories/base.py @@ -35,7 +35,7 @@ from ..utils.filters import filter_content_return_one_of_type from ..utils.json import json_loads from ..utils.logger import LOGGER -from ..utils.path import is_safe +from ..utils.path import is_safe, is_safe_relative_path from ..utils.queue_manager import QueueManager from ..utils.store import async_remove_store from ..utils.url import github_archive, github_release_asset @@ -249,6 +249,15 @@ def from_dict(manifest: dict): setattr(manifest_data, key, [value]) elif key in manifest_data.__dict__: setattr(manifest_data, key, value) + + # These end up in filesystem paths, a hostile manifest must not be able + # to point them outside the repository content directory. The whole + # manifest is rejected, a manifest that tries this is not to be trusted. + for key in ("filename", "persistent_directory"): + value = getattr(manifest_data, key) + if value is not None and not is_safe_relative_path(value): + raise HacsException(f"Unsafe {key} value '{value}' in the HACS manifest") + return manifest_data def update_data(self, data: dict) -> None: diff --git a/custom_components/hacs/utils/data.py b/custom_components/hacs/utils/data.py index f540272e62d..f1a96535d77 100644 --- a/custom_components/hacs/utils/data.py +++ b/custom_components/hacs/utils/data.py @@ -12,6 +12,7 @@ from ..base import HacsBase from ..const import HACS_REPOSITORY_ID from ..enums import HacsDisabledReason, HacsDispatchEvent +from ..exceptions import HacsException from ..repositories.base import TOPIC_FILTER, HacsManifest, HacsRepository from .logger import LOGGER from .path import is_safe @@ -304,9 +305,19 @@ def async_restore_repository(self, entry: str, repository_data: dict[str, Any]): if last_fetched := repository_data.get("last_fetched"): repository.data.last_fetched = datetime.fromtimestamp(last_fetched, UTC) - repository.repository_manifest = HacsManifest.from_dict( - repository_data.get("manifest") or repository_data.get("repository_manifest") or {} - ) + try: + repository.repository_manifest = HacsManifest.from_dict( + repository_data.get("manifest") or repository_data.get("repository_manifest") or {} + ) + except HacsException as exception: + # Stored data can predate the path validation of the manifest, one bad + # entry must not take down the restore of every other repository. + self.logger.warning( + " %s for %s", + exception, + repository.data.full_name, + ) + repository.repository_manifest = HacsManifest.from_dict({}) if repository.data.prerelease == repository.data.last_version: repository.data.prerelease = None diff --git a/custom_components/hacs/utils/path.py b/custom_components/hacs/utils/path.py index 7994a8dcc66..9f231403ed8 100644 --- a/custom_components/hacs/utils/path.py +++ b/custom_components/hacs/utils/path.py @@ -3,7 +3,7 @@ from __future__ import annotations from functools import lru_cache -from pathlib import Path +from pathlib import Path, PureWindowsPath from typing import TYPE_CHECKING if TYPE_CHECKING: @@ -39,3 +39,15 @@ def is_safe(hacs: HacsBase, path: str | Path) -> bool: configuration.python_script_path, configuration.theme_path, ) + + +def is_safe_relative_path(value: str) -> bool: + """Check that a repository provided path is relative, without traversal.""" + if not isinstance(value, str): + return False + + normalized = value.replace("\\", "/") + if normalized.startswith("/") or PureWindowsPath(value).drive: + return False + + return ".." not in normalized.split("/") diff --git a/custom_components/hacs/utils/validate.py b/custom_components/hacs/utils/validate.py index fa25be9af8a..b75797569f9 100644 --- a/custom_components/hacs/utils/validate.py +++ b/custom_components/hacs/utils/validate.py @@ -11,6 +11,7 @@ import voluptuous as vol from ..const import LOCALE +from .path import is_safe_relative_path @dataclass @@ -43,15 +44,25 @@ def _country_validator(values) -> list[str]: return countries +def _relative_path_validator(value) -> str: + """Custom validator for repository provided paths.""" + if not isinstance(value, str): + raise vol.Invalid(f"Value '{value}' is not a string.") + if not is_safe_relative_path(value): + raise vol.Invalid(f"Value '{value}' is not a safe relative path.") + + return value + + HACS_MANIFEST_JSON_SCHEMA = vol.Schema( { vol.Optional("content_in_root"): bool, vol.Optional("country"): _country_validator, - vol.Optional("filename"): str, + vol.Optional("filename"): _relative_path_validator, vol.Optional("hacs"): str, vol.Optional("hide_default_branch"): bool, vol.Optional("homeassistant"): str, - vol.Optional("persistent_directory"): str, + vol.Optional("persistent_directory"): _relative_path_validator, vol.Optional("render_readme"): bool, vol.Optional("zip_release"): bool, vol.Required("name"): str, diff --git a/tests/hacsbase/test_hacsbase_data.py b/tests/hacsbase/test_hacsbase_data.py index efe4a7815b3..f7a8c85b931 100644 --- a/tests/hacsbase/test_hacsbase_data.py +++ b/tests/hacsbase/test_hacsbase_data.py @@ -67,3 +67,46 @@ async def _mocked_loads(hass, key): await data.async_write() assert mock_async_save_to_store.called assert "Loading base repository information" not in caplog.text + + +async def test_hacs_data_restore_with_unsafe_manifest(hacs, caplog): + """An unsafe stored manifest is dropped, without failing the whole restore.""" + data = HacsData(hacs) + + async def _mocked_loads(hass, key): + if key == "repositories": + return { + "202226247": { + "category": "integration", + "full_name": "shbatm/hacs-isy994", + "installed": True, + "manifest": { + "name": "ISY994", + "persistent_directory": "../../../evil", + }, + }, + "999888777": { + "category": "integration", + "full_name": "test-org/second-integration", + "installed": False, + }, + } + if key in ("hacs", "data", "renamed_repositories"): + return {} + raise ValueError(f"No mock for {key}") + + with patch("os.path.exists", return_value=True), patch( + "custom_components.hacs.utils.data.async_load_from_store", + side_effect=_mocked_loads, + ): + assert await data.restore() + + repository = hacs.repositories.get_by_id("202226247") + assert repository.repository_manifest.persistent_directory is None + assert ( + "Unsafe persistent_directory value '../../../evil' in the HACS manifest for shbatm/hacs-isy994" + in caplog.text + ) + + # The other repositories are still restored + assert hacs.repositories.get_by_full_name("test-org/second-integration") diff --git a/tests/repositories/test_hacs_manifest.py b/tests/repositories/test_hacs_manifest.py index e1f44f95937..1afbcc76e30 100644 --- a/tests/repositories/test_hacs_manifest.py +++ b/tests/repositories/test_hacs_manifest.py @@ -1,5 +1,7 @@ """HACS Manifest Test Suite.""" # pylint: disable=missing-docstring +import re + import pytest from custom_components.hacs.exceptions import HacsException @@ -42,3 +44,29 @@ def test_manifest_structure(): def test_edge_pass_none(): with pytest.raises(HacsException): assert HacsManifest.from_dict(None) + + +@pytest.mark.parametrize("key", ["filename", "persistent_directory"]) +def test_unsafe_paths_reject_the_manifest(key: str): + with pytest.raises( + HacsException, + match=re.escape(f"Unsafe {key} value '../../../evil' in the HACS manifest"), + ): + HacsManifest.from_dict({"name": "TEST", key: "../../../evil"}) + + +@pytest.mark.parametrize("key", ["filename", "persistent_directory"]) +def test_safe_paths_are_kept(key: str): + manifest = HacsManifest.from_dict({"name": "TEST", key: "sub/dir"}) + + assert getattr(manifest, key) == "sub/dir" + + +@pytest.mark.parametrize("value", [False, 0, 123, ["list"]]) +@pytest.mark.parametrize("key", ["filename", "persistent_directory"]) +def test_non_string_paths_reject_the_manifest(key: str, value): + with pytest.raises( + HacsException, + match=re.escape(f"Unsafe {key} value '{value}' in the HACS manifest"), + ): + HacsManifest.from_dict({"name": "TEST", key: value}) diff --git a/tests/snapshots/api-usage/tests/hacsbase/test_hacsbase_datatest-hacs-data-restore-with-unsafe-manifest.json b/tests/snapshots/api-usage/tests/hacsbase/test_hacsbase_datatest-hacs-data-restore-with-unsafe-manifest.json new file mode 100644 index 00000000000..f9b5ae666bb --- /dev/null +++ b/tests/snapshots/api-usage/tests/hacsbase/test_hacsbase_datatest-hacs-data-restore-with-unsafe-manifest.json @@ -0,0 +1,9 @@ +{ + "tests/hacsbase/test_hacsbase_data.py::test_hacs_data_restore_with_unsafe_manifest": { + "https://api.github.com/repos/hacs/integration": 1, + "https://api.github.com/repos/hacs/integration/contents/custom_components/hacs/manifest.json": 1, + "https://api.github.com/repos/hacs/integration/contents/hacs.json": 1, + "https://api.github.com/repos/hacs/integration/git/trees/main": 1, + "https://api.github.com/repos/hacs/integration/releases": 1 + } +} \ No newline at end of file diff --git a/tests/snapshots/api-usage/tests/validate/test_hacsjson_checktest-hacs-manifest-with-unsafe-path.json b/tests/snapshots/api-usage/tests/validate/test_hacsjson_checktest-hacs-manifest-with-unsafe-path.json new file mode 100644 index 00000000000..d50c90642e1 --- /dev/null +++ b/tests/snapshots/api-usage/tests/validate/test_hacsjson_checktest-hacs-manifest-with-unsafe-path.json @@ -0,0 +1,9 @@ +{ + "tests/validate/test_hacsjson_check.py::test_hacs_manifest_with_unsafe_path": { + "https://api.github.com/repos/hacs/integration": 1, + "https://api.github.com/repos/hacs/integration/contents/custom_components/hacs/manifest.json": 1, + "https://api.github.com/repos/hacs/integration/contents/hacs.json": 1, + "https://api.github.com/repos/hacs/integration/git/trees/main": 1, + "https://api.github.com/repos/hacs/integration/releases": 1 + } +} \ No newline at end of file diff --git a/tests/utils/test_path.py b/tests/utils/test_path.py index 9c1adef0de7..7bc9e1110d4 100644 --- a/tests/utils/test_path.py +++ b/tests/utils/test_path.py @@ -1,3 +1,5 @@ +import pytest + from custom_components.hacs.base import HacsBase from custom_components.hacs.utils import path @@ -7,3 +9,28 @@ def test_is_safe(hacs: HacsBase) -> None: assert not path.is_safe(hacs, f"{hacs.core.config_path}/{hacs.configuration.theme_path}/") assert not path.is_safe(hacs, f"{hacs.core.config_path}/custom_components/") assert not path.is_safe(hacs, f"{hacs.core.config_path}/custom_components") + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + ("example.js", True), + ("sub/dir/example.js", True), + ("userfiles", True), + ("..", False), + ("../example.js", False), + ("sub/../../example.js", False), + ("/etc/passwd", False), + ("\\windows\\style", False), + ("sub\\..\\..\\example.js", False), + ("C:\\windows\\style", False), + ("C:/windows/style", False), + ("C:windows\\style", False), + ("//server/share", False), + (None, False), + (123, False), + (False, False), + ], +) +def test_is_safe_relative_path(value, expected: bool) -> None: + assert path.is_safe_relative_path(value) is expected diff --git a/tests/utils/test_validate.py b/tests/utils/test_validate.py index e936c51bc52..c15492a93e2 100644 --- a/tests/utils/test_validate.py +++ b/tests/utils/test_validate.py @@ -88,6 +88,20 @@ def test_hacs_manifest_json_schema(): with pytest.raises(Invalid, match=re.escape("Value 'False' is not a string or list.")): hacs_json_schema({"name": "My awesome thing", "country": False}) + for key in ("filename", "persistent_directory"): + with pytest.raises( + Invalid, match=re.escape("Value '../secrets' is not a safe relative path."), + ): + hacs_json_schema({"name": "My awesome thing", key: "../secrets"}) + + with pytest.raises( + Invalid, match=re.escape("Value '/etc/passwd' is not a safe relative path."), + ): + hacs_json_schema({"name": "My awesome thing", key: "/etc/passwd"}) + + with pytest.raises(Invalid, match=re.escape("Value 'False' is not a string.")): + hacs_json_schema({"name": "My awesome thing", key: False}) + def test_integration_json_schema(): """Test integration manifest.""" diff --git a/tests/validate/test_hacsjson_check.py b/tests/validate/test_hacsjson_check.py index 2a994c0d2c7..a495ce34fef 100644 --- a/tests/validate/test_hacsjson_check.py +++ b/tests/validate/test_hacsjson_check.py @@ -66,6 +66,21 @@ async def _async_get_hacs_json_raw(**_): ) +async def test_hacs_manifest_with_unsafe_path(repository, caplog): + repository.tree = test_tree + repository.data.category = "integration" + + async def _async_get_hacs_json_raw(**_): + return {"name": "test", "filename": "../../../evil.zip"} + + repository.get_hacs_json_raw = _async_get_hacs_json_raw + + check = Validator(repository) + await check.execute_validation() + assert check.failed + assert "'../../../evil.zip' is not a safe relative path" in caplog.text + + async def test_hacs_manifest_integration_zip_release_with_filename(repository): repository.tree = test_tree repository.data.category = "integration"