Harden preshared-key lifecycle with zeroizing ownership#164
Open
lurenjia534 wants to merge 1 commit into
Open
Conversation
b4b0946 to
d92c619
Compare
Contributor
Author
|
Hi @hulthe, could you please review this PR when you have a chance? Thank you |
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
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.
Closes #163
Summary
Introduce a public, non-
CopyPresharedKeytype and use it throughout peer configuration, tunnel state, and Noise handshake state.PresharedKeyowns its bytes in zeroizing heap storage, redactsDebug, and makes secret duplication explicit throughClone. This is defense-in-depth memory-lifetime hardening; it does not claim complete erasure of every physical copy of a key.Changes
Secret ownership
PresharedKey, backed by stable heap storage that is zeroized on drop.Copy; cloning creates an independent zeroizing allocation.Debug.zeroizedependency.PresharedKey::take_fromclears the supplied mutable source array after importing it. The by-value and borrowed constructors cannot clear copies retained by the caller.Device and Noise state
Replace long-lived
Option<[u8; 32]>PSKs inPeer,PeerMut,Tunn,Handshake, andNoiseParams.Peer creation and update paths move PSK ownership where possible. APIs that return owned peer configurations create an explicit independent clone. Handshake PSK mixing borrows the configured bytes instead of materializing another owned array; an absent PSK continues to use WireGuard's 32-byte all-zero input.
UAPI integration
Make only the mechanical conversions required by the new core API:
KeyBytesvalue intoPresharedKeywhen applying configuration;PresharedKeyinto the existing wire response representation; andThere is no intended UAPI wire-format or parsing behavior change in this PR.
Runtime behavior
WireGuard behavior remains unchanged:
Noneretains the standard all-zero no-PSK input;Public API changes
This is an intentional pre-1.0 SemVer-relevant API change:
Peer::preshared_keybecomesOption<PresharedKey>.PeerMut::set_preshared_keyacceptsOption<PresharedKey>.Tunn::newandTunn::new_with_rngacceptOption<PresharedKey>.Tunn::set_preshared_keyacceptsOption<PresharedKey>.Tunn::preshared_keyreturnsOption<&PresharedKey>instead of an owned array.Peer::with_preshared_key([u8; 32])remains available, but is no longerconstbecause it allocates zeroizing storage.Example migration:
Callers that need an owned value from
Tunn::preshared_key()can use.cloned().Security properties and limitations
Covered by best-effort zeroization:
PresharedKeyallocation;PresharedKey::take_from.Moving a
PresharedKeymoves its heap pointer at the Rust abstraction boundary rather than copying its backing array. Replacing or dropping one owner clears that allocation, but does not revoke other clones.This does not guarantee erasure of:
take_from;as_bytes()borrow.The implementation does not use
mlockand does not protect live secrets from arbitrary process-memory reads. Drop-based clearing also requires destructors to run.Testing
Tests cover:
ZeroizeOnDropcontract;take_from;Debug;Locally passed:
cargo fmt --all -- --checkcargo check --locked --workspacecargo check --locked --workspace --no-default-features --features ringcargo check --locked --workspace --all-featurescargo clippy --locked --workspace --all-targets --all-features -- -D warningsRUSTDOCFLAGS='-D warnings' cargo doc --locked -p gotatun --all-features --no-depsgit diff --checkCARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER=env cargo test --locked -p gotatun --lib --all-features -- --skip udp::socket::linux::gro::tests::recv_many_from_grows_for_oversized_datagram --skip udp::socket::linux::gro::tests::recv_many_from_preserves_ipv6_source_addr --skip udp::socket::tests::bind_ipv4_socket_uses_ipv4 --skip udp::socket::tests::bind_retries_on_ipv6_conflictThe filtered library run completed with 81 passed, 0 failed, 7 ignored, and 4 filtered. The four socket tests were also attempted without filters; all four failed with
Operation not permittedin this restricted environment, while the other 81 tests passed.Not run locally: the seven privileged WireGuard integration tests, the cross-platform/Android CI matrix, the
cargo hackfeature powerset, andcargo-semver-checks. The SemVer check is expected to report the intentional API changes and requires a maintainer release decision.Out of scope
This PR intentionally does not change:
KeyBytesownership, visibility, or zeroization;Those concerns should be handled in separate, independently reviewable changes.
This change is