Skip to content

Clamp X25519 private scalars loaded via from_private_bytes - #92

Open
bergie wants to merge 3 commits into
attermann:masterfrom
bergie:fix/clamp-seeded-x25519-scalars
Open

bergie wants to merge 3 commits into
attermann:masterfrom
bergie:fix/clamp-seeded-x25519-scalars

Conversation

@bergie

@bergie bergie commented Sep 28, 2026

Copy link
Copy Markdown

The raw Curve25519::eval ladder leaves RFC 7748 scalar clamping to the caller (as Curve25519::dh1 does for generated keys), but the Python reference implementation clamps inside X25519. Identities derived from seed material that is not already clamped — for example deterministic rfed channel identities that use SHA-256(name) directly as the private scalar — therefore produced public keys and Diffie-Hellman shared secrets that disagreed with Python RNS, breaking encryption against Python peers.

Clamp the scalar in from_private_bytes. Clamping is idempotent, so generated keys and keys loaded from files written by Python RNS (whose stored scalars are already clamped) are unaffected.

Adds a test_crypto interop vector generated by the Python reference pinning public-key derivation, the shared secret, and the Identity::load_private_key path for a seeded identity.

bergie and others added 3 commits September 28, 2026 12:16
The raw Curve25519::eval ladder leaves RFC 7748 scalar clamping to the
caller (as Curve25519::dh1 does for generated keys), but the Python
reference implementation clamps inside X25519. Identities derived from
seed material that is not already clamped — for example deterministic
rfed channel identities that use SHA-256(name) directly as the private
scalar — therefore produced public keys and Diffie-Hellman shared
secrets that disagreed with Python RNS, breaking encryption against
Python peers.

Clamp the scalar in from_private_bytes. Clamping is idempotent, so
generated keys and keys loaded from files written by Python RNS (whose
stored scalars are already clamped) are unaffected.

Adds a test_crypto interop vector generated by the Python reference
pinning public-key derivation, the shared secret, and the
Identity::load_private_key path for a seeded identity.
Updated CHANGELOG with cryptography fixes and test additions.
@attermann

Copy link
Copy Markdown
Owner

Thanks @bergie for the fix, and for generating the vector with the Python reference.

I ran the new testX25519SeededKeyClamping on current master and it passes, so right now it doesn't show the bug. I think the vector is the reason. The seed's first byte is 0x88, so its low three bits are already clear. Its last byte is 0xfd, so bit 254 is already set. The only bit clamp() changes for this seed is bit 255, and the X25519 ladder never reads that bit. Master gives the same public key and shared secret with or without the clamp.

The contribution policy only accepts crypto changes that fix a demonstrated bug against the reference implementation (section 4), and asks for a way to reproduce (section 5.3). So before this can go further:

  1. Replace or add a vector whose seed actually needs clamping: at least one of the low three bits of byte 0 set, and/or bit 6 (0x40) of byte 31 clear. Generate the expected values with Python RNS as before, and check that the test fails on master and passes with your change.
  2. Add a short reproduction to the description: how you saw the mismatch with Python peers (for example, the rfed channel identity case), and on which platform.

If no such seed fails on master, the Crypto fork's Curve25519::eval already clamps internally and this change isn't needed. Please say so in that case, so we can close this or narrow it to the test.

Optional hardening while you're in there: check that the key is 32 bytes in from_private_bytes before clamping, so a short key can't cause an out-of-bounds write on scalar[31].

My time for reviews is limited, so it may take me a while to come back to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants