-
Notifications
You must be signed in to change notification settings - Fork 229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
restrict ExpandedSK::sign visibility to avoid pk oracle #205
Conversation
While this partly addresses the issue, the To really address the issue, these fields also need to be marked private, and the |
it really isn't our call, maybe maintainers like @isislovecruft and @hdevalence can review then approve? |
|
instead of removing it, I added simple validation logic to ensure matching key pair in fd8c5fb @sopium wdyt? |
Well it's certainly better backward compatibility. But if backward compatibility is less a concern, I think a better API would be to have a |
If the |
@sopium "expanded secret key" means something different: an Ed25519 private key is a 256-bit "seed" which is expanded into 512-bits using SHA-512. The left half of that value is the canonical private scalar, and the other half is a random unrelated input used when deterministically deriving An Ed25519 keypair includes both the expanded secret key and the public key. |
I suggested having a |
Added a few docs and did formatting. We should be able to merge/publish this soon. A few notes/questions
|
@rozbb returning a reference would make it possible to impl the |
Ah good point. Probably should be a ref then. And the secret key could also be a ref (probably better to not have clones of it lying around; if a user really needs a copy, they can roundtrip it through serialization). Also, regarding points 1 and 2 above, is the extra expansion really a big deal for performance? Signing already requires two |
Yeah, the C implementation also does the expansion / clamping every time too |
Ok, then are we good to merge once If so, @alxiong could you make the following changes:
|
Done @rozbb (btw, didn't remove the unused methods |
Ah.. what would you suggest we should do with serde test code for |
I believe |
@rozbb yeah removing the |
The current MSRV test is failing because the The easiest way to work around that is probably to check in Cargo.lock and ensure it's computed for Edit: I'd probably suggest someone do that in a separate PR to keep those changes out-of-band from these. |
Thanks again! |
- Eliminates dead code left over from #205 - Adds `-D warnings` in CI so that warnings are treated as errors. This ensures code must be warning-free to pass CI.
- Eliminates dead code left over from #205 - Adds `-D warnings` in CI so that warnings are treated as errors. This ensures code must be warning-free to pass CI.
- Eliminates dead code left over from #205 - Adds `-D warnings` in CI so that warnings are treated as errors. This ensures code must be warning-free to pass CI.
- Eliminates dead code left over from #205 - Adds `-D warnings` in CI so that warnings are treated as errors. This ensures code must be warning-free to pass CI.
The fix has been merged but still hasn't shipped to crates.io. Is there anything in particular holding up a new release with the fix? |
The PR included breaking changes to the API to eliminate potential misuses, so a fix can only go out as part of That release will include a bump to curve25519-dalek v4.0, whenever that is released: dalek-cryptography/curve25519-dalek#405 |
I was relying on |
See ample previous discussion on #298 |
Thanks, looks like the PR #299 linked therein would solve my issue. |
We were not vulnerable to the "Double Public Key Signing Function Oracle Attack on `ed25519-dalek`" either way, since it was fixed at 2.0.0-rc2. See dalek-cryptography/ed25519-dalek#205
We were not vulnerable to the "Double Public Key Signing Function Oracle Attack on `ed25519-dalek`" either way, since it was fixed at 2.0.0-rc2. See dalek-cryptography/ed25519-dalek#205 Ref: https://rustsec.org/advisories/RUSTSEC-2022-0093
* Bump our use of ed25519-dalek 2.0.0-rc3 We were not vulnerable to the "Double Public Key Signing Function Oracle Attack on `ed25519-dalek`" either way, since it was fixed at 2.0.0-rc2. See dalek-cryptography/ed25519-dalek#205 Ref: https://rustsec.org/advisories/RUSTSEC-2022-0093 * Ignore the RUSTSEC-2022-0093 for now * Update features snapshot
The least disruptive API changes might be turn
ExpandedSecretKey::sign_*()
topub(crate)
, leaving only the correctKeyPair.sign_*()
API for signing to public invocation, which is always correct and not vulnerable to "Signing Function Oracle" attack.