2 Commits

Include renames

Author	SHA1	Message	Date
Jonathan BennettandClaude Opus 5	57bdedf324	Bind the ack proof to the node we addressed, not the ack's sender (#11932) ... * Bind the ack proof to the node we addressed, not the ack's sender The MAC proves only that its author holds a pairwise key with us, and every keyed peer holds one. ackProofVerify looks the key up by getFrom(p) - the ack's claimed sender - and nothing compared that against the node we actually sent to, so C, whose authoritative key we hold, could read our packet id out of the cleartext header and mint a receipt for a packet that went to B. It verified VALID. "An authenticated delivery receipt from the actual recipient" was not what the code delivered. Guard on orig->packet->to before verifying. Since that establishes getFrom(p) == orig->packet->to, the existing key lookup is then correct and AckProof.cpp is untouched. Bailing out rather than verifying against the recipient key and reporting INVALID is deliberate twice over: it skips the X25519 an attacker would otherwise choose when we pay, and a third-party ack is "not a receipt" rather than "a forged receipt" - naks from intermediates (NO_CHANNEL, PKI_UNKNOWN_PUBKEY, MAX_RETRANSMIT) legitimately come from a node that is not the destination and must not be logged as proof mismatches. A broadcast original has no single recipient, so there is nothing to bind to. Also correct the header doc. It claimed channel (non-PKI) traffic gets nothing from this, but isProvableAck tests only the ack's shape: a DM that travelled under channel encryption still gets a proven ack when we hold the peer's key, because the secret comes from X25519 rather than from the channel. That is more coverage than the receipt needs and it costs one X25519 per ack generated - both worth stating rather than implying the opposite. test_proof_from_third_peer_fails_under_recipient_key pins the property the guard relies on: Carol's proof for a packet Alice sent to Bob verifies under Carol's key and fails under Bob's. The guard itself is not directly assertable while every branch of the verdict switch returns true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012hLcVif8GDEmA2k77hmFG8 * Make the sender check honor ACK_PROOF_ENFORCE The mismatch branch returned true unconditionally. Today every branch of this function returns true, so that reads as equivalent - but if enforcement is ever switched on it is a hole rather than a no-op. The sender field is not authenticated, so an attacker would simply address the ack from anyone other than the node we sent to, take the early return, and skip the proof requirement entirely. Hold only success acks to it. A nak legitimately arrives from an intermediate rather than from the destination - NO_CHANNEL, PKI_UNKNOWN_PUBKEY and MAX_RETRANSMIT all do - so naks keep today's behavior in either mode. Broadcast gets its own unconditional return rather than sharing the condition. orig->packet->to is NODENUM_BROADCAST there and can never equal any sender, so folding it into the sender check would, under enforcement, stop every reliable broadcast from ever being acked. This does not make enforcement sound on its own and the comment says so: ABSENT still permits, so spoofing the sender and omitting the proof gets through regardless, and ABSENT cannot be made to block for the reasons recorded at ACK_PROOF_ENFORCE. The narrower point is that a flag named "enforce" should not have a branch that silently ignores it. Raised by CodeRabbit on #11932. Its reading - that this is a live authorization bypass a third party can use to suppress retransmissions - does not hold: the sender field is unauthenticated either way, and perhapsGenerateImplicitAckForOwn Overheard already clears a pending retransmission on a replayed copy of our own ciphertext, with no ack and no key involved. The structural point stands on its own merits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012hLcVif8GDEmA2k77hmFG8 * Avoid cppcheck's duplicateValueTernary in the enforce check `return isAck ? !ACK_PROOF_ENFORCE : true` has the same value in both arms while the flag is off, which is the whole point of the line - and is exactly what cppcheck reports: style: Same value in both branches of ternary operator. [duplicateValueTernary] That failed the `check` matrix on every platform. Write it as an if, which expresses the same thing and does not trip the rule, and say so in a comment so it does not get folded back into a ternary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012hLcVif8GDEmA2k77hmFG8 --------- Co-authored-by: Claude <noreply@anthropic.com>	2026-09-24 07:15:54 +00:00
Jonathan BennettandClaude Opus 5	6f3f0bd7c2	Prove explicit acks with Routing.ack_proof (#11877) ... * Prove explicit acks with Routing.ack_proof Explicit acks are ROUTING_APP packets, and ROUTING_APP is excluded from PKC, so an ack travels under channel encryption alone - and the default channel key is public. Anyone in range can forge one, and the client grants its strongest delivery claim on the strength of the ack's unauthenticated `from`. Where the acknowledged packet was PKI encrypted the endpoints already share a Curve25519 secret, so the recipient can prove receipt in ~10 encoded bytes: ack_proof = HMAC-SHA256(shared_key, "ack" | LE32(from) | LE32(to) | LE32(request_id) | routing)[0..8) where `routing` is the encoded Routing message without the ack_proof field, taken as received with that byte range removed rather than re-encoded. Excising keeps the value a function of the received bytes alone, so it does not depend on two implementations' encoders agreeing and does not drop fields this build has never heard of. For the same reason the sender appends the field rather than setting it on a decoded struct and re-encoding. What this does and does not buy. It buys an authenticated delivery receipt from the actual recipient, which is the property a forged ack costs a user and which matters where people act on a delivery confirmation. It does NOT protect the retransmission loop, and must not be described as if it does: perhapsGenerateImplicitAckForOwnOverheard clears a pending retransmission on any overheard rebroadcast of our own (from, id), header-only and keyless, so replaying the originator's own ciphertext stops their retries more cheaply than forging an ack. No ack authentication of any kind closes that path. Advisory only, and deliberately not a step toward enforcement. A rule requiring a proof once a peer has sent one would make a missing proof destroy the only delivery signal we have, on state the user cannot see: the proof needs the peer to hold our key, and peer-side eviction, a downgrade or a factory reset are all invisible to us. A valid proof marks the ack verified; anything else behaves exactly as today. The client renders the difference. Verification costs one X25519 and nothing caches the shared secret, so ackProofPermitsAction gates on findPendingPacket first - otherwise a forged ack naming any packet id, which is visible in the cleartext header, would force a DH. Depends on meshtastic/protobufs#1094 for the generated field. * test: correct a comment that predates ack_proof being generated The wire-roundtrip test described ack_proof as an unknown field, which was true while the prototype hand-encoded it. The field is generated now, so this build understands it - but older firmware does not, which is the case the assertion actually covers. * Fix the EXCLUDE_PKI test build, and format Two copies of test_proof_binds_error_reason and test_proof_binds_direction were sitting in the #else branch of test_ack_proof, where Identity, makeIdentity, makeAck, becomeNode, crypto and ACK_PROOF_SIZE do not exist. They were also unregistered, so they were dead code that only served to break the build. The native suite never compiles that branch, so a local run could not see it. Also apply clang-format to a declaration that had been wrapped by hand. * Exempt test_ack_proof from trufflehog's Lob false positive Same detector and same shape as the three suites already listed here: it stitches nearby hex literals into one candidate string, and this suite's node numbers and request ids (0x0A0A0A0A, 0x0B0B0B0B, 0xABCD1234) happen to match a Lob API key. The suite holds no literal key material - every key it uses comes from crypto->generateKeyPair at runtime. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012hLcVif8GDEmA2k77hmFG8 --------- Co-authored-by: Claude <noreply@anthropic.com>	2026-09-17 22:24:29 +00:00