diff options
Diffstat (limited to 'packages/meshbay-node')
| -rw-r--r-- | packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py | 127 | ||||
| -rw-r--r-- | packages/meshbay-node/tests/test_security_regressions.py | 86 |
2 files changed, 184 insertions, 29 deletions
diff --git a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py index fe4e3c2..66ba7ef 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -106,6 +106,37 @@ UPLOAD_DIR_NAME = ".uploads" SAFE_UPLOAD_NAME = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._ -]{0,127}$") +def safe_subdir(shared_root: Path, rel: str) -> Path | None: + """ + Resolve a client-supplied directory under the shared root, or refuse. + + Uploads land where the member is looking now rather than in a per-user + quarantine, so the path arrives from the wire and every part of it has to be + checked: each segment against the same allowlist as filenames, and the + resolved result against the root. `..`, absolute paths, symlinks pointing + out, and anything with a separator in a segment are all refused here rather + than in the caller, so there is one place to get it right. + + The quarantine was the fix for C5a; what actually mattered in it — no + overwrite, a name allowlist, and confinement — is kept by this plus the + caller's existing checks. + """ + rel = (rel or "").strip().strip("/") + if not rel: + return shared_root + parts = [seg for seg in rel.split("/") if seg not in ("", ".")] + if any(seg == ".." or not SAFE_UPLOAD_NAME.match(seg) for seg in parts): + return None + try: + target = (shared_root / Path(*parts)).resolve() + root = shared_root.resolve() + except OSError: + return None + if target != root and root not in target.parents: + return None + return target + + def _extract_dtls_fingerprint(sdp: str) -> bytes: """Extract the DTLS SHA-256 fingerprint from SDP as raw 32 bytes.""" for line in sdp.splitlines(): @@ -301,6 +332,8 @@ class WebRTCPeerSession: self._do_chat_history(msg) elif mtype == MNP.FILE_UPLOAD: self._do_file_upload(msg) + elif mtype == MNP.DIR_CREATE: + self._do_dir_create(msg) elif mtype == MNP.FILE_DELETE: self._do_file_delete(msg) elif mtype == MNP.ADMIN_RESPONSE: @@ -834,6 +867,48 @@ class WebRTCPeerSession: self._send(reply) self._audit_join("gek_wrapped", f"group={group_id[:8]}") + def _do_dir_create(self, msg: dict) -> None: + """ + Create a directory, for any member of the group. + + Same confinement as an upload: every segment passes the name allowlist and + the result must resolve under the shared root. Making a directory is not a + privileged act — a member who can add a file can organise where it goes — + but it writes to the operator's disk, so it is audited like one. + """ + ctx = self._group_ctx() + shared_root = ctx.get("shared_root") + if not shared_root: + self._send({"type": "error", "detail": "No shared directory"}) + return + + name = str(msg.get("name", "")).strip() + if not SAFE_UPLOAD_NAME.match(name): + self._send({"type": "error", "detail": "Invalid directory name"}) + return + + parent = safe_subdir(shared_root, msg.get("dir") or "") + if parent is None or not parent.is_dir(): + self._send({"type": "error", "detail": "Invalid directory"}) + return + + target = safe_subdir(shared_root, f"{(msg.get('dir') or '').strip('/')}/{name}") + if target is None: + self._send({"type": "error", "detail": "Invalid directory"}) + return + if target.exists(): + self._send({"type": "error", "detail": "Already exists"}) + return + + target.mkdir(parents=False) + log.info("Directory created by %s: %s", self._user_id[:8], + target.relative_to(shared_root)) + self._audit("dir_create", str(target.relative_to(shared_root))) + self._send({ + "type": MNP.DIR_CREATE_ACK, "v": MNP_VERSION, + "dir": str(target.relative_to(shared_root)), + }) + async def _do_keypair_bundle_delete(self) -> None: """ Withdraw our own key backup from this node. @@ -918,8 +993,28 @@ class WebRTCPeerSession: "group_id": idx.group_id, "version": idx.version, "entries": entries, + # Directories are not index entries, so the client used to infer them + # from file paths — which means a folder someone just created, or one + # they emptied, simply did not exist as far as the UI was concerned. + "dirs": self._list_dirs(ctx.get("shared_root")), }) + @staticmethod + def _list_dirs(shared_root: Path | None) -> list[str]: + """Directories under the shared root, relative and sorted.""" + if not shared_root: + return [] + out = [] + try: + for path in sorted(shared_root.rglob("*")): + if path.is_dir() and not path.name.startswith("."): + rel = path.relative_to(shared_root) + if not any(part.startswith(".") for part in rel.parts): + out.append(str(rel)) + except OSError: + return [] + return out[:2000] + def _do_file_request(self, msg: dict) -> None: ctx = self._group_ctx() file_id = msg["file_id"] @@ -1120,21 +1215,31 @@ class WebRTCPeerSession: self._send({"type": "error", "detail": "No shared directory"}) return - # Per-user quarantine: a member can only ever write inside their own directory, - # so they cannot overwrite the operator's files or another member's (C5a). - rel_dir = f"{UPLOAD_DIR_NAME}/{self._user_id}" - user_dir = shared_root / UPLOAD_DIR_NAME / self._user_id - user_dir.mkdir(parents=True, exist_ok=True) - tmp_path = user_dir / f"{filename}.part" - final_path = user_dir / filename + # Where the member is looking, not a quarantine named after their user id. + # C5a is still honoured by what follows: the path is confined under the + # shared root (safe_subdir), the name passed the allowlist above, and an + # existing file is never overwritten — which is what made the quarantine + # necessary, since overwriting a file also made the attacker its recorded + # uploader and therefore able to delete it. + rel_dir = (msg.get("dir") or "").strip().strip("/") + target_dir = safe_subdir(shared_root, rel_dir) + if target_dir is None: + self._send({"type": "error", "detail": "Invalid directory"}) + return + if not target_dir.is_dir(): + self._send({"type": "error", "detail": "No such directory"}) + return + tmp_path = target_dir / f"{filename}.part" + final_path = target_dir / filename - state = self._uploads.get(filename) + upload_key = f"{rel_dir}/{filename}" + state = self._uploads.get(upload_key) if chunk_index == 0: if final_path.exists(): self._send({"type": "error", "detail": "File already exists"}) return state = {"next_index": 0, "bytes": 0} - self._uploads[filename] = state + self._uploads[upload_key] = state elif state is None: self._send({"type": "error", "detail": "Upload not started"}) return @@ -1151,7 +1256,7 @@ class WebRTCPeerSession: chunk_bytes = bytes(data) if state["bytes"] + len(chunk_bytes) > MAX_UPLOAD_BYTES: - self._uploads.pop(filename, None) + self._uploads.pop(upload_key, None) tmp_path.unlink(missing_ok=True) self._send({"type": "error", "detail": "Upload exceeds size limit"}) return @@ -1169,7 +1274,7 @@ class WebRTCPeerSession: }) if chunk_index + 1 >= total_chunks: - self._uploads.pop(filename, None) + self._uploads.pop(upload_key, None) tmp_path.rename(final_path) log.info("Upload complete: %s (%d chunks, %d bytes)", filename, total_chunks, state["bytes"]) diff --git a/packages/meshbay-node/tests/test_security_regressions.py b/packages/meshbay-node/tests/test_security_regressions.py index dcd9cf6..7627926 100644 --- a/packages/meshbay-node/tests/test_security_regressions.py +++ b/packages/meshbay-node/tests/test_security_regressions.py @@ -120,9 +120,14 @@ def _session(tmp_path: Path, user_id: str) -> WebRTCPeerSession: def test_upload_cannot_overwrite_another_members_file(tmp_path): """ C5a: uploads used to land in the shared root under a client-chosen name and - overwrite whatever was there. That let any member destroy the operator's files, - and — by becoming the recorded uploader of the replaced file — delete them - through the uploader path, bypassing the Ed25519 admin challenge entirely. + overwrite whatever was there. That let any member destroy the operator's + files, and — by becoming the recorded uploader of the replaced file — delete + them through the uploader path, bypassing the Ed25519 admin challenge. + + The per-user quarantine that fixed it was removed on 2026-08-14: files now go + where the member is looking, because a shared directory nobody can organise is + not a shared directory. What made the quarantine work is kept, and is what + this test now asserts — an existing file is never replaced. """ victim = _session(tmp_path, "victim-user") shared_root = victim._ctx["shared_root"] @@ -139,23 +144,14 @@ def test_upload_cannot_overwrite_another_members_file(tmp_path): }) assert original.read_bytes() == b"operator's original content" - uploaded = shared_root / ".uploads" / "attacker-user" / "important.mp4" - assert uploaded.exists(), "upload should be quarantined, not dropped" - assert uploaded.read_bytes() == b"attacker content" - - -def test_upload_rejects_out_of_order_chunks(tmp_path): - """C5a: chunk_index > 0 used to append blindly to any .part file on disk.""" - session = _session(tmp_path, "user-1") - session._do_file_upload({ - "filename": "movie.mp4", "chunk_index": 3, "total_chunks": 5, - "data": base64.b64encode(b"spliced").decode(), - }) - assert any(m.get("type") == "error" for m in session.sent) + assert any(m.get("type") == "error" for m in attacker.sent), ( + "the upload must be refused outright, not silently dropped") + assert not (shared_root / "important.mp4.part").exists(), ( + "a refused upload must leave nothing behind") def test_upload_second_attempt_cannot_replace_own_completed_file(tmp_path): - """C5a: even the original uploader goes through a fresh name, not an overwrite.""" + """C5a: even the original uploader does not get to overwrite.""" session = _session(tmp_path, "user-1") payload = {"filename": "movie.mp4", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(b"first").decode()} @@ -164,10 +160,64 @@ def test_upload_second_attempt_cannot_replace_own_completed_file(tmp_path): session._do_file_upload(dict(payload)) assert any(m.get("type") == "error" for m in session.sent) - stored = session._ctx["shared_root"] / ".uploads" / "user-1" / "movie.mp4" + stored = session._ctx["shared_root"] / "movie.mp4" assert stored.read_bytes() == b"first" +@pytest.mark.parametrize("bad_dir", [ + "..", "../..", "/etc", "a/../../b", "./../x", "sub/../../..", + "\\..\\..", "~", "a/./../..", +]) +def test_upload_cannot_escape_the_shared_root(tmp_path, bad_dir): + """ + The destination now arrives from the client, which is a path the node did not + choose. Every segment goes through the same allowlist as a filename and the + result must resolve inside the shared root. + """ + session = _session(tmp_path, "user-1") + outside = tmp_path / "outside.txt" + + session._do_file_upload({ + "filename": "outside.txt", "dir": bad_dir, + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"escaped").decode(), + }) + + assert any(m.get("type") == "error" for m in session.sent), bad_dir + assert not outside.exists(), f"upload escaped the shared root via {bad_dir!r}" + + +@pytest.mark.parametrize("bad", [ + {"dir": "..", "name": "evil"}, + {"dir": "", "name": ".."}, + {"dir": "", "name": "a/b"}, + {"dir": "/etc", "name": "evil"}, + {"dir": "", "name": ".hidden"}, +]) +def test_dir_create_cannot_escape_the_shared_root(tmp_path, bad): + """Creating a directory is not privileged, but it still writes to a disk.""" + session = _session(tmp_path, "user-1") + before = set(tmp_path.rglob("*")) + + session._do_dir_create(bad) + + assert any(m.get("type") == "error" for m in session.sent), bad + assert set(tmp_path.rglob("*")) == before, f"created something via {bad!r}" + + +def test_upload_still_refuses_unsafe_names_in_a_subdirectory(tmp_path): + """The name allowlist is not weakened by having somewhere to put the file.""" + session = _session(tmp_path, "user-1") + (session._ctx["shared_root"] / "docs").mkdir() + + session._do_file_upload({ + "filename": "../escape.txt", "dir": "docs", + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"x").decode(), + }) + assert any(m.get("type") == "error" for m in session.sent) + + # ── H1: group isolation ────────────────────────────────────────────────────── def test_chat_store_and_peers_are_per_group(tmp_path): |