fix: three access-control bugs found by a security review, plus a relay teardown bug - #24
Merged
Conversation
EventSender::request hardcoded `match self.mask.get` for all four request
types. `mask.push`, `mask.get_many` and `mask.observe` were never read
anywhere in the crate -- the only mask reads were `connected`, `get` and
`throttle`. This is a security fix: push writes to the local store, and
the mask is the documented control that gates it.
The contract says so in three places: EventMask docs ("push requests are
disabled by default, as they can write to the local store"), DEFAULT and
ALL_READONLY both set `push: RequestMode::Disabled`, and ALL_READONLY
explicitly declines to ship a push-enabled constant because it "would
risk misuse". None of it took effect. `RequestMode::None` -- what
BlobsProtocol::new(&store, None) installs -- means "allow, don't notify",
so the Disabled arm returning ProgressError::Permission was unreachable
for push under every configuration in the tree, including the public ALPN
registration in framework/app.
An unauthenticated peer that knows an EndpointId could therefore write
blobs of its choosing into the store, which the node then serves to
anyone presenting the hash. Content is BLAKE3-verified against the
pusher's own hash, so existing data cannot be forged or corrupted; the
impact is unauthorized write and content injection under the operator's
identity.
Thread the applicable RequestMode into `request` as a parameter, supplied
by four typed wrappers next to the mask itself. Observe maps through a
new From<ObserveMode> for RequestMode; it has no Disabled variant, so its
behaviour is unchanged beyond reading the right field.
The existing push tests were passing *because* of the bug: event_handler
used ALL_READONLY, which disables push. They now use an explicitly
push-enabled mask -- the conscious act the ALL_READONLY doc asks for.
New tests push the same blob to a denying node and to a permitting one.
The pusher cannot observe a refusal (execute_push_sink stops its receive
stream and returns Stats::default(), and the provider handles streams in
detached tasks), so the assertion is that the blob does not land. The
permitting node is the control: it proves the push path works in that
setup, so the deny assertion cannot pass vacuously. Both fail without the
fix, for the right reason, and pass with it. krikos-blobs 108/108.
Inherited from upstream iroh-blobs v0.103.0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
get_range built its redb bounds straight from the range endpoints in an incoming RangeItem. Every namespace shares one records table keyed (namespace, author, key), so those bounds are the only thing keeping documents apart, and the endpoints arrive off the wire unvalidated: RecordIdentifier is a raw Bytes with a derived Deserialize, and validate_limits counts parts and entries without ever inspecting `range`. The Ordering::Equal branch scoped correctly to the session namespace. The other two did not. Less used both remote endpoints verbatim; Greater used from_start/to_end, which pin only *one* side of each sub-range and leave the other remote. RecordsBounds::new is a passthrough and RecordsRange::with_bounds applies no post-filter, so nothing downstream caught it. get_fingerprint delegates to get_range and inherited the flaw. This is a security fix. The resulting diff is echoed back to the peer (ranger.rs, `have_local: false` -> diff -> RangeItem), and namespace and signature validation apply only to *incoming* entries, never to the outgoing diff. Since accept_request admits any authenticated peer for any namespace in our sync set, a peer holding one valid ticket could read every other document on the node: namespace ids, author keys, record keys, timestamps, content hashes and signatures. A leaked NamespaceId is itself the read capability, and leaked content hashes are fetchable from the blobs store, so it chains past metadata to content. Add RecordsBounds::clamp_to_namespace and apply it to every branch that consumes remote input, including both sub-ranges of Greater. An empty intersection -- what a range naming only foreign namespaces clamps to -- normalizes to a range selecting nothing, since inverted bounds are not a valid query. Normal traffic never exercised the gap: sessions start at Range::new(x,x) (the Equal branch) and recursion only produces in-namespace split points. Reconciliation is unaffected -- sync_big, sync_full_basic, sync_gossip_bulk and sync_restart_node all pass, krikos-docs 81 lib + 13 integration green. New test drives all three orderings with endpoints spanning the whole table and asserts nothing outside the session namespace comes back. It fails on the Less branch without the fix. Inherited from upstream iroh-docs v0.101.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The mainline fallback in ZoneStore::resolve passed whatever the DHT returned to mutable_item_to_signed_packet, which assembled it with SignedPacket::from_parts_unchecked -- no signature verification -- and never compared item.key() to the pubkey being resolved. That check cannot be delegated to mainline. MutableItem::from_dht_message verifies the signature against the key carried in the *response* and then stores query.target() verbatim; target_from_key is only ever used to build a request. The contrast is a few lines up in the same match, where immutable items *are* validated against the target. So mainline guarantees "signed by somebody", while we were reading it as "signed by the key I asked for". This is a security fix. Any DHT node answering the lookup could reply with a packet correctly signed under a key it controls and a high seq (get_mutable_most_recent folds on seq, so it wins deterministically). The zone cache then strips the attacker's z32 root and node_zone_handler re-appends the queried one, so the records are served authoritatively under the victim's name. Clients cannot detect it -- from_txt_lookup derives the EndpointId from the queried name and plain DNS carries no signature to re-check. QUIC/TLS still prevents impersonating the node identity, but the attacker forges user-data attributed to that EndpointId and serves arbitrary records under its public-key domain, which is precisely the binding pkarr exists to provide. Add SignedPacket::from_parts, which verifies rather than trusting, and have mutable_item_to_signed_packet take the queried key and build from it. A packet signed by any other key now fails signature verification instead of being silently re-attributed. Exposure was limited: mainline defaults to None and config.prod.toml disables it, but config.dev.toml enables it and lib.rs documents it as a supported production mode. New test accepts an item under the key that signed it and refuses the same item under another; it fails without the fix. krikos-dns-server 38/38. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The ingress parser accepted client datagrams the egress encoder then refused, and that refusal landed on the receiving client rather than the sender. Two shapes got through. An empty payload: Datagrams::from_bytes needs only the ECN byte, so a 34-byte frame decodes to empty contents, which RelayedStream::start_send rejects with SendError::EmptyPacket. And a one-byte overflow: ingress bounds the frame *after* the type varint (payload <= 65503), while the forwarded RelayToClientMsg carries its own type byte on top of the same payload, for 65537 > MAX_PACKET_SIZE. Either way send_packet returned Err, run_inner propagated it, and Actor::run logged "actor errored, exiting" and dropped the actor, whose Drop unregisters the client and closes its stream. The honest client cannot produce either frame -- client/conn.rs blocks empty sends locally -- so the checks read as unreachable, but a hand-rolled client reaches both trivially, aimed at any EndpointId it knows. Reject both at ingress, bounding by the *forwarded* length rather than the received one. And make the egress refusals non-fatal: a packet on that queue was put there by another client, so drop it (it is already counted in send_packets_dropped) and keep the session, matching how every other forwarding failure is handled -- handle_frame logs and continues, a full send queue drops. Only genuine stream errors, which mean this connection is already broken, stay fatal. Not a security finding: the impact is availability only. Verified there is no misrouting, src-spoofing or data-leak variant -- src always comes from the authenticated handshake identity, dst is an exact 32-byte map lookup, and start_send returns before touching the inner sink, so no partial frame is ever emitted. Two tests, both failing first: one asserts everything the parser accepts survives re-encoding, the other that an undeliverable packet is dropped while the session stays live and later packets still arrive. The proptest generator no longer emits empty datagrams, which are not valid relayed packets in either direction. krikos-relay 97/97 with --all-features. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Documentation for this PR has been generated and is available at: https://holon-technologies.github.io/iroh/pr/24/docs/krikos/ Last updated: 2026-08-04T17:23:51Z |
This was referenced Aug 2, 2026
Routing each request type through its own mask field is right, but it un-gated every config that had only ever set `mask.get` and relied on the old shared dispatch: - `examples/limit.rs::limit_by_hash` now answers get_many and observe with the same allowlist instead of leaving them at `DEFAULT`, where a peer could read any blob by asking for it as a `GetManyRequest`. - `examples/random_store.rs` hands push to its handler rather than inheriting `ALL_READONLY`'s `Disabled`, which made `--allow-push` a no-op. `ObserveMode` gains `Disabled` — an observe response streams the local bitfield for a hash, so a locked-down provider needs to refuse it outright, which no mask value could express — and its conversion now maps to the `*Log` variants: an observe request transfers no blobs, so completion is the only update it can emit, and the plain variants dropped it. The push-denial test asserts `BlobStatus::NotFound` rather than `!has()`, which is false for a partial import, and keeps sampling until a deadline instead of once, since nothing on the denying node is observable from the test and a single early sample would pass on a broken mask. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The catch-all arms replaced the caller's bound with the namespace bound, which widens rather than narrows: an `Included` end below the namespace end (what `RecordsBounds::author_key` with `KeyFilter::Exact` produces) became the whole namespace, and an `Excluded` start was re-admitted as `Included(namespace_start)`. No current caller hits either arm, but the function is `pub` and its doc promises the fallback is never wider. Handle the tighter shapes explicitly and extend the empty-range normalization to the bound combinations those arms can now produce. The regression test picked both namespaces at random, so a foreign namespace landed on one side of ours and only exercised the clamp facing it — each defect was caught half the time, unreproducibly. Pin one neighbour below and one above. It also only asserted that nothing leaked, which an over-clamp returning nothing satisfies; assert the expected count of our own entries per range as well. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`send_packet` listed the two droppable `SendError` variants and sent everything else to `Err`, the inverse of the policy the comment states. Both that enum and `WriteFrameError` are `#[non_exhaustive]`, so a future encode-validation variant would land in the fatal arm and let any peer end an unrelated client's session again, with no compile error. Match on the fatal set instead: a new `WriteFrameError` defaults to fatal, a new encode-side `SendError` defaults to a drop. The re-encodability test computed its boundary payload lengths for the packed layout only, so both batch boundary cases were rejected by the older ingress length check and never reached the new `forwarded_len` guard — deleting that guard's batch branch left the test green. Compute the boundary per layout and assert how many frames survived ingress, so the cases cannot go back to being silently skipped. Restoring empty-datagram coverage for the relay-to-client direction: narrowing the shared generator was only required for client-to-relay, but `RelayToClientMsg::from_bytes` still accepts zero-length contents and lost its only coverage. Split the strategy into `datagrams` and `forwardable_datagrams`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`from_parts` took the public key as `&[u8; 32]` but the signature as `&[u8]`. The wire layout is fixed-offset, so any other length carves the timestamp and packet out of the wrong bytes and reports a signature failure instead of a length error. Take `&[u8; 64]`. `from_parts_unchecked` still recommended itself for "DHT mutable items", the exact unbound use the checked constructor was added to replace — a consumer following that doc reassembles an attacker-signed packet under the queried name. Say what it is actually safe for. A forged DHT item is the one thing the new check produces, and it was swallowed by `let Ok(packet)`: a poisoned item outranking the victim's record made the name unresolvable, logged identically to a plain miss. Log the rejection and count it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both inventories are line-sensitive, so the security fixes on this branch moved sixteen recorded occurrences and CI has been red on the boundary check since the relay commit landed. The only content change is one addition: the `tokio::task::spawn` in `server/client.rs::tests::undeliverable_packet_does_not_end_the_session`. It falls under the existing "direct spawns in `#[cfg(test)]` regions" row — it owns only the actor under test, the test joins it before returning, and no simulator backend executes that module — so it is classified as acceptable nondeterminism and the row now names it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`push_is_rejected_when_disabled` propagated the `execute_push_sink` result for both targets, including the node that refuses. Whether a refusal reaches the pusher is a race: the provider resets the stream, and `execute_push_sink` sees that only if the reset lands before it has finished writing. On the Windows runner it did, and the test failed at the push rather than at any assertion — "sending stopped by peer: error 0". That result was never the property under test; the doc comment on the function already says the refusal is unobservable from the pusher and that the blob not landing is what matters. Tolerate either outcome for the denying node, and keep requiring success from the permitting one, since that control is what stops a passing deny assertion from being vacuous. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
These four have been failing on the macOS-arm runner, a different one each run, since before this branch: main's own CI shows test_download_policies timing out on 3a9a5b3 (2026-07-31). None of them is a sync defect. The ordered assertions in sync_full_basic required an exact event sequence from a live document that emits more than the sequence under test. A second SyncFinished -- the ticket-driven sync and the NeighborUp-driven sync both run, which the optional matchers in that same test already document -- or a PendingContentReady from a draining download queue landing between two asserted events failed the whole test. Add assert_next_ignoring, which skips a tolerated event without consuming a matcher, and give those sites the two events that interleave. Ordering stays asserted, which switching to assert_next_unordered_with_optionals would have given up; expected matchers take precedence, so a required SyncFinished is still matched, not skipped. assert_next has no callers left, so it goes. sync_restart_node and test_sync_via_relay accepted only `result.is_ok()` sync events. node1 has just restarted, so the first attempt can still land on the connection node2 held to the dead process ("Failed to close connection1"), and a relayed connection can drop mid-sync. Sync retries in both cases. Tolerate one failed attempt; the required matchers still demand a successful sync plus the entry and its content. test_sync_via_relay's join also had a 2s budget to finish a relay handshake, sync, insert and download -- it now uses the same TIMEOUT as every other wait here. test_download_policies is left doing what it did, but able to say why it stopped. Its select! arms used the refutable pattern `Ok(Some(ev)) = try_next()`; a stream that ends or errors fails to match, which disables that arm and leaves the loop waiting on the other one until the 120s timeout, reported as "timeout elapsed" and nothing else. Bind the whole Result and fail where it breaks, with the counters and downloaded keys. That is a diagnosis, not a fix: the root cause of the hang is not established, and this is what makes the next occurrence readable. 20 iterations of all four under 6-way CPU saturation: 0/80 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A security review of the whole tree turned up three exploitable access-control
bugs and one robustness bug. Each is fixed in its own commit, driven by a test
that was watched failing for the right reason first.
fix(blobs): the event mask was never applied to push requests.EventSender::requesthardcodedmatch self.mask.getfor all four requesttypes, so
mask.push,mask.get_manyandmask.observewere never readanywhere in the crate. Push writes to the local store and is documented as
deny-by-default in three places, but
RequestMode::Disabledwas unreachable forit under every configuration in the tree — including the public ALPN
registration in
framework/app. An unauthenticated peer that knew anEndpointIdcould write blobs of its choosing into the store, which the nodethen served to anyone with the hash. Content is BLAKE3-verified against the
pusher's own hash, so existing data could not be forged or corrupted; the impact
is unauthorized write and content injection under the operator's identity. Fixed
by threading the applicable
RequestModein from four typed wrappers.fix(docs): sync ranges were not clamped to the session namespace.All namespaces share one records table keyed
(namespace, author, key), so thequery bounds are the only thing keeping documents apart — and
get_rangebuiltthose bounds from the remote peer's unvalidated range endpoints.
Equalscopedcorrectly;
Lessused both remote endpoints verbatim andGreaterpinned onlyone side of each sub-range. The resulting diff is echoed back to the peer, and
namespace/signature validation applies only to incoming entries. Since
accept_requestadmits any authenticated peer for any namespace in the syncset, one valid ticket was enough to read every other document on the node:
namespace ids, author keys, record keys, content hashes, signatures. A leaked
NamespaceIdis itself the read capability and leaked hashes are fetchable fromthe blobs store, so it chains past metadata to content. Fixed with
RecordsBounds::clamp_to_namespaceon every branch that consumes remote input.fix(dns-server): DHT packets were not bound to the queried key.The mainline fallback assembled whatever the DHT returned with
from_parts_uncheckedand never compareditem.key()to the key beingresolved. That check cannot be delegated to mainline: it verifies a mutable
item's signature against the key carried in the response and stores
query.target()verbatim, so it only ever guarantees "signed by somebody" —the contrast is a few lines up in the same match, where immutable items are
validated against the target. Any DHT node could answer a lookup with a packet
signed under a key it controls and a high
seq, and those records were re-servedauthoritatively under the queried name. QUIC/TLS still prevents impersonating
the node identity, but this forges
user-dataattributed to thatEndpointIdand serves arbitrary records under its public-key domain, which is exactly the
binding pkarr exists to provide. Fixed by adding
SignedPacket::from_parts,which verifies, and building from the queried key.
fix(relay): one client could end another's session. Not a securityfinding — the impact is availability only, and there is no misrouting,
src-spoofing or leak variant. The ingress parser accepted two datagram shapes
the egress encoder then refused (empty contents, and a payload one byte too
large once the forwarded frame type is added), and that refusal was fatal to the
receiving client's actor. Fixed at ingress by bounding on the forwarded
length, and at egress by dropping the packet instead of the session, matching
how every other forwarding failure is already handled.
Breaking Changes
None to the wire protocol or public API. Two behaviour changes worth knowing:
EventMask::DEFAULTorALL_READONLYwas relying on the bug. Push now requires a mask that enablesit, which is what the
ALL_READONLYdoc has always asked for.accepted payload one byte lower so the forwarded frame fits. No conforming
client sends either —
client/conn.rsalready blocks empty sends locally.Notes & open questions
iroh-blobsv0.103.0 andiroh-docsv0.101.0, so they likely affect upstream and other forks. Worthreporting to n0-computer.
cargo test -p krikos --test patchbayis not verified here: it aborts in a#[ctor]withfailed to init userns: write setgroupsbefore any library coderuns, because the environment this was prepared in blocks user namespaces.
Nothing in this branch touches that path, but it needs a run in a normal
environment.
krikos-docs 81 + 13, krikos-relay 97 with
--all-features, krikos-dns-server38.
cargo clippy --all-features --all-targetsandcargo fmtclean on everytouched crate.
watching the new test fail.
Change checklist
🤖 Generated with Claude Code