Conversation
RFC 8446 §4.2.8.2 requires aborting when the X25519 shared secret is the identity:
a peer key of low order makes it all zeros, i.e. a secret the peer knows.
`x25519-dalek` computes that result and only *reports* it through
`was_contributory()`, and `ActiveKeyExchange::complete` documents the rejection as
a requirement of the trait it implements ("must return an error if `peer_pub_key`
is invalid: either mis-encoded, or an invalid public key (such as, but not limited
to, being in a small order subgroup)"). The `ring` and `aws-lc-rs` providers do
reject it, so X25519 is the one group here that silently completes on a predictable
secret.
The two NIST groups cannot reach this case — their curves have cofactor 1, and the
identity is not a valid `PublicKey` — so the check belongs to `X25519KeyExchange`
alone.
`tests/x25519_low_order.rs` pins it: the seven encodings of a low-order point from
libsodium's `has_small_order` table (order 1, 2, 4 and 8, plus the three
non-canonical encodings at or above the field prime that reduce to them) are each
rejected with `PeerMisbehaved::InvalidKeyShare`, a peer key of the wrong length with
them, and a real peer key completes as the control that a group rejecting everything
would fail.
Runnin4ik
added a commit
to Runnin4ik/dpi-detector
that referenced
this pull request
Sep 25, 2026
…ands The group in `crates/dpi-core/src/net/x25519.rs` and the substitution in `crypto_provider()` exist because `rustls-rustcrypto`'s own X25519 completes a handshake on a low-order peer key. That is a gap in the crate, not a decision it made — nothing in that project's tracker names `was_contributory` — and it is now a PR against its `master`: [RustCrypto/rustls-rustcrypto#314](RustCrypto/rustls-rustcrypto#314), two files: the check, and `tests/x25519_low_order.rs` over libsodium's seven low-order encodings with a real-peer control. Both files here record it, because the next session should not have to find out again: `vendor/rustls-rustcrypto/README-PATCH.md` says why the check is not in the patch and what to remove when a release carries it, and the module doc points at the PR. It is the one piece of that fork whose fate is upstream's to decide: the `rustls-webpki` swap is already on `master` and only unreleased (ISS #107), so if the check lands and the next generation ships, the substitution goes and the provider becomes an ordinary dependency. Verified before opening it, in a clone at `%TEMP%/pr-provider`: their CI rows pass — `cargo test --features tls12` and `cargo test --no-default-features --features tls12,alloc`, five tests each, the three new ones among them — and reverting the check makes `x25519_rejects_a_low_order_peer_key` fail, which is the test holding the behaviour rather than the code. Clippy on that tree reports only the pre-existing `src/lib.rs:16` (`clippy::from_iter_instead_of_collect` was removed from clippy, so naming it is itself a warning on a current toolchain); the PR body says so, and it is not this change's to fix.
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.
X25519KeyExchange::completeaccepts a peer key of low order.x25519-dalekcomputes the Diffie-Hellman result for such a key — the identity, a shared secret the peer knows — and only reports that throughwas_contributory(), so the check has to be made here. RFC 8446 §4.2.8.2 requires the handshake to abort, andActiveKeyExchange::completedocuments it as a requirement of the trait:The
ringandaws-lc-rsproviders reject such a share, so a client that switches provider changes behaviour on that wire input. The two NIST groups in this crate cannot reach the case at all — their curves have cofactor 1, and the identity is not a validPublicKey— which leavesX25519KeyExchangeas the one group that silently continues.The change is the check itself, plus
tests/x25519_low_order.rs:has_small_ordertable — the points of order 1, 2, 4 and 8, and the three non-canonical encodings at or above the field prime that reduce to them — are rejected withPeerMisbehaved::InvalidKeyShare. A check that only rejected the all-zero key would still accept a key of order 2;Verified:
cargo test --features tls12andcargo test --no-default-features --features tls12,allocpass (5 tests each); reverting the check makesx25519_rejects_a_low_order_peer_keyfail, so the test does hold the behaviour rather than the code.Unrelated, and not something this PR touches: on my toolchain (clippy 1.98)
make checkstops atclippy::from_iter_instead_of_collectinsrc/lib.rs— the lint has been removed from clippy, so naming it in the crate's#![warn(…)]is itself an error under-D warnings. CI's clippy job is green here, so this is a toolchain-version matter rather than a CI failure, and it is not this change's to fix.