Skip to content

Conversation

@threema-theo
Copy link
Contributor

@threema-theo threema-theo commented Apr 8, 2025

crypto_box currently uses the * operator directly to multiply the secret_key.scalar and the public_key.0.
This results in a minor incompatibility with libsodium, especially for specially drafted public keys.

This MR adds

  1. a test for one such special case, i.e., the public key being on the twist of a curve. Along with this comes the function of generating the ciphertext with libsodium, see test-vector-gen/src/crypto_box.rs. This test is taken from bleichenbacher-daniel/Rooterberg.
  2. a fix for the Diffie-Hellman computation that essentially copies the code for multiplication from x25519_dalek.

To reproduce the issue, clone this repository, and run the tests.

  • Without the fix (second commit), the tests fail.
  • With the fix, the tests succeed.

@tarcieri tarcieri merged commit 20be967 into RustCrypto:master Apr 8, 2025
11 checks passed
@threema-theo
Copy link
Contributor Author

Thanks for the fast merging!

We have a security audit we would like to share with you. As this MR solved the only issue ( 🎉 ), we could directly include the fix. For that we would need a release first. Otherwise, we can share the audit as it is. Please tell me your preference.

On a side note, the security advisory URL https://github.com/RustCrypto/nacl-compat/security/advisories/new is broken. How should we send you the audit?

@tarcieri
Copy link
Member

We have a security audit we would like to share with you. As this MR solved the only issue ( 🎉 ), we could directly include the fix. For that we would need a release first. Otherwise, we can share the audit as it is. Please tell me your preference.

Will try to cut a new release soon

On a side note, the security advisory URL https://github.com/RustCrypto/nacl-compat/security/advisories/new is broken. How should we send you the audit?

Please open a separate issue with screenshots that illustrate whatever problem you're having. The link works for me.

@dignifiedquire
Copy link
Member

This change actually breaks basic usage with ed25519-dalek: #182

@tarcieri
Copy link
Member

Not sure what to do about that. We may need to add first-class support for Ed25519.

@dignifiedquire
Copy link
Member

what I don't quite understand is why does this fail in conjunction with ed25519-dalek but seems to be fine in other use cases

@tarcieri
Copy link
Member

I believe it's because the Ed25519 scalar has already been clamped.

Clamping can make the Edwards -> Montgomery equivalence somewhat difficult to use

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.

3 participants