diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cdd51582..9b714ce0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -384,10 +384,18 @@ jobs: # Debug-only helpers (anything behind #[cfg(debug_assertions)]) vanish in # a release build, so a test calling one without the same gate breaks a # build no other job performs: every run above compiles the test target - # in debug. Compile it in release too, without running it — the point is - # that it builds at all. Mirrored in testing/ci-local.sh. - - name: Compile the library tests in release mode - run: cargo test --release --lib --no-run + # in debug. Compile it in release too. Of what it builds, run only the + # leg-slot residue test, which is release-only because only an optimised + # build leaves the slot in a state worth measuring; the grep fails the + # step if that test did not run, since a name filter that matches + # nothing passes. Mirrored in testing/ci-local.sh. + - name: Compile the library tests in release mode and run the leg-slot residue test + shell: bash + env: + RESIDUE_TEST: peer::machine::tests::take_leg_leaves_no_session_keys_in_the_slot_it_empties + run: | + cargo test --release --lib -- --exact "$RESIDUE_TEST" | tee "$RUNNER_TEMP/release-lib-tests.log" + grep -q '^test result: ok\. 1 passed;' "$RUNNER_TEMP/release-lib-tests.log" # ───────────────────────────────────────────────────────────────────────────── # Job 2b – Unit tests (macOS) diff --git a/docs/reference/security.md b/docs/reference/security.md index 71a2b13f..197fd262 100644 --- a/docs/reference/security.md +++ b/docs/reference/security.md @@ -123,14 +123,13 @@ reach: dropped. Each of those moves leaves behind, in a stack frame that is no longer in use, a copy of the node's long-term private key and, once the handshake has started, of its ephemeral key and chaining - key. Two moves out of the slots on a connection's control machine + key. Three moves out of the slots on a connection's control machine are cleared: when a completed handshake leaves the slot that held it, - and when a session is taken out of its slot for a rekey, the slot is - overwritten as the value leaves. Other moves are not. When a - connection is promoted to an active peer, or reaped as stale, its - whole handshake state is moved off its control machine, which leaves - the session's two traffic keys, or an unfinished handshake's private - keys, in the heap memory the machine occupies. + when a session is taken out of its slot for a rekey, and when the + whole handshake state leaves the machine because the connection is + promoted to an active peer, resolved as one side of a + cross-connection, or reaped as stale. In each case the slot is + overwritten as the value leaves. Other moves are not. - **Loading the identity from a secret string.** Building the node's identity from its key file or from `node.identity.nsec` leaves copies of the private key, among them a whole intermediate identity, diff --git a/src/peer/machine.rs b/src/peer/machine.rs index 90ca055d..1c4a8792 100644 --- a/src/peer/machine.rs +++ b/src/peer/machine.rs @@ -594,7 +594,12 @@ impl PeerMachine { /// Take the handshake crypto carrier off the machine (promotion and /// teardown consume it by value). pub(crate) fn take_leg(&mut self) -> Option { - self.leg.take() + // On promotion the machine lives on as the peer's control machine, so + // the slot is cleared as the leg leaves, or it would keep the session's + // traffic keys. Callers unwrap the result at once, which `take_cleared` + // warns may leave a second stack copy; clearing here still covers every + // caller's heap slot, and a stack copy is a move like any other. + noise::take_cleared(&mut self.leg) } /// Attach a handshake crypto carrier to the machine. @@ -3405,6 +3410,44 @@ mod tests { assert_eq!(decrypted, plaintext); } + /// Non-zero bytes left in a leg slot, read in place. + #[cfg(not(debug_assertions))] + fn leg_slot_nonzero_bytes(slot: &Option) -> usize { + let base = (slot as *const Option).cast::(); + (0..std::mem::size_of::>()) + // SAFETY: `base` comes from a live reference, so every byte of the + // slot is in bounds; volatile so the read is of memory as it is. + .filter(|&i| unsafe { std::ptr::read_volatile(base.add(i)) } != 0) + .count() + } + + /// Release only: in a debug build `Option::take` leaves stack garbage in + /// the payload of the `None` it writes, so the count means nothing there. + #[cfg(not(debug_assertions))] + #[test] + fn take_leg_leaves_no_session_keys_in_the_slot_it_empties() { + let initiator = Identity::generate(); + let responder = Identity::generate(); + let responder_id = PeerIdentity::from_pubkey_full(responder.pubkey_full()); + let mut ini = outbound_leg(LinkId::new(1), responder_id, 1000); + let mut res = Box::new(inbound_leg(LinkId::new(2), 1000)); + let msg1 = ini + .start_handshake(initiator.keypair(), make_epoch(), 1100) + .unwrap(); + res.receive_handshake_init(responder.keypair(), make_epoch(), &msg1, 1200) + .unwrap(); + assert!(res.has_session(), "the responder's leg holds the session"); + assert!(leg_slot_nonzero_bytes(&res.leg) > 64); + + let leg = res.take_leg().expect("the leg was attached"); + assert!(leg.noise_session.is_some(), "the session left with the leg"); + let left = leg_slot_nonzero_bytes(&res.leg); + assert!( + left <= 8, + "{left} non-zero bytes of the session stayed in the emptied leg slot" + ); + } + #[test] fn test_connection_failure() { // `mark_failed` releases the leg's Noise handshake handle. The failure diff --git a/testing/ci-local.sh b/testing/ci-local.sh index d4526341..8359b40e 100755 --- a/testing/ci-local.sh +++ b/testing/ci-local.sh @@ -676,16 +676,24 @@ run_tests() { # Debug-only helpers (anything behind #[cfg(debug_assertions)]) vanish in a # release build, so a test calling one without the same gate breaks a build # nothing here ever performs: every run above compiles the test target in - # debug. Compile it in release too, without running it — the point is that - # it builds at all. Mirrored in .github/workflows/ci.yml; check-ci-parity.sh - # compares integration suites only and would not catch a stage added to one - # runner and not the other. - info "cargo test --release --lib --no-run" - if cargo test --release --lib --no-run 2>&1; then + # debug. Compile it in release too. Of what it builds, run only the leg-slot + # residue test, which is release-only because only an optimised build + # leaves the slot in a state worth measuring; the grep fails the stage if + # that test did not run, since a name filter that matches nothing passes. + # Mirrored in .github/workflows/ci.yml; check-ci-parity.sh compares + # integration suites only and would not catch a stage added to one runner + # and not the other. + local residue_test="peer::machine::tests::take_leg_leaves_no_session_keys_in_the_slot_it_empties" + local release_log + release_log=$(mktemp "/tmp/ci-release-lib-tests.XXXXXX") + info "cargo test --release --lib -- --exact $residue_test" + if cargo test --release --lib -- --exact "$residue_test" 2>&1 | tee "$release_log" \ + && grep -q '^test result: ok\. 1 passed;' "$release_log"; then record "release-test-compile" 0 else record "release-test-compile" 1 fi + rm -f "$release_log" } # ── Stage 3: Integration Tests ─────────────────────────────────────────────