From 5fa6f9274af3b550ab60269b3616d8c6403c4c30 Mon Sep 17 00:00:00 2001 From: W0AEZ Date: Mon, 31 Aug 2026 18:37:04 -0600 Subject: [PATCH 1/2] fix(remote): symlink escape, decompression amplification, stalled requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the adversarial security review of the embedded server. 1. The opaque-id registry fences off client-supplied paths, but it is built by walking the gallery directories and Path.is_file() follows symlinks. A symlink dropped into images_save_dir or a gallery_extra_dir became a legitimate gallery id whose bytes the server would then serve — and gallery_extra_dirs is documented as pointing at a shared folder, which is exactly where an attacker can create one. image_path() now resolves the target and requires containment inside a configured gallery dir; thumbnail_path() inherits it. The entry still appears in the listing (filtering at scan time would cost a resolve() per file per scan); its bytes and thumbnail are refused, which is the security boundary. 2. /api/compose/render needs only the token, not the TX gate, and 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, with nothing bounding concurrent renders. open() is lazy, so the declared size is checked before load(). 3. BaseHTTPRequestHandler leaves timeout at None, so a client that sent half a header and stopped held a request thread forever — parked before auth runs, so no token needed. A few hundred such connections exhaust the thread pool and the fd limit and block the accept loop. Requests now time out at 15 s; that bounds reading the request, not writing an SSE response. Verified against the real server: the symlink target is refused, a 49 MP / 152 KB payload gets a 400, and a stalled connection is closed after 15 s. Two regression tests. Note the compose one nearly shipped meaningless — it passed without the fix because a bogus template id returned None before the decode was ever reached. It now uses a real template id and asserts a normal photo through the same call succeeds, so a None can only be the cap. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 16 ++++++++++++ src/open_sstv/remote/compose.py | 16 ++++++++++++ src/open_sstv/remote/server.py | 19 ++++++++++++++ src/open_sstv/remote/service.py | 28 +++++++++++++++++++++ tests/remote/test_compose.py | 42 +++++++++++++++++++++++++++++++ tests/remote/test_service.py | 44 +++++++++++++++++++++++++++++++++ 6 files changed, 165 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 75d3874..b83f8c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,22 @@ 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. + - **The remote web UI no longer scrolls sideways on narrower phones.** The header's own controls added up to about 408 px and flex items refuse to shrink below their content, so on a 375 or 390 px screen — iPhone SE, 12/13 diff --git a/src/open_sstv/remote/compose.py b/src/open_sstv/remote/compose.py index 79fcc3d..feff003 100644 --- a/src/open_sstv/remote/compose.py +++ b/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 diff --git a/src/open_sstv/remote/server.py b/src/open_sstv/remote/server.py index 3e5fdd6..0338ab8 100644 --- a/src/open_sstv/remote/server.py +++ b/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: diff --git a/src/open_sstv/remote/service.py b/src/open_sstv/remote/service.py index 71cf54e..c403f7d 100644 --- a/src/open_sstv/remote/service.py +++ b/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: diff --git a/tests/remote/test_compose.py b/tests/remote/test_compose.py index 3bcd0f3..bdcb6ab 100644 --- a/tests/remote/test_compose.py +++ b/tests/remote/test_compose.py @@ -128,3 +128,45 @@ 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" + + cfg = replace(AppConfig(), logbook_db_path=str(tmp_path / "no.db")) + svc = ComposeService(lambda: cfg) + # 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, "need at least one template for this test to mean anything" + 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 diff --git a/tests/remote/test_service.py b/tests/remote/test_service.py index c6ab600..d33369f 100644 --- a/tests/remote/test_service.py +++ b/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 From 61613851ec5d9c63394f64523edc939b8ac3bd75 Mon Sep 17 00:00:00 2001 From: W0AEZ Date: Mon, 31 Aug 2026 19:07:48 -0600 Subject: [PATCH 2/2] test(remote): stop the decompression test depending on installed templates It asserted that svc.list_templates() was non-empty, which held on a machine with the starter pack installed and failed on all 17 CI jobs where nothing had installed it. The test now installs the bundled starter pack into a temp dir and points the service at it, so it stands alone. Verified it still fails without the pixel-cap fix. Co-Authored-By: Claude Opus 4.8 --- tests/remote/test_compose.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/remote/test_compose.py b/tests/remote/test_compose.py index bdcb6ab..8c14091 100644 --- a/tests/remote/test_compose.py +++ b/tests/remote/test_compose.py @@ -155,12 +155,20 @@ def test_oversized_canvas_rejected_without_decoding(self, tmp_path) -> None: 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")) - svc = ComposeService(lambda: cfg) + # 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, "need at least one template for this test to mean anything" + 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