Skip to content
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

Not for merge: quick n' dirty ECDSA #57

Closed
wants to merge 2 commits into from

Conversation

nickray
Copy link
Member

@nickray nickray commented Jun 22, 2020

This builds on #56 by implementing a signing method on Scalar. It outsources entropy concerns and error handling to the caller. There are no deterministic signatures and there is no "additional randomness".

The masking scalar approach I believe goes back to PolarSSL (mbed TLS), although I have found no clear story about it. It is also used by kmackay/micro-ecc.

I did not find myself capable of understanding how existing traits are intended to be used, which is why I opted for this very direct approach: A secret key is a scalar, a public key is an affine point.

This is not for merge, rather as input to the discussion mentioned in #54 (comment)

@tarcieri
Copy link
Member

Awesome! Thanks for submitting this.

I did not find myself capable of understanding how existing traits are intended to be used, which is why I opted for this very direct approach: A secret key is a scalar, a public key is an affine point.

The traits you really need for a cross-curve abstraction don't exist yet, so don't worry about it (see #22), however this is great food for thought about it. I would agree "secret key is scalar, public key is an affine point" is the right starting point.

Comment on lines +3 to 14
#[cfg(feature = "expose-arithmetic")]
pub mod field;
#[cfg(not(feature = "expose-arithmetic"))]
mod field;
#[cfg(feature = "expose-arithmetic")]
pub mod scalar;
#[cfg(not(feature = "expose-arithmetic"))]
mod scalar;
#[cfg(feature = "expose-arithmetic")]
pub mod util;
#[cfg(not(feature = "expose-arithmetic"))]
mod util;
Copy link
Member

@tarcieri tarcieri Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to figure out a better solution for this sort of gating.

One option would be to make the arithmetic module mod arithmetic (no pub), and then re-export the types it contains at the toplevel when the arithmetic feature is enabled.

Since doc_cfg tags everything with the requisite cargo features, I don't think this would be confusing:

https://docs.rs/p256/0.3.0/p256/arithmetic/index.html

I might open a separate PR for discussion on that idea (skipping the expose-arithmetic stuff for now)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the easiest solution will be to use the cfg_if crate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that would work too.

For what it's worth I opened a PR with my suggested refactoring and I like it:

#58

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmh, agreed. This is just a hack to allow use of the arithmetic in an experimental/hazmat manner outside the crate while core pieces are missing. I actually think the arithmetic should be public, but that's another topic - this PR is just to exemplify a possible ECDSA implementation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickray now that #58 is merged, you can refactor this to use something like the following in the toplevel of the crate:

#[cfg(feature = "expose-arithmetic"))]
#[cfg_attr(docsrs, doc_cfg(feature = "expose-arithmetic"))]
pub use arithmetic::{field, scalar, util};

@nickray
Copy link
Member Author

nickray commented Jun 23, 2020

The traits you really need for a cross-curve abstraction don't exist yet, so don't worry about it (see #22), however this is great food for thought about it. I would agree "secret key is scalar, public key is an affine point" is the right starting point.

I'm not sure how many abstractions are actually needed (looking at the p256 implementation here). The only thing I'm aware of that's tricky is the "left-most bits" thing, which for p256 is just "subtract modulus if necessary", but e.g. for p521 requires a right shift to chop off some low bits.

@nickray
Copy link
Member Author

nickray commented Jul 22, 2020

Do you need me to do anything here? I think it might have a more uniform look n' feel to k256 if you make whatever adjustments you deem necessary (and I'm a little low on time/energy right now).

It would be good though to have the masking scalar / vartime inversion option, as things are horribly slow otherwise on microcontrollers.

@nickray nickray marked this pull request as ready for review July 22, 2020 18:18
@tarcieri
Copy link
Member

@nickray I'm pretty much just going to open a new PR and copy over the implementation from the k256 crate, however I can incorporate the masking_scalar / invert_vartime support you have in this PR.

(we don't have an optimized invert_vartime in k256 yet, however perhaps random blinding would still be nice for things like mitigating SPA/DPA /cc @fjarri)

@fjarri
Copy link
Contributor

fjarri commented Jul 22, 2020

I checked the speed of vartime inversion from this PR in k256, and it's about 25% faster than the constant-time implementation. Would be nice to have a generic implementation somewhere because it will be the same for all curves as long as the scalars expose the necessary methods.

@tarcieri
Copy link
Member

A generic implementation would be great, as it would make it possible to move handling of the masking scalar out of SignPrimitive (it feels a bit awkward in the current API).

It could go into the elliptic-curve crate itself, although it'd probably need some prerequisite traits first (see #22)

@tarcieri
Copy link
Member

Relevant parts of this were incorporated into #84, which implements ECDSA/P-256 (low-level) signing and verification

@tarcieri tarcieri closed this Jul 24, 2020
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.

None yet

4 participants