Boundaries that were supposed to hold
The security pass. Six findings, none reachable by visiting the site and every one a boundary this codebase says it keeps. A subagent is pinned to a list of read-only commands, in every mode, unattended, with no card anybody could approve -- and `find *` was on it. find writes files with -fprintf, runs programs with -exec and removes them with -delete, and none of that needs a character the metacharacter guard refuses. A page the model had just read could ask for a helper and get a key into authorized_keys, from Plan mode, which promises to change nothing. Refused in `subject()` rather than trimmed from the list: a pattern cannot say "and no dangerous flags", and "this one looks read-only" is exactly what put find there. The loopback guard missed `0.0.0.0`, which is not is_loopback but does connect to localhost -- so it answered a *decided* False and skipped the DNS half too. The one spelling of "this machine" that walked past a guard whose whole job is that sentence. Twice in the update helper, which is the one place this deliberately crosses a privilege boundary: root ran a script the service account owns, and root sourced a file that account can replace. Either turns a compromise of the web application into root. The first needed no compromise at all -- a pull happens as the service user and root runs whatever it fetched, so control of the branch was control of root. The old test asserted that exact ExecStart line and had pinned it in place. Push endpoints skipped check_url, the only outbound request that did. And a chat could be filed in another account's folder, which hands over its system prompt -- `_new_chat` resolved the folder, discarded it when it was not the caller's, and stored the raw id anyway. An existing helper install keeps the old wiring until install.sh is re-run; update.sh now says so when it finds itself inside the checkout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -267,3 +267,22 @@ def test_a_draft_cannot_be_opened_against_a_refused_connection(
|
||||
profile = _profile(db)
|
||||
|
||||
assert client.get(f"/api/agents/{profile.id}/draft").status_code == 403
|
||||
|
||||
|
||||
def test_the_unspecified_address_is_this_machine_too():
|
||||
"""`0.0.0.0` and `::` are neither a real destination nor a refused one:
|
||||
connect() to either goes to loopback on Linux, so a profile pointed at
|
||||
`0.0.0.0` reached this host's own sshd.
|
||||
|
||||
`is_loopback` alone answered a decided **False**, which also short-circuited
|
||||
`resolves_here` -- so the DNS half never ran either, and the one spelling of
|
||||
"this machine" that mattered walked past a guard whose whole job is that
|
||||
sentence.
|
||||
"""
|
||||
for spelling in ("0.0.0.0", "::", "[::]", "0.0.0.0 "):
|
||||
assert hosts.is_loopback(spelling), spelling
|
||||
|
||||
|
||||
def test_a_real_address_is_still_not_this_machine():
|
||||
for spelling in ("192.168.1.5", "10.0.0.1", "example.com", "203.0.113.9"):
|
||||
assert not hosts.is_loopback(spelling), spelling
|
||||
|
||||
@@ -308,3 +308,57 @@ def test_an_mcp_tool_is_assumed_to_change_things(db):
|
||||
)
|
||||
db.commit()
|
||||
assert mcp_registry.tool_defs(db, None, everything=True)[0].risk == RISK_WRITE
|
||||
|
||||
|
||||
# --- A read-only name is not a read-only command --------------------------------
|
||||
@pytest.mark.parametrize(
|
||||
"line",
|
||||
[
|
||||
"find . -maxdepth 0 -fprintf /root/.ssh/authorized_keys 'ssh-ed25519 AAAA'",
|
||||
"find / -maxdepth 1 -exec /bin/sh /tmp/payload +",
|
||||
"find . -execdir /bin/sh {} +",
|
||||
"find . -delete",
|
||||
"find . -ok rm {} ;",
|
||||
"rg --pre /bin/sh pattern",
|
||||
"rg --pre=/bin/sh pattern",
|
||||
],
|
||||
)
|
||||
def test_a_command_that_writes_or_executes_matches_no_pattern(line):
|
||||
"""`_UNSAFE` stops a line being *composed* of two commands. It says nothing
|
||||
about one command that composes another itself, and the obvious read-only
|
||||
tools do: `find -exec` runs a program, `-fprintf` writes a file, `-delete`
|
||||
removes one, `rg --pre` runs a preprocessor per file. None needs a character
|
||||
`_UNSAFE` refuses.
|
||||
|
||||
That mattered because `find *` was on `SAFE_COMMANDS` -- the list a
|
||||
**subagent** is pinned to, in every mode, unattended, with no approval card
|
||||
possible. It was arbitrary write and arbitrary execution wearing a read-only
|
||||
name.
|
||||
"""
|
||||
assert policy.subject("shell_run", line) is None
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"line",
|
||||
[
|
||||
"find . -name '*.py'",
|
||||
"find src -type f",
|
||||
"grep -rn TODO src/",
|
||||
"rg --json pattern",
|
||||
"git log --oneline -5",
|
||||
"ls -la",
|
||||
],
|
||||
)
|
||||
def test_ordinary_reading_still_matches(line):
|
||||
"""Or the guard has taken the tool away rather than the escape."""
|
||||
assert policy.subject("shell_run", line) == line
|
||||
|
||||
|
||||
def test_the_subagent_allow_list_is_all_reachable():
|
||||
"""Every entry on `SAFE_COMMANDS` has to still resolve, or the list quietly
|
||||
promises a helper something it cannot do."""
|
||||
from lembas.services.subagent import SAFE_COMMANDS
|
||||
|
||||
for entry in SAFE_COMMANDS:
|
||||
sample = entry.replace("*", "x").strip()
|
||||
assert policy.subject("shell_run", sample) is not None, entry
|
||||
|
||||
@@ -436,3 +436,41 @@ def test_moving_respects_the_depth_cap(client: TestClient, db, registered):
|
||||
refused = client.patch(f"/api/folders/{loose}", data={"parent_id": chain[-1]})
|
||||
assert refused.status_code == 400
|
||||
assert db.get(Folder, loose).parent_id is None
|
||||
|
||||
|
||||
def test_a_chat_cannot_be_put_in_somebody_elses_folder(client: TestClient, db, registered):
|
||||
"""A folder is not just a label. `effective_system_prompt` walks up from the
|
||||
chat through its folder and that folder's parents, so a chat attached to
|
||||
another account's folder would take their system prompt -- a setting read
|
||||
across an ownership boundary through a field that looks like a tag.
|
||||
|
||||
Both paths had it. `_new_chat` resolved the folder, discarded it when it was
|
||||
not the caller's, and then stored the **raw id** anyway -- so the check
|
||||
governed which seeds were applied and not where the chat went.
|
||||
"""
|
||||
from lembas.security.passwords import hash_password
|
||||
|
||||
stranger = User(name="Sam", email="sam@shire.test", password_hash=hash_password("x"))
|
||||
db.add(stranger)
|
||||
db.commit()
|
||||
theirs = Folder(user_id=stranger.id, name="Private", system_prompt="Speak as Sam.")
|
||||
db.add(theirs)
|
||||
db.commit()
|
||||
|
||||
mine = db.scalar(select(User).where(User.email != "sam@shire.test"))
|
||||
chat = Chat(user_id=mine.id, model_id="m")
|
||||
db.add(chat)
|
||||
db.commit()
|
||||
|
||||
client.patch(f"/api/chats/{chat.id}", data={"folder_id": theirs.id})
|
||||
db.refresh(chat)
|
||||
assert chat.folder_id is None
|
||||
|
||||
started = client.post(
|
||||
"/api/chats/start",
|
||||
data={"content": "hello", "folder_id": theirs.id},
|
||||
follow_redirects=False,
|
||||
)
|
||||
assert started.status_code in (200, 204, 303)
|
||||
made = db.scalars(select(Chat).where(Chat.id != chat.id)).all()
|
||||
assert all(c.folder_id != theirs.id for c in made)
|
||||
|
||||
@@ -28,6 +28,46 @@ from lembas.services import push as push_service
|
||||
from lembas.services import reports as reports_service
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def resolvable_push_service(monkeypatch):
|
||||
"""`push.example` is not a real host, and `check_url` resolves.
|
||||
|
||||
The endpoint a browser hands back is now checked by the SSRF guard on both
|
||||
sides -- at subscribe, and again before the POST, because the row outlives
|
||||
the first check. That guard does DNS, so every fixture endpoint in this file
|
||||
would be refused for not existing rather than for being anywhere bad.
|
||||
|
||||
Stood in with something that keeps the part under test: the *shape* of the
|
||||
refusal. A literal private or loopback address is still refused, so the
|
||||
tests that assert the guard is wired in are asserting the real thing.
|
||||
"""
|
||||
import ipaddress
|
||||
from urllib.parse import urlsplit
|
||||
|
||||
from lembas.services import fetch as real_fetch
|
||||
|
||||
def stand_in(url: str, *, allow_private: bool = False) -> str:
|
||||
host = (urlsplit(url).hostname or "").strip("[]")
|
||||
try:
|
||||
address = ipaddress.ip_address(host)
|
||||
except ValueError:
|
||||
if host.endswith(".example"):
|
||||
return url
|
||||
return real_fetch.check_url(url, allow_private=allow_private)
|
||||
if not allow_private and (
|
||||
address.is_loopback
|
||||
or address.is_private
|
||||
or address.is_link_local
|
||||
or address.is_unspecified
|
||||
):
|
||||
raise real_fetch.FetchError("That address is not reachable from here.")
|
||||
return url
|
||||
|
||||
for module in ("lembas.api.push", "lembas.services.push"):
|
||||
monkeypatch.setattr(f"{module}.fetch_service.check_url", stand_in)
|
||||
return stand_in
|
||||
|
||||
|
||||
# --- A browser, standing in for one --------------------------------------------
|
||||
class Browser:
|
||||
"""The client half of RFC 8291, written out rather than imported.
|
||||
@@ -404,3 +444,49 @@ def test_the_poll_announces_the_messages_conversation(client: TestClient, db, re
|
||||
# And it is recorded as said, so the next tick does not say it again.
|
||||
db.expire_all()
|
||||
assert db.get(Chat, conversation.id).unread_notified
|
||||
|
||||
|
||||
def test_a_push_endpoint_pointing_inside_the_network_is_refused(client, db, registered):
|
||||
"""The endpoint is a URL the browser hands us and the server later POSTs to,
|
||||
and it was the one outbound client not going through the SSRF guard --
|
||||
`https://` alone says nothing about *where*. Delivery is triggered by the
|
||||
caller themselves: send a message, close the tab, and `_persist` announces
|
||||
it because nobody is following."""
|
||||
refused = client.post(
|
||||
"/api/push/subscribe",
|
||||
json={
|
||||
"endpoint": "https://10.0.0.5:8443/admin/shutdown",
|
||||
"keys": {"p256dh": "BAA" + "A" * 84, "auth": "AAAAAAAA"},
|
||||
},
|
||||
)
|
||||
assert refused.status_code == 400
|
||||
assert db.scalar(select(PushSubscription)) is None
|
||||
|
||||
|
||||
async def test_a_stored_endpoint_is_checked_again_before_the_post(db, registered):
|
||||
"""A row outlives the check made when it was written: a name that pointed at
|
||||
a push service can point inside the network later. The same split
|
||||
`agent/hosts.py` makes."""
|
||||
owner = db.scalar(select(User))
|
||||
row = PushSubscription(
|
||||
user_id=owner.id,
|
||||
endpoint="https://127.0.0.1:9/hijacked",
|
||||
p256dh=push_service.b64(
|
||||
ec.generate_private_key(ec.SECP256R1())
|
||||
.public_key()
|
||||
.public_bytes(
|
||||
encoding=serialization.Encoding.X962,
|
||||
format=serialization.PublicFormat.UncompressedPoint,
|
||||
)
|
||||
),
|
||||
auth_secret=push_service.b64(b"0123456789abcdef"),
|
||||
)
|
||||
db.add(row)
|
||||
db.commit()
|
||||
|
||||
sent = await push_service.send_one(db, row, {"title": "x", "body": "y", "url": "/"})
|
||||
|
||||
assert sent is False
|
||||
# Refused rather than dropped: the row is somebody's registration, and being
|
||||
# unreachable today is not the same as being gone.
|
||||
assert db.scalar(select(PushSubscription)) is not None
|
||||
|
||||
@@ -209,6 +209,40 @@ def test_the_panel_searches_rather_than_listing_everybody(db, client, registered
|
||||
assert "Person 29" in found
|
||||
|
||||
|
||||
def test_a_quote_in_the_search_does_not_break_the_panels_buttons(
|
||||
db, client, registered, owner, reader
|
||||
):
|
||||
"""`hx-vals` carries the values, and it was built by pasting the search term
|
||||
into a JSON string. Jinja escapes the quote for HTML, but the parser decodes
|
||||
it again before htmx parses the JSON -- so a `"` ended the string, made the
|
||||
attribute unparseable, and every checkbox in the panel silently stopped
|
||||
submitting anything. `q` is the last key, so an injected one would also have
|
||||
won a duplicate-key parse.
|
||||
|
||||
Built with `| tojson` over the whole object now, which escapes for JSON
|
||||
first and lets Jinja escape that for HTML. An existing grant is what keeps a
|
||||
row on screen whatever the search says.
|
||||
"""
|
||||
import html
|
||||
import json
|
||||
import re
|
||||
|
||||
note = notes_service.create(db, owner=owner, title="Note", body="x")
|
||||
sharing.set_grants(db, note, user_ids=[reader.id], group_ids=[])
|
||||
|
||||
hostile = '", "principal_id": "smuggled'
|
||||
page = client.get(
|
||||
f"/api/library/share/note/{note.id}", params={"q": hostile}
|
||||
).text
|
||||
|
||||
values = re.findall(r"hx-vals='([^']*)'", page)
|
||||
assert values, "the panel rendered no hx-vals at all"
|
||||
for raw in values:
|
||||
parsed = json.loads(html.unescape(raw))
|
||||
assert parsed["principal_id"] != "smuggled"
|
||||
assert parsed["q"] == hostile
|
||||
|
||||
|
||||
def test_an_existing_grant_stays_listed_whatever_the_search_says(
|
||||
db, client, registered, owner, reader
|
||||
):
|
||||
|
||||
+75
-1
@@ -376,12 +376,86 @@ def test_the_helper_units_take_no_branch_from_the_request():
|
||||
assert "__UPDATE_BRANCH__" in unit
|
||||
assert "__UPDATE_CHANNEL__" in unit
|
||||
assert "update-requested" in unit # deleted, not read
|
||||
assert "ExecStart=/bin/bash __PREFIX__/app/deploy/update.sh" in unit
|
||||
# Deleted before the script runs, or the path unit re-arms on a file that is
|
||||
# still there and the update loops.
|
||||
assert unit.index("ExecStartPre") < unit.index("ExecStart=")
|
||||
|
||||
|
||||
def test_root_does_not_run_a_script_the_service_account_owns():
|
||||
"""The unit has no `User=`, so ExecStart is root. It used to be
|
||||
`__PREFIX__/app/deploy/update.sh` -- inside the checkout, owned by the
|
||||
unprivileged service account -- so anything able to write as that account
|
||||
could rewrite it and be root, and so could whoever controlled the branch,
|
||||
since the pull runs as that account and root runs what it fetched.
|
||||
|
||||
**The old test asserted that exact line.** It passed for the whole life of
|
||||
the feature and pinned the vulnerability in place, which is this codebase's
|
||||
recurring failure applied to a privilege boundary: an assertion about the
|
||||
text rather than about the property the text was supposed to have.
|
||||
"""
|
||||
from pathlib import Path
|
||||
|
||||
import lembas
|
||||
|
||||
root = Path(lembas.__file__).resolve().parents[2]
|
||||
unit = (root / "deploy/lembas-update.service").read_text()
|
||||
install = (root / "deploy/install.sh").read_text()
|
||||
|
||||
exec_line = next(line for line in unit.splitlines() if line.startswith("ExecStart="))
|
||||
assert "__PREFIX__" not in exec_line, "root would run a file inside the checkout"
|
||||
assert "__UPDATE_HELPER__" in exec_line
|
||||
|
||||
# And the installer puts that copy somewhere root owns.
|
||||
assert 'install -o root -g root -m 755' in install
|
||||
assert "__UPDATE_HELPER__|$UPDATE_HELPER" in install
|
||||
|
||||
|
||||
def test_the_root_script_never_sources_a_file_the_service_account_can_replace():
|
||||
"""`.deploy-env` is written by the installer with `sudo tee`, so the file is
|
||||
root-owned -- but `$PREFIX` is the service account's own directory at mode
|
||||
755, and write permission on a directory is all it takes to unlink a file
|
||||
and put another there. `.` would have run its contents **as root**, and this
|
||||
script became root-triggerable by anyone who can create one file in
|
||||
`$PREFIX/data`, which is that same account.
|
||||
|
||||
Asserted as "nothing under $PREFIX is sourced" rather than as the shape of
|
||||
one line, because the next thing to be read from there would have the same
|
||||
problem and a test naming `.deploy-env` would not notice.
|
||||
"""
|
||||
from pathlib import Path
|
||||
|
||||
import lembas
|
||||
|
||||
root = Path(lembas.__file__).resolve().parents[2]
|
||||
script = (root / "deploy/update.sh").read_text()
|
||||
|
||||
for line in script.splitlines():
|
||||
stripped = line.strip()
|
||||
if stripped.startswith((". ", "source ")):
|
||||
assert "$PREFIX" not in stripped and "$APP" not in stripped, stripped
|
||||
|
||||
# And the two values it does want are extracted by pattern.
|
||||
assert "s/^SITE_HOST=" in script
|
||||
assert "s/^APP_PORT=" in script
|
||||
|
||||
|
||||
def test_the_update_script_notices_the_old_wiring():
|
||||
"""A host installed before the fix keeps the old unit, and re-running the
|
||||
installer is the only thing that moves it. The script therefore has to say
|
||||
so when it finds itself running from inside the checkout -- otherwise the
|
||||
hosts that are vulnerable are exactly the ones that never hear about it."""
|
||||
from pathlib import Path
|
||||
|
||||
import lembas
|
||||
|
||||
root = Path(lembas.__file__).resolve().parents[2]
|
||||
script = (root / "deploy/update.sh").read_text()
|
||||
|
||||
assert "INSECURE WIRING" in script
|
||||
assert 'readlink -f "$0"' in script
|
||||
assert "INSTALL_UPDATE_HELPER=1" in script
|
||||
|
||||
|
||||
def test_the_image_bakes_no_secret_and_no_data():
|
||||
from pathlib import Path
|
||||
|
||||
|
||||
Reference in New Issue
Block a user