Split out RFC6979 implementation to its own crate #257

rvolgers · 2021-02-26T17:29:56Z

This is the deterministic k (nonce) generation used by ecdsa. It is being split out because it will also be used in the upcoming dsa implementation.

For the currently implemented ECDSA use cases we always have a modulus which has a bit length which is divisible by 8 and equal to the length of the digest function output. This is not guaranteed to be the case for classic DSA, so some missing features had to be implemented.

(This commit contains these changes as well as the move to a separate crate. I can split those changes out again if needed, but I've been testing with the in-progress DSA code as well to make sure the interface is correct and things are working. It would take a bit of extra work and by now I just want to get something up for review.)

One of the changes was to the HMAC DRBG code, to produce output strings of arbitrary length (as defined in its standard). This was not a huge change, just adding a loop.

To properly implement the new aspects of RFC6979 the generation code needs to know the bit length of the modulus. I decided it was probably nicer to just pass the whole modulus, so the code can also check that various values are smaller than the modulus.

The modulus is only available via the bitvec-based PrimeField::char_le_bits API, and the value of scalars is also available in this way (via to_le_bits). So I decided to use it for all Scalar to bytes conversion. I would much prefer an API which properly defines the endianness (and I've posted a discussion of this in an issue on the traits repo), but for now this will do.

This PR depends on RustCrypto/traits#565 (just merged) and RustCrypto/traits#566 (not yet merged), otherwise tests will fail because of unimplemented features on the MockCurve used for testing.

The test suite in the ecdsa crate is very minimal (and does not exercise the new features needed for DSA) and I've added no dedicated tests for the new crate. However, the new DSA code has many tests based on official test vectors with many combinations of digest and key length, and these do exercise all the features except for moduli with a bit length not divisible by 8.

This is currently at a "ready for review" state and I've tested it fairly well, but I would not be surprised to find something missing a bit of polish.

This is the deterministic k (nonce) generation used by `ecdsa`. It is being split out because it will also be used in to upcoming `dsa` implementation.

tarcieri · 2021-02-26T21:11:37Z

ecdsa/src/rfc6979.rs

+ let bits_to_bytes = |b: elliptic_curve::ScalarBits<C>| {
+ // make byte buffer of the right size
+ let mut tmp = FieldBytes::<C>::default();
+ // convert bitslice from internal repr based on u32 or u64 to one based on u8
+ tmp.as_mut_slice()
+ .view_bits_mut::<Lsb0>()
+ .clone_from_bitslice(b.as_bitslice());
+ // convert to big endian
+ tmp.reverse();
+ tmp
+ };
+
+ // Verify that values roundtrip through bits_to_bytes and from_repr as expected.
+ // The from_repr interface does not guarantee big endian interpretation, and if it is not
+ // as expected the generated value may not have the required entropy.
+ let one = Scalar::<C>::one();
+ let one_bytes = bits_to_bytes(one.to_le_bits());
+ assert!(
+ one_bytes[one_bytes.len() - 1] == 1 && Scalar::<C>::from_repr(one_bytes) == Some(one),
+ "curve repr not as expected"
+ );
+
+ // Convert inputs to big endian byte representation
+ let modulus = bits_to_bytes(Scalar::<C>::char_le_bits());
+ let h1 = bits_to_bytes(Scalar::<C>::from_digest(msg_digest).to_le_bits());
+ let mut x = bits_to_bytes(secret_scalar.to_le_bits());


All of this feels like complexity that's trying to work around the potential that to_repr will ever be anything other than big endian.

That will never be the case for ECDSA, which is to say that ECDSA is undefined for any curve with a non-big-endian field element representation for either the scalar or base fields, and the chances of that changing at any point in the future is, for all intents and purposes, zero.

If you'd prefer to have a separate trait defined by the ecdsa crate which by contract will always emit a big endian-serialized FieldBytes, we can define that, but really I don't think there's any need for any of this, and separately there shouldn't be any need to loop in any sort of bit-level views of field elements.

I agree that ECDSA with the wrong kind of representation does not make sense. I just want to make sure it explodes instead of silently doing the wrong thing.

I also agree on not wanting to deal with bitvec. The endian thing is not the only reason why I need it though! The char_le_bits API is the only way to get the modulus of the prime field, and it returns a bitvec based type too. (Edit: well, I don't need the modulus, I just need the number of significant bits in the modulus to implement RFC6979 correctly. I guess I can do without. I'd lose the ability to validate the digest has been reduced correctly, but that's not critical anyway.)

Also, worth pointing out that there is already an ECDSA specific trait: FromDigest. Perhaps it would be better to turn it into an ECDSACompatible trait or something, and add "must have a big endian repr" to its contract? I kind of hate the name, but just to get the idea across.

I agree that ECDSA with the wrong kind of representation does not make sense. I just want to make sure it explodes instead of silently doing the wrong thing.

The requirements for integrating the ecdsa crate with a particular curve implementation are a pretty tall order right now. We made the API deliberately coarse-grained both to allow curve implementations to hide certain types from the public API (i.e. base field elements) but also permitting optimizations that are only possible by leveraging potentially unique features of curve arithmetic backends.

https://docs.rs/ecdsa/0.10.2/ecdsa/hazmat/trait.SignPrimitive.html

https://docs.rs/ecdsa/0.10.2/ecdsa/hazmat/trait.VerifyPrimitive.html

Concrete implementations end up looking like this:

https://github.com/RustCrypto/elliptic-curves/blob/master/k256/src/ecdsa/sign.rs#L211-L237

https://github.com/RustCrypto/elliptic-curves/blob/master/k256/src/ecdsa/verify.rs#L110-L130

All of these things are explicitly called out as hazmat in the API and the bar is pretty high for even getting a particular implementation working in the first place, so I'm not too particularly worried that someone will manage to produce an ECDSA implementation which is otherwise working but manages to get everything else but endianness wrong.

Ah, that makes a lot of sense. I had seen those traits briefly because I am just starting to think about what kind of public interface I want for the dsa code (it started from a very different place, with everything dynamic in the rpgp style instead of the mostly static dispatch style more common in RustCrypto), but had not really gone through them or considered why they are the way they are. This is good background information also for that work.

I'll take some time to digest this and then see how far I get making something we're both happy with.

rvolgers

Added some notes

rvolgers · 2021-02-26T20:58:24Z

ecdsa/Cargo.toml

@@ -18,6 +18,8 @@ der = { version = "0.2", optional = true, features = ["big-uint"] }
 elliptic-curve = { version = "0.9", default-features = false }
 hmac = { version = "0.10", optional = true, default-features = false }
 signature = { version = ">= 1.3.0, < 1.4.0", default-features = false, features = ["rand-preview"] }
+rfc6979 = {path = "../rfc6979" }
+bitvec = { version = "0.20.0", default-features = false }


Adding an explicit dependency on bitvec here because ff only re-exports a very limited part of the API and we need more. Perhaps it would be better if ff did a pub use bitvec or similar so we can get what we need via ff and be sure that we get the same version.

Per zkcrypto/ff#42 I think it'd be nice if we could make the bitvec dependency optional and if that's possible I would like for it not to be required for ECDSA

I also think this would be nice.

rvolgers · 2021-02-26T21:06:21Z

ecdsa/src/rfc6979.rs

+ // Verify that values roundtrip through bits_to_bytes and from_repr as expected.
+ // The from_repr interface does not guarantee big endian interpretation, and if it is not
+ // as expected the generated value may not have the required entropy.
+ let one = Scalar::<C>::one();
+ let one_bytes = bits_to_bytes(one.to_le_bits());
+ assert!(
+ one_bytes[one_bytes.len() - 1] == 1 && Scalar::<C>::from_repr(one_bytes) == Some(one),
+ "curve repr not as expected"
+ );


This is obviously kind of ugly, but I don't see how I can avoid doing something like this with the current APIs, at least not without leaving a gaping hole in the API allowing for misuse. In theory at least the compiler should be able to fully evaluate this at compile time, but I kind of doubt it's that good.

rvolgers · 2021-02-26T21:09:17Z

ecdsa/src/rfc6979.rs

+
+ // Convert inputs to big endian byte representation
+ let modulus = bits_to_bytes(Scalar::<C>::char_le_bits());
+ let h1 = bits_to_bytes(Scalar::<C>::from_digest(msg_digest).to_le_bits());


It might be nicer to pass the Digest down all the way to the new crate instead of converting everything to byte buffers here. Unfortunately the new crate can't use the FromDigest trait, because it's declared in ecdsa and depending on ecdsa would create a circular dependency between the ecdsa and rfc6079 crates.

If it's helpful to push the FromDigest trait down to the rfc6979 crate, I guess I'd be ok with having it there.

As I mentioned earlier in ECDSA the specific transformation is a core part of the algorithm and not specifically related to RFC6979, but what ECDSA does and what RFC6979 needs are the same, so I guess I wouldn't be opposed to moving it.

ecdsa/src/sign.rs

rfc6979/src/hmac_drbg.rs

rvolgers · 2021-02-26T21:15:14Z

rfc6979/src/lib.rs

+ /// The modulus must be in big endian format with no padding (i.e. first byte must not be 0).
+ ///
+ /// This function will panic if these requirements are not met.
+ pub fn new<T: AsRef<[u8]>>(modulus: I, secret: T, digest: T, additional_data: &[u8]) -> Self {


Separate generics I and T is a bit ugly here, I think the idea was to allow user to pick an owned or borrowed representation for the modulus?

rvolgers · 2021-02-26T21:16:41Z

rfc6979/src/lib.rs

+ assert!(secret.len() == modulus.len() && secret < modulus);
+ assert!(digest.len() == modulus.len() && digest < modulus);


This pattern is also used in generate_into: if the length of two big-endian byte buffers is equal, doing a numeric comparison is equivalent to lexical comparison between the buffers.

tarcieri · 2021-11-21T18:29:41Z

Closing in favor of #409

Split out RFC6979 implementation to its own crate

b380915

This is the deterministic k (nonce) generation used by `ecdsa`. It is being split out because it will also be used in to upcoming `dsa` implementation.

tarcieri reviewed Feb 26, 2021

View reviewed changes

rvolgers commented Feb 26, 2021

View reviewed changes

tarcieri closed this Nov 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split out RFC6979 implementation to its own crate #257

Split out RFC6979 implementation to its own crate #257

rvolgers commented Feb 26, 2021 •

edited

Loading

tarcieri Feb 26, 2021 •

edited

Loading

rvolgers Feb 26, 2021 •

edited

Loading

rvolgers Feb 26, 2021

tarcieri Feb 26, 2021 •

edited

Loading

rvolgers Feb 26, 2021

rvolgers left a comment

rvolgers Feb 26, 2021

tarcieri Feb 26, 2021

rvolgers Feb 26, 2021

rvolgers Feb 26, 2021

rvolgers Feb 26, 2021

tarcieri Feb 26, 2021

rvolgers Feb 26, 2021

rvolgers Feb 26, 2021

tarcieri commented Nov 21, 2021

		assert!(secret.len() == modulus.len() && secret < modulus);
		assert!(digest.len() == modulus.len() && digest < modulus);

Split out RFC6979 implementation to its own crate #257

Split out RFC6979 implementation to its own crate #257

Conversation

rvolgers commented Feb 26, 2021 • edited Loading

tarcieri Feb 26, 2021 • edited Loading

Choose a reason for hiding this comment

rvolgers Feb 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarcieri Feb 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvolgers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarcieri commented Nov 21, 2021

rvolgers commented Feb 26, 2021 •

edited

Loading

tarcieri Feb 26, 2021 •

edited

Loading

rvolgers Feb 26, 2021 •

edited

Loading

tarcieri Feb 26, 2021 •

edited

Loading