fix(remote): symlink escape, decompression amplification, stalled requests

CHANGELOG.md

@@ -11,6 +11,21 @@ Versions follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### Fixed
 
+- **Remote web server hardening.** Three issues found by an adversarial
+  security review of the embedded server:
+  - A symlink placed in a watched gallery folder became a readable gallery
+    entry, so its target — anywhere on disk — could be fetched over the
+    network. Image paths are now required to resolve inside a configured
+    gallery directory. (`gallery_extra_dirs` is explicitly meant for shared
+    folders, which is where this mattered.)
+  - A small upload declaring an enormous canvas sailed past the 32 MP cap,
+    because Pillow only *rejects* above twice the limit and merely warns
+    below that. A ~150 KB file could allocate hundreds of megabytes, with
+    nothing bounding concurrent renders. Dimensions are now checked before
+    decoding.
+  - A client that sent half a request and stopped held a server thread
+    indefinitely, before authentication, so no token was needed. Requests
+    now time out.
 - **Switching audio devices no longer corrupts an in-flight decode.** The
   audio-worker hot-swap called the RX decoder's reset directly from the GUI
   thread, while a flush triggered by the same event was still decoding on

src/open_sstv/remote/compose.py

@@ -24,6 +24,7 @@
 from typing import TYPE_CHECKING
 
 from open_sstv.core.modes import MODE_TABLE, Mode
+from open_sstv.security import MAX_IMAGE_PIXELS
 from open_sstv.templates import manager as template_manager
 from open_sstv.templates.model import QSOState, TXContext
 from open_sstv.templates.renderer import render_template
@@ -118,6 +119,21 @@ def render(
             return None
         try:
             photo = PIL.Image.open(io.BytesIO(photo_bytes))
+            # Check the declared size before decoding.  Pillow's
+            # MAX_IMAGE_PIXELS only *raises* above 2x the limit — at 1x-2x it
+            # merely warns and decodes anyway — so a ~150 KB solid-colour PNG
+            # declaring 7000x7000 sailed past the 32 MP cap and materialised
+            # hundreds of MB of pixels, with nothing bounding concurrent
+            # renders.  open() is lazy, so the dimensions are known here
+            # without having decoded anything.
+            pixels = photo.size[0] * photo.size[1]
+            if pixels > MAX_IMAGE_PIXELS:
+                _log.warning(
+                    "compose: rejecting %dx%d photo (%.1f MP > %.1f MP cap)",
+                    photo.size[0], photo.size[1],
+                    pixels / 1e6, MAX_IMAGE_PIXELS / 1e6,
+                )
+                return None
             photo.load()
             # Phone photos carry their orientation in an EXIF tag rather than
             # in the pixels; bake it in so the composited frame is upright

src/open_sstv/remote/server.py

@@ -201,6 +201,25 @@ def start(self) -> None:
         compose = self._compose
 
         class _Handler(BaseHTTPRequestHandler):
+            #: Drop a connection that stalls part-way through its request.
+            #: BaseHTTPRequestHandler leaves this at None, so a client that
+            #: sent half a header line and then stopped held a request
+            #: thread forever — and did so *before* auth runs, so it needed
+            #: no token.  A few hundred such connections exhaust the thread
+            #: pool and the file-descriptor limit, blocking the accept loop
+            #: for the operator's own phone.  SSE responses are unaffected:
+            #: this bounds the read of the request, not the life of a
+            #: response we are writing.
+            timeout = 15.0
+
+            def handle_one_request(self) -> None:  # noqa: D102
+                try:
+                    super().handle_one_request()
+                except TimeoutError:
+                    # Stalled client: close quietly rather than logging a
+                    # traceback per abandoned connection.
+                    self.close_connection = True
+
             # Route stdlib access logs through our logger at DEBUG rather
             # than spamming stderr with one line per request.
             def log_message(self, fmt: str, *args: object) -> None:

src/open_sstv/remote/service.py

@@ -228,11 +228,39 @@ def resolve(self, image_id: str) -> Path | None:
         with self._lock:
             return self._registry.get(image_id)
 
+    def _within_source_dirs(self, path: Path) -> bool:
+        """True if *path*'s real location is inside a configured gallery dir.
+
+        The id registry is the fence against client-supplied paths, but it
+        is built by walking the gallery directories, and ``Path.is_file()``
+        follows symlinks — so a symlink dropped into a watched folder
+        (``gallery_extra_dirs`` explicitly invites pointing at a shared
+        one) became a legitimate gallery id whose bytes this server would
+        then serve.  Resolve both sides and require containment.
+        """
+        try:
+            real = path.resolve(strict=True)
+        except OSError:
+            return False
+        for d in self._source_dirs():
+            try:
+                if real.is_relative_to(d.resolve(strict=True)):
+                    return True
+            except OSError:
+                continue
+        _log.warning(
+            "remote: refusing %s — resolves to %s, outside every gallery "
+            "directory (symlink?)", path, real,
+        )
+        return False
+
     def image_path(self, image_id: str) -> Path | None:
         """Resolve *image_id* to a still-present source file, or ``None``."""
         path = self.resolve(image_id)
         if path is None or not path.is_file():
             return None
+        if not self._within_source_dirs(path):
+            return None
         return path
 
     def thumbnail_path(self, image_id: str) -> Path | None:

tests/remote/test_compose.py

@@ -128,3 +128,53 @@ def test_staging_store_is_bounded(self, service: ComposeService) -> None:
 
     def test_gallery_id_is_not_staged(self, service: ComposeService) -> None:
         assert service.is_staged_id("ee5998afb64b94f6") is False
+
+
+class TestDecompressionCap:
+    """A small upload declaring a huge canvas must be refused before decode.
+
+    Pillow's MAX_IMAGE_PIXELS only *raises* above 2x the limit; between 1x
+    and 2x it warns and decodes anyway. So a ~150 KB solid-colour PNG
+    declaring 7000x7000 cleared the 32 MP cap and materialised hundreds of
+    MB of pixels per request, with nothing bounding concurrency.
+    """
+
+    def test_oversized_canvas_rejected_without_decoding(self, tmp_path) -> None:
+        import io
+        from dataclasses import replace
+
+        from PIL import Image
+
+        from open_sstv.config.schema import AppConfig
+        from open_sstv.remote.compose import ComposeService
+        from open_sstv.security import MAX_IMAGE_PIXELS
+
+        buf = io.BytesIO()
+        Image.new("RGB", (7000, 7000), (3, 5, 7)).save(buf, "PNG", compress_level=9)
+        payload = buf.getvalue()
+        assert 7000 * 7000 > MAX_IMAGE_PIXELS
+        assert len(payload) < 1_000_000, "payload should be small — that's the point"
+
+        from open_sstv.templates import manager as template_manager
+
+        cfg = replace(AppConfig(), logbook_db_path=str(tmp_path / "no.db"))
+        # Install the bundled starter templates into a temp dir and point the
+        # service at it.  Relying on whatever templates happen to exist on the
+        # machine made this pass locally and fail in CI, where there are none.
+        templates_dir = tmp_path / "templates"
+        templates_dir.mkdir()
+        template_manager.install_starter_pack(templates_dir)
+        svc = ComposeService(lambda: cfg, templates_dir=templates_dir)
+        # Use a REAL template id: _resolve() runs before the decode, so a
+        # bogus id would make this pass without ever exercising the cap.
+        templates = svc.list_templates()
+        assert templates, "starter pack should have installed at least one template"
+        template_id = templates[0]["id"]
+
+        # Sanity: a normal photo through the same call must succeed, so a
+        # None below can only be the pixel cap.
+        small = io.BytesIO()
+        Image.new("RGB", (320, 256), (9, 9, 9)).save(small, "PNG")
+        assert svc.render(small.getvalue(), template_id, {}, "scottie_s1") is not None
+
+        assert svc.render(payload, template_id, {}, "scottie_s1") is None

tests/remote/test_service.py

@@ -152,3 +152,47 @@ def test_lists_qsos_newest_first(self, tmp_path: Path) -> None:
         assert k1["rst_received"] == "595"
         # A logged image exposes the same opaque id the gallery uses.
         assert k1["image_id"] and svc.image_path(str(k1["image_id"])) == img
+
+
+class TestSymlinkContainment:
+    """A symlink in a gallery folder must not become a readable id.
+
+    The id registry fences off client-supplied paths, but it is built by
+    walking the gallery directories and ``Path.is_file()`` follows
+    symlinks — so ``leak.png -> ~/.ssh/id_rsa`` dropped into a watched
+    folder became a legitimate id whose bytes the server would serve.
+    ``gallery_extra_dirs`` explicitly invites watching a shared folder,
+    which is where that precondition stops being far-fetched.
+    """
+
+    def test_symlink_escaping_the_gallery_is_refused(self, tmp_path) -> None:
+        import os
+        from dataclasses import replace
+
+        from PIL import Image
+
+        from open_sstv.config.schema import AppConfig
+        from open_sstv.remote.service import GalleryService
+
+        gallery = tmp_path / "gallery"
+        gallery.mkdir()
+        outside = tmp_path / "secret.txt"
+        outside.write_text("PRIVATE KEY MATERIAL")
+        Image.new("RGB", (8, 8)).save(gallery / "real.png")
+        os.symlink(outside, gallery / "leak.png")
+
+        cfg = replace(
+            AppConfig(),
+            images_save_dir=str(gallery),
+            gallery_extra_dirs=[],
+            logbook_db_path=str(tmp_path / "no.db"),
+        )
+        svc = GalleryService(lambda: cfg)
+        svc.list_items()  # populates the id registry
+        by_name = {path.name: image_id for image_id, path in svc._registry.items()}
+        assert {"real.png", "leak.png"} <= set(by_name)
+
+        # The genuine image resolves; the escaping symlink does not.
+        assert svc.image_path(by_name["real.png"]) is not None
+        assert svc.image_path(by_name["leak.png"]) is None
+        assert svc.thumbnail_path(by_name["leak.png"]) is None