diff --git a/custom_components/hacs/repositories/base.py b/custom_components/hacs/repositories/base.py index 43fa1967501..751027e0651 100644 --- a/custom_components/hacs/repositories/base.py +++ b/custom_components/hacs/repositories/base.py @@ -36,7 +36,7 @@ from ..utils.github_graphql_query import GET_REPOSITORY_RELEASES 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 @@ -255,6 +255,16 @@ 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. + for key in ("filename", "persistent_directory"): + value = getattr(manifest_data, key) + if value is not None and not is_safe_relative_path(value): + LOGGER.warning("Ignoring unsafe %s value '%s' in the HACS manifest", key, value) + setattr(manifest_data, key, None) + manifest_data.manifest.pop(key, None) + return manifest_data def update_data(self, data: dict) -> 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/repositories/test_hacs_manifest.py b/tests/repositories/test_hacs_manifest.py index e1f44f95937..b011b8811b3 100644 --- a/tests/repositories/test_hacs_manifest.py +++ b/tests/repositories/test_hacs_manifest.py @@ -42,3 +42,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_are_ignored(key: str, caplog: pytest.LogCaptureFixture): + manifest = HacsManifest.from_dict({"name": "TEST", key: "../../../evil"}) + + assert getattr(manifest, key) is None + assert key not in manifest.manifest + assert f"Ignoring unsafe {key} value '../../../evil' in the HACS manifest" in caplog.text + + +@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_are_ignored(key: str, value, caplog: pytest.LogCaptureFixture): + manifest = HacsManifest.from_dict({"name": "TEST", key: value}) + + assert getattr(manifest, key) is None + assert key not in manifest.manifest + assert f"Ignoring unsafe {key} value '{value}' in the HACS manifest" in caplog.text 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 2fb9e678c5d..5b6ac2f963d 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."""