mirror of
https://github.com/meshtastic/firmware.git
synced 2026-10-09 14:41:19 -04:00
Jonathan BennettandClaude Opus 5
57bdedf324
* 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>