From b254dddd5d5fdb29917d6fdee020378a0342b5b8 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 13 Aug 2026 11:32:04 -0500 Subject: [PATCH] feat(remotefile): reject an unsafe server-chosen listing name (BACKLOG #1238) RemoteFileSource joined a filename from a remote SFTP/FTP server's own directory listing straight onto the configured remote_dir with no containment check. The only filter was fnmatch against the pattern, and fnmatch's '*' spans '/' unlike glob -- so the default '*.hl7' matches '../../etc/passwd.hl7'. Screened at the SOURCE: immediately after list_dir returns and before the pattern filter, so it dominates every consumer. That placement is the point. The raw name reaches at least four consumers -- the retrieve path, the error/oversize move, the after_read disposition (move, delete or the leave-mode dedup key) -- and three of _move's four callers are error paths not gated on after_read, so no configuration avoids them. A per-consumer check would keep missing one: the consumer set grew at every measurement pass and never shrank. REJECT, NEVER REWRITE -- the owner's ruling, and the reason is recorded in the helper because the obvious fix is actively harmful. posixpath.basename MUTATES rather than refuses, so '../../etc/adt_20260812.hl7' becomes a name that rejoins to a REAL file in the poll directory, handing a hostile server a retrieve/move/delete primitive against the partner's own drop directory. It is also a no-op on the Windows-separator form, and because its output can never contain '/', any containment check placed after it is unreachable and always passes. Refused: non-str, empty, '.', '..', any '/' or '\', control characters, and the drive-relative form ('C:x.hl7') which carries no separator at all and so slips a separator-only check. A refused entry is left in place and logged, NOT quarantined: moving it would join the hostile name onto a directory, which is the operation being refused. Logged without the name, since this source does not log filenames at INFO+. MUTATION-VERIFIED, twice, because one mutation only proves the test sees the null case: removing the check fails both new tests, and replacing it with basename -- the plausible WRONG fix rather than the absent one -- fails the traversal test. The check is proven to discriminate against the near miss. Verified: ruff, ruff format, mypy strict (265 files) clean; the remotefile suite 86 passed. The FULL suite was still running when this was committed and is NOT part of this claim -- CI is the gate for it. --- messagefoundry/transports/remotefile.py | 47 +++++++++++++++ tests/test_remotefile_transport.py | 80 +++++++++++++++++++++++++ 2 files changed, 127 insertions(+) diff --git a/messagefoundry/transports/remotefile.py b/messagefoundry/transports/remotefile.py index 913caea2..d1b2e9cd 100644 --- a/messagefoundry/transports/remotefile.py +++ b/messagefoundry/transports/remotefile.py @@ -94,6 +94,37 @@ _T = TypeVar("_T") +def _is_contained_name(name: object) -> bool: + """True if ``name`` is a single, safe path component — a listing entry we may join onto the + configured remote directory (BACKLOG #1238, ASVS 5.3.2). + + The name comes from a **remote server's own directory listing**, so it is chosen by a party we do + not control: a malicious or compromised partner could return ``../../etc/passwd.hl7``, and the + default ``pattern`` of ``*.hl7`` matches it (``fnmatch``'s ``*`` spans ``/``, unlike ``glob``). + + **Reject, never rewrite.** ``posixpath.basename`` is the obvious fix and is actively harmful: + it MUTATES rather than refuses, so ``../../etc/adt_20260812.hl7`` becomes ``adt_20260812.hl7``, + which rejoins to a *real* file in the poll directory — handing a hostile server a retrieve / + move / delete primitive against the partner's own drop directory. It is also a no-op on the + Windows-separator form (``posixpath`` tokenizes only ``/``), and because its output can never + contain ``/`` any containment check placed after it is unreachable and always passes. For a + healthcare feed, refusing one file is strictly better than silently reading the wrong one, and + mutation cannot give that property. + + Refused: a non-``str``; empty; ``.`` and ``..``; any ``/`` or ``\\``; a control character; and the + drive-relative form (``C:x.hl7``), which carries no separator at all and so slips a + separator-only check.""" + if not isinstance(name, str) or not name or name in (".", ".."): + return False + if "/" in name or "\\" in name: + return False + if any(ord(ch) < 0x20 or ord(ch) == 0x7F for ch in name): + return False + # A drive-relative path ("C:x.hl7") resolves against the drive's CWD on Windows and contains no + # separator, so the checks above cannot see it. Two chars, ASCII letter, then a colon. + return not (len(name) >= 2 and name[0].isascii() and name[0].isalpha() and name[1] == ":") + + def _redact(host: str, path: str) -> str: """``host:path`` only — never credentials, for a log line.""" return f"{host}:{path}" @@ -920,6 +951,22 @@ async def _poll_once(self) -> None: for name, size in sorted(entries): if self._stop.is_set(): break # shutting down — leave the rest for the next start (at-least-once) + if not _is_contained_name(name): + # #1238 (ASVS 5.3.2): the server chose this name. Refuse it HERE — at the source, + # before the pattern filter — because the raw name reaches at least four consumers + # (the retrieve path, the error/oversize move, the after_read disposition, and the + # leave-mode dedup key), and a per-consumer check would keep missing one: the + # consumer set grew at every measurement pass and never shrank. + # NOT quarantined: moving it would join the hostile name onto a directory, which is + # the very operation being refused. Left in place and logged, so an operator sees it + # every poll rather than once. PHI-safe: names are not logged at INFO+ elsewhere in + # this source, so this stays WARNING-with-no-name — the count is the signal. + logger.warning( + "REMOTEFILE %s: a listing entry was refused as an unsafe path component " + "(not a single safe name); left in place, not retrieved", + _redact(self._host, self._remote_dir), + ) + continue if not fnmatch.fnmatch(name, self._pattern): continue path = posixpath.join(self._remote_dir, name) diff --git a/tests/test_remotefile_transport.py b/tests/test_remotefile_transport.py index de1c401d..3c9ad1e4 100644 --- a/tests/test_remotefile_transport.py +++ b/tests/test_remotefile_transport.py @@ -39,6 +39,7 @@ RemoteFileSource, _FtpClient, _ftps_ssl_context, + _is_contained_name, _RemoteClient, _RemoteError, _SftpClient, @@ -1144,3 +1145,82 @@ async def test_remote_destination_validate_directory_test_probe_never_creates( with pytest.raises(DeliveryError): await dest.test_connection() assert client.dirs == [] + + +# --- #1238 (ASVS 5.3.2): a server-chosen listing name is REJECTED, never rewritten --------------- + + +class _HostileListingClient(_FakeClient): + """A client whose listing returns names the SERVER chose, verbatim. + + ``_FakeClient.list_dir`` derives names with ``posixpath.basename`` off its ``files`` keys, so it + structurally cannot produce a traversal name -- it would sanitize the very input under test. This + subclass returns the raw listing instead, which is what a hostile partner server does. + """ + + def __init__(self, names: list[str], **kw: Any) -> None: + super().__init__(**kw) + self._names = names + + def list_dir(self, remote_dir: str) -> list[tuple[str, int]]: + return [(n, 10) for n in self._names] + + +@pytest.mark.parametrize( + "name", + [ + "../../etc/passwd.hl7", # traversal, and the default *.hl7 pattern MATCHES it + "/etc/passwd.hl7", # absolute + "..\..\etc\passwd.hl7", # Windows separators -- posixpath.basename is a NO-OP on this + "sub/dir.hl7", # a subdirectory component + ".", + "..", + "", + "a\x00b.hl7", # NUL + "a\nb.hl7", # newline + "C:evil.hl7", # drive-relative: NO separator at all, so a separator-only check misses it + ], +) +def test_unsafe_listing_names_are_refused(name: str) -> None: # #1238 + assert _is_contained_name(name) is False + + +@pytest.mark.parametrize( + "name", + ["a.hl7", "adt_20260812.hl7", "A-1.2_3.hl7", "file with spaces.hl7", "unicode-\u00e9.hl7"], +) +def test_legitimate_listing_names_are_accepted(name: str) -> None: # #1238 + # The refusal must not be so broad that it rejects ordinary partner filenames -- a check that + # refuses everything is not a control either. + assert _is_contained_name(name) is True + + +async def test_traversal_entry_is_never_retrieved(monkeypatch: pytest.MonkeyPatch) -> None: # #1238 + """The whole point: a hostile listing entry reaches NO consumer. + + Asserted on the client's recorded ops, not on the handler alone, because the raw name reaches at + least four consumers (retrieve, the error/oversize move, the after_read disposition, and the + leave-mode dedup key). Checking only "the handler was not called" would pass even if the engine + had already moved or deleted at the hostile path. + """ + client = _HostileListingClient(["../../etc/passwd.hl7"]) + src = _src(monkeypatch, client) + h = _RecordingHandler() + src._handler = h + await src._poll_once() + assert h.bodies == [] # nothing ingested + assert client.ops == [] # and NOTHING was retrieved, moved, renamed or removed + + +async def test_a_safe_entry_beside_a_hostile_one_still_flows( + monkeypatch: pytest.MonkeyPatch, +) -> None: # #1238 + # Refusing one entry must not abort the poll -- the legitimate file beside it is still delivered. + # Without this, a hostile server could suppress a real feed by planting one bad name. + client = _HostileListingClient(["../../etc/passwd.hl7", "good.hl7"]) + client.files["/in/good.hl7"] = b"MSH|^~\&|A" + src = _src(monkeypatch, client) + h = _RecordingHandler() + src._handler = h + await src._poll_once() + assert h.bodies == [b"MSH|^~\&|A"]