diff --git a/CHANGELOG.md b/CHANGELOG.md index c6ee8c0..3317dd2 100644 --- a/CHANGELOG.md +++ b/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 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 1d05fd8..222162d 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..8c14091 100644 --- a/tests/remote/test_compose.py +++ b/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 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