Security hardening (maziggy/bambuddy-security #22)

backend/app/services/external_camera.py

@@ -17,7 +17,7 @@ import shutil
 import socket
 from collections.abc import AsyncGenerator, Callable
 from pathlib import Path
-from urllib.parse import urlparse
+from urllib.parse import urljoin, urlparse
 
 import aiohttp
 
@@ -535,6 +535,51 @@ async def _capture_usb_frame(device: str, timeout: int) -> bytes | None:
         return None
 
 
+# Redirects a camera URL may take before the fetch gives up.
+_MAX_CAMERA_REDIRECTS = 5
+_REDIRECT_STATUSES = (301, 302, 303, 307, 308)
+
+
+def _with_same_camera_login(current: str, target: str) -> str:
+    """*target* with the login of *current* when it is the same camera (scheme,
+    host and port) and names no login of its own, as the HTTP client kept it."""
+    here, there = urlparse(current), urlparse(target)
+    if there.username or not here.username:
+        return target
+    if (here.scheme, here.hostname, here.port) != (there.scheme, there.hostname, there.port):
+        return target
+    return there._replace(netloc=f"{here.netloc.rsplit('@', 1)[0]}@{there.netloc}").geturl()
+
+
+@contextlib.asynccontextmanager
+async def _get_camera_url(session: aiohttp.ClientSession, url: str, schemes: tuple[str, ...]):
+    """GET a sanitized camera URL, following redirects only where the camera
+    guard allows: each destination is checked as the URL itself was. Yields
+    the response, or None when a redirect goes somewhere refused or never ends.
+    """
+    current = url
+    for _ in range(_MAX_CAMERA_REDIRECTS + 1):
+        async with session.get(current, allow_redirects=False) as response:
+            location = response.headers.get("Location") if response.status in _REDIRECT_STATUSES else None
+            if location is None:
+                yield response
+                return
+        try:
+            target = _sanitize_camera_url(_with_same_camera_login(current, urljoin(current, location)), schemes)
+        except ValueError:
+            # A malformed Location (bad IPv6 literal, port out of range).
+            target = None
+        if target is None:
+            logger.error(
+                "Camera URL redirected to a refused destination: %s", (redact_url_credentials(location) or "")[:50]
+            )
+            yield None
+            return
+        current = target
+    logger.error("Camera URL redirected more than %d times", _MAX_CAMERA_REDIRECTS)
+    yield None
+
+
 async def _capture_mjpeg_frame(url: str, timeout: int) -> bytes | None:
     """Extract a single representative frame from an MJPEG stream.
 
@@ -564,9 +609,12 @@ async def _capture_mjpeg_frame(url: str, timeout: int) -> bytes | None:
 
     try:
         async with (
+            asyncio.timeout(timeout),
             aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=timeout)) as session,
-            session.get(safe_url) as response,
+            _get_camera_url(session, safe_url, ("http", "https")) as response,
         ):
+            if response is None:
+                return None
             if response.status != 200:
                 logger.error("MJPEG stream returned status %s", response.status)
                 return None
@@ -735,8 +783,8 @@ def _transcode_to_jpeg(data: bytes) -> bytes | None:
     (plate detection, Obico, finish photo) working.
 
     Returns None if the bytes are not a decodable image (e.g. an HTML error page)
-    or if the imaging libraries are unavailable — callers fall back to the raw
-    bytes so behaviour is never worse than before.
+    or if the imaging libraries are unavailable; the caller then has no image.
+    The raw bytes are not passed on: a URL's response is not a camera frame.
     """
     try:
         import cv2
@@ -770,10 +818,15 @@ async def _capture_snapshot(url: str, timeout: int) -> bytes | None:
         return None
 
     try:
+        # One deadline for every redirect and the read, as the client's own
+        # timeout covered them when it followed redirects itself.
         async with (
+            asyncio.timeout(timeout),
             aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=timeout)) as session,
-            session.get(safe_url) as response,
+            _get_camera_url(session, safe_url, ("http", "https")) as response,
         ):
+            if response is None:
+                return None
             if response.status != 200:
                 logger.error("Snapshot URL returned status %s", response.status)
                 return None
@@ -804,15 +857,15 @@ async def _capture_snapshot(url: str, timeout: int) -> bytes | None:
         return transcoded
 
     # Couldn't decode it as an image at all — most likely not an image response
-    # (HTML error page, auth redirect, wrong URL). Return the raw bytes as a last
-    # resort (unchanged behaviour) but log enough to debug.
+    # (HTML error page, auth redirect, wrong URL). Not passed on: whatever a URL
+    # returns would reach the viewer of the camera stream.
     logger.warning(
         "External camera snapshot is not a decodable image "
         "(%d bytes, header %s) — verify the camera URL returns an image",
         len(data),
         data[:4].hex(),
     )
-    return data
+    return None
 
 
 async def test_connection(url: str, camera_type: str) -> dict:
@@ -1014,7 +1067,12 @@ async def _stream_mjpeg(url: str) -> AsyncGenerator[bytes, None]:
 
     try:
         timeout = aiohttp.ClientTimeout(total=None, sock_read=30)
-        async with aiohttp.ClientSession(timeout=timeout) as session, session.get(safe_url) as response:
+        async with (
+            aiohttp.ClientSession(timeout=timeout) as session,
+            _get_camera_url(session, safe_url, ("http", "https")) as response,
+        ):
+            if response is None:
+                return
             if response.status != 200:
                 logger.error("MJPEG stream returned status %s", response.status)
                 return

backend/tests/unit/services/test_external_camera.py

@@ -30,6 +30,7 @@ class _FakeMjpegResponse:
         self._chunks = list(chunks)
         self._raise_after = raise_after
         self._raise_exc = raise_exc
+        self.headers = {}
         self.content = self  # the function calls `response.content.iter_chunked(...)`
 
     def iter_chunked(self, _size):  # noqa: ARG002 — chunk size is informational
@@ -59,7 +60,7 @@ class _FakeMjpegSession:
     def __init__(self, response):
         self._response = response
 
-    def get(self, _url):
+    def get(self, _url, **_kwargs):
         return self._response
 
     async def __aenter__(self):
@@ -615,6 +616,7 @@ def _fake_snapshot_session(body: bytes, status: int = 200):
     class _Resp:
         def __init__(self):
             self.status = status
+            self.headers = {}
 
         async def __aenter__(self):
             return self
@@ -635,7 +637,7 @@ def _fake_snapshot_session(body: bytes, status: int = 200):
         async def __aexit__(self, *a):
             return False
 
-        def get(self, _url):
+        def get(self, _url, **_kwargs):
             return _Resp()
 
     return _Session
@@ -696,12 +698,12 @@ class TestSnapshotTranscode:
         assert out == jpeg  # identical object bytes — proves no transcode ran
 
     @pytest.mark.asyncio
-    async def test_capture_snapshot_non_image_falls_back_to_raw(self):
-        """Undecodable (non-image) responses return the raw bytes unchanged, so
-        behaviour is never worse than before the fix."""
+    async def test_capture_snapshot_non_image_is_not_passed_on(self):
+        """An undecodable (non-image) response is no frame: whatever a URL
+        returns would otherwise reach the viewer of the camera stream."""
         from backend.app.services import external_camera as ec
 
         html = b"<html><body>unauthorized</body></html>"
         with patch.object(ec.aiohttp, "ClientSession", _fake_snapshot_session(html)):
             out = await ec._capture_snapshot("http://192.168.50.50/snapshot", 10)
-        assert out == html
+        assert out is None

backend/tests/unit/services/test_external_camera_redirects.py

@@ -0,0 +1,241 @@
+"""A camera URL's redirects are checked like the URL itself.
+
+The configured URL goes through the camera guard, but the HTTP client followed
+redirects on its own, so a camera host could send a fetch anywhere the guard
+refuses: the cloud metadata endpoint, the host itself. A snapshot that is not
+an image is not passed on either: it would hand whatever a URL returns to the
+viewer of the camera stream.
+"""
+
+from __future__ import annotations
+
+from unittest.mock import patch
+
+import pytest
+
+from backend.app.services import external_camera
+
+JPEG = b"\xff\xd8" + b"\x00" * 16 + b"\xff\xd9"
+CAMERA = "http://192.168.1.50/snapshot"
+
+
+class _Response:
+    def __init__(self, status: int, body: bytes = b"", location: str | None = None):
+        self.status = status
+        self._body = body
+        self.headers = {"Location": location} if location else {}
+        self.content = self
+
+    async def read(self) -> bytes:
+        return self._body
+
+    async def iter_chunked(self, _size):
+        yield self._body
+
+    async def __aenter__(self):
+        return self
+
+    async def __aexit__(self, *_exc):
+        return False
+
+
+class _Session:
+    """aiohttp-like: follows redirects itself unless told not to."""
+
+    def __init__(self, routes: dict[str, _Response], visited: list[str]):
+        self.routes = routes
+        self.visited = visited
+
+    def get(self, url, allow_redirects=True, **_kwargs):
+        self.visited.append(url)
+        response = self.routes.get(url, _Response(404))
+        hops = 0
+        while allow_redirects and response.status in (301, 302, 303, 307, 308) and hops < 10:
+            url = response.headers["Location"]
+            self.visited.append(url)
+            response = self.routes.get(url, _Response(404))
+            hops += 1
+        return response
+
+    async def __aenter__(self):
+        return self
+
+    async def __aexit__(self, *_exc):
+        return False
+
+
+def _session(routes: dict[str, _Response]):
+    visited: list[str] = []
+    factory = patch.object(external_camera.aiohttp, "ClientSession", lambda *a, **k: _Session(routes, visited))
+    return factory, visited
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize(
+    "target",
+    [
+        "http://169.254.169.254/latest/meta-data/",
+        "http://127.0.0.1:8000/api/v1/settings",
+        "http://metadata.google.internal/computeMetadata/v1/",
+        "file:///etc/passwd",
+    ],
+)
+@pytest.mark.parametrize("fetch", ["snapshot", "mjpeg_frame"])
+async def test_a_redirect_to_a_refused_destination_is_not_followed(target, fetch):
+    factory, visited = _session({CAMERA: _Response(302, location=target), target: _Response(200, JPEG)})
+
+    with factory:
+        if fetch == "snapshot":
+            frame = await external_camera._capture_snapshot(CAMERA, timeout=5)
+        else:
+            frame = await external_camera._capture_mjpeg_frame(CAMERA, timeout=5)
+
+    assert frame is None
+    assert target not in visited
+
+
+@pytest.mark.asyncio
+async def test_the_live_stream_does_not_follow_a_refused_redirect():
+    target = "http://169.254.169.254/latest/meta-data/"
+    factory, visited = _session({CAMERA: _Response(302, location=target), target: _Response(200, JPEG + JPEG)})
+
+    with factory:
+        frames = [frame async for frame in external_camera._stream_mjpeg(CAMERA)]
+
+    assert frames == []
+    assert target not in visited
+
+
+@pytest.mark.asyncio
+async def test_a_redirect_the_guard_allows_is_followed():
+    """A camera moving its snapshot, or to https, keeps working."""
+    target = "https://192.168.1.50/snapshot.jpg"
+    factory, visited = _session({CAMERA: _Response(302, location=target), target: _Response(200, JPEG)})
+
+    with factory:
+        frame = await external_camera._capture_snapshot(CAMERA, timeout=5)
+
+    assert frame == JPEG
+    assert visited == [CAMERA, target]
+
+
+@pytest.mark.asyncio
+async def test_a_relative_redirect_is_resolved_against_the_camera():
+    factory, visited = _session(
+        {CAMERA: _Response(302, location="/snap.jpg"), "http://192.168.1.50/snap.jpg": _Response(200, JPEG)}
+    )
+
+    with factory:
+        frame = await external_camera._capture_snapshot(CAMERA, timeout=5)
+
+    assert frame == JPEG
+
+
+@pytest.mark.asyncio
+async def test_endless_redirects_give_up():
+    a, b = "http://192.168.1.50/a", "http://192.168.1.50/b"
+    factory, visited = _session(
+        {CAMERA: _Response(302, location=a), a: _Response(302, location=b), b: _Response(302, location=a)}
+    )
+
+    with factory:
+        frame = await external_camera._capture_snapshot(CAMERA, timeout=5)
+
+    assert frame is None
+    assert len(visited) <= 7
+
+
+@pytest.mark.asyncio
+async def test_a_snapshot_that_is_not_an_image_is_not_passed_on():
+    factory, _visited = _session({CAMERA: _Response(200, b'{"AccessKeyId": "x"}')})
+
+    with factory:
+        frame = await external_camera._capture_snapshot(CAMERA, timeout=5)
+
+    assert frame is None
+
+
+@pytest.mark.asyncio
+async def test_the_camera_login_follows_a_redirect_on_the_same_camera():
+    """As the HTTP client did: an absolute redirect to the same scheme, host and
+    port keeps the login from the camera URL."""
+    camera = "http://admin:pw@192.168.1.50/snapshot"
+    factory, visited = _session(
+        {
+            camera: _Response(302, location="http://192.168.1.50/snap.jpg"),
+            "http://admin:pw@192.168.1.50/snap.jpg": _Response(200, JPEG),
+        }
+    )
+
+    with factory:
+        frame = await external_camera._capture_snapshot(camera, timeout=5)
+
+    assert frame == JPEG
+    assert visited[-1] == "http://admin:pw@192.168.1.50/snap.jpg"
+
+
+@pytest.mark.asyncio
+async def test_the_camera_login_does_not_follow_a_redirect_elsewhere():
+    camera = "http://admin:pw@192.168.1.50/snapshot"
+    factory, visited = _session(
+        {
+            camera: _Response(302, location="http://192.168.1.60/snap.jpg"),
+            "http://192.168.1.60/snap.jpg": _Response(200, JPEG),
+        }
+    )
+
+    with factory:
+        frame = await external_camera._capture_snapshot(camera, timeout=5)
+
+    assert frame == JPEG
+    assert visited[-1] == "http://192.168.1.60/snap.jpg"
+
+
+@pytest.mark.asyncio
+async def test_one_deadline_covers_every_redirect():
+    """Each hop is slow but under the timeout; together they are not."""
+    import asyncio
+
+    class _Slow(_Session):
+        def get(self, url, allow_redirects=True, **_kwargs):
+            response = super().get(url, allow_redirects=allow_redirects)
+
+            class _Delayed:
+                async def __aenter__(self):
+                    await asyncio.sleep(0.4)
+                    return response
+
+                async def __aexit__(self, *_exc):
+                    return False
+
+            return _Delayed()
+
+    hops = {f"http://192.168.1.50/{i}": _Response(302, location=f"http://192.168.1.50/{i + 1}") for i in range(4)}
+    hops["http://192.168.1.50/4"] = _Response(200, JPEG)
+    visited: list[str] = []
+    with patch.object(external_camera.aiohttp, "ClientSession", lambda *a, **k: _Slow(hops, visited)):
+        started = asyncio.get_running_loop().time()
+        frame = await external_camera._capture_snapshot("http://192.168.1.50/0", timeout=1)
+        elapsed = asyncio.get_running_loop().time() - started
+
+    assert frame is None
+    assert elapsed < 1.5
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize(
+    ("camera", "location"),
+    [
+        (CAMERA, "http://[::1"),
+        (CAMERA, "//[zz/"),
+        ("http://admin:pw@192.168.1.50/snapshot", "http://192.168.1.50:99999/x"),
+    ],
+)
+async def test_a_malformed_redirect_is_refused_not_raised(camera, location):
+    factory, _visited = _session({camera: _Response(302, location=location)})
+
+    with factory:
+        frame = await external_camera._capture_snapshot(camera, timeout=5)
+        frames = [f async for f in external_camera._stream_mjpeg(camera)]
+
+    assert frame is None and frames == []