Reject unsafe filesystem paths from the HACS manifest

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,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:

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(
+                "<HacsData async_restore_repository> %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

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("/")

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,

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")

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})

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
+    }
+}

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
+    }
+}

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

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."""

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"