feat: add traits for group digest #865

mikelodder7 · 2022-01-05T04:47:36Z

Traits for hash to curve in groups and fields

daxpedda

I think this is a bit too narrowly targeted towards P-256 & co., I believe a CurveToMap trait to fill the gap would alleviate that problem.

elliptic-curve/src/group_digest.rs

elliptic-curve/src/hash2field.rs

daxpedda · 2022-01-05T05:29:20Z

elliptic-curve/src/group_digest.rs

+    ///
+    /// let pt = ProjectivePoint::hash_from_bytes::<hash2field::ExpandMsgXof<sha3::Shake256>>(b"test data", b"CURVE_XOF:SHAKE-256_SSWU_RO_");
+    ///
+    fn hash_from_bytes<X: ExpandMsg, M: AsRef<[u8]>>(msg: M, dst: &'static [u8]) -> Self::Output {


Suggested change

fn hash_from_bytes<X: ExpandMsg, M: AsRef<[u8]>>(msg: M, dst: &'static [u8]) -> Self::Output {

fn hash_from_bytes<X: ExpandMsg, M: AsRef<[u8]>>(msg: M, dst: &[u8]) -> Self::Output {

Is there a reason this is 'static?

Yes. HashToCurve requires handling a DST longer than 255 chars. See Domain. Having implemented this multiple times across various languages and libraries, 99% (I've never seen a dynamic one yet but there must be one somewhere) of the time the domain is static or known and not composed at run time. To be no-std compliant I can't use Vec. That leaves a couple of options: use [u8; 255] or GenericArray<u8, U255>, use &'a [u8] or &'static [u8]. The first option allocates more than enough space to handle any DST < 255 but overly so since they are usually < 32. This also requires copying the DST into the other array which is not really necessary. That leaves the other two. The first &'a [u8] requires adding the lifetime to all traits up the chain and more complex work to make Rust happy. This is undesirable and complicated. Thus the use of &'static [u8] which is not that strict of a requirement and keeps the API clean.

I actually thought that &'a [u8] was a perfectly fine solution here. This can be improved in a follow-up PR, not really a big issue.

elliptic-curve/src/group_digest.rs

daxpedda · 2022-01-06T00:47:45Z

elliptic-curve/src/group_digest.rs

+pub trait GroupDigest {
+    /// The field element representation for a group value with multiple elements
+    type FieldElement: FromOkm + Default + Copy;
+    /// The resulting group element
+    type Output: CofactorGroup<Subgroup = Self::Output>
+        + MapToCurve<FieldElement = Self::FieldElement, Output = Self::Output>;


After some thinking, I'm not sure this trait actually adds anything useful. All functions are already provided by default and the associated types can be derived from other traits.

For example, this blanket impl would work:

impl<C: PrimeCurveArithmetic + ScalarArithmetic> GroupDigest for C where C::Scalar: FromOkm + Default + Copy, C::CurveGroup: CofactorGroup<Subgroup = C::CurveGroup> + MapToCurve<FieldElement = C::Scalar, Output = C::CurveGroup>, { type FieldElement = C::Scalar; type Output = C::CurveGroup; }

I would actually suggest making these free standing functions, like diffie_hellman or hash_to_field below. It could also be added to an existing trait behind where bounds.

fn hash_from_bytes<C: PrimeCurveArithmetic + ScalarArithmetic, X: ExpandMsg>( msg: &[u8], dst: &'static [u8], ) -> C::CurveGroup where C::Scalar: FromOkm, C::CurveGroup: CofactorGroup<Subgroup = C::CurveGroup> + MapToCurve<FieldElement = C::Scalar, Output = C::CurveGroup>, { let mut u = [C::Scalar::default(), C::Scalar::default()]; hash_to_field::<X, _>(msg, dst, &mut u); let q0 = C::CurveGroup::map_to_curve(u[0]); let q1 = C::CurveGroup::map_to_curve(u[1]); q0.clear_cofactor() + q1.clear_cofactor() }

WDYT?

I like where you're going with this but I prefer the methods be directly accessible on the traits if possible rather than standalone functions.

elliptic-curve/src/group_digest.rs

daxpedda · 2022-01-06T01:28:19Z

elliptic-curve/src/hash2field/expand_msg_xmd.rs

+                .chain(tmp)
+                .chain([self.index as u8])
+                .chain(self.domain.data())
+                .chain([self.domain.len() as u8])


Can't this overflow? As far as I understand this depends on the OutputSize of the hash, I don't see anything preventing it from being bigger then 255? If this is required, it could be enforced on the type level with a where bound.

You can use typenum's IsLess or IsLessOrEqual traits to express that constraint, e.g.

where HashT: Digest + BlockInput, HashT::OutputSize: IsLess<U256>

daxpedda · 2022-01-06T02:35:37Z

elliptic-curve/src/hash2field/expand_msg_xmd.rs

+        let ell = (len_in_bytes + b_in_bytes - 1) / b_in_bytes;
        if ell > 255 {
            panic!("ell was too big in expand_message_xmd");
        }


I played around with how to circumvent this locally and I achieved some pretty ugly code to check this on a type level.

So I believe the goal is to keep these functions infallible, certainly not panicking. I don't know how else to do this without doing some ugly where bounds on the length. To do this we will need to keep the previous implementation you had of storing the LEN_IN_BYTES as a generic (probably using ArrayLength instead of const usize).

That way we can put all these kind of checks on the ExpandMsgXmd type itself and keep functions infallible.
Another check that's missing here:
https://www.ietf.org/archive/id/draft-irtf-cfrg-hash-to-curve-13.html#section-5.4.1-4

[..] For correctness, H requires b <= s.

This could be easily checked on the type level: HashT::OutputSize: IsLessOrEqual<HashT::BlockSize>.

The constraint pointed above that is currently handled by panicking:
https://www.ietf.org/archive/id/draft-irtf-cfrg-hash-to-curve-13.html#section-5.4.1-6

len_in_bytes, the length of the requested output in bytes,
not greater than the lesser of (255 * b_in_bytes) or 2^16-1.

Would be a bit harder, but not impossible.

Alternatively of course, we can just return a Result, but panicking is not a long-term solution, or am I missing something? Maybe I'm overcautious or not understanding something properly.

WDYT? I'm willing to help, either by making follow-up PR's or PR against your fork @mikelodder7.

A follow PR would be great if we can somehow enforce it with type bounds. I'm not sure how to do this cleanly though. We could just change it to be a Result and declare it good enough. The odds of this happening are pretty low when using any hash function from RustCrypto. That won't stop someone from shooting themselves if they have a custom function that has a ridiculous OutputSize

Maybe an easy solution would be just limit the Outputsize to something sane and be done with it. But yes, let's clear that in a follow-up.

elliptic-curve/src/hash2field/expand_msg_xmd.rs

elliptic-curve/src/hash2field/expand_msg_xof.rs

Signed-off-by: Michael Lodder <[email protected]>

mikelodder7 · 2022-01-06T20:03:09Z

I propose we merge this PR but have a follow-up to address before publishing the update to the crate:

Limit HashT::OutputSize so it can't possibly result in an ell larger than 255. This could possibly be done with Traits somehow. @daxpedda I'm curious about your approach to this.

We could also try to do the blanket impl. This could be done by breaking GroupDigest into two traits where the first requires to implement the required const values and the 2nd is the blanket impl.

daxpedda · 2022-01-06T20:15:31Z

I agree. It would be great to keep RustCrypto/elliptic-curves#495 up-to-date to see how it actually plays out. Preferably also run it over the test-vectors.

I will find some time to make a follow-up PR with some experiments on how to do the constraint for ell.

elliptic-curve/Cargo.toml

tarcieri · 2022-01-06T20:50:59Z

Perhaps it would make sense to put all of this functionality in a hash2curve module to keep it organized and avoid polluting the toplevel crate namespace.

It would also make gating for conditional compilation easier.

It seems like modules like osswu could be placed in there as well? (or hash2field)

mikelodder7 · 2022-01-06T21:26:10Z

@tarcieri Moved all traits to hash2curve module. digest has been restored and now these traits are gated under the hash2curve feature now.

elliptic-curve/Cargo.toml

elliptic-curve/src/hash2curve.rs

elliptic-curve/src/lib.rs

tarcieri · 2022-01-06T21:41:17Z

I propose we merge this PR but have a follow-up to address before publishing the update to the crate:

Sounds good to me!

A few nits but otherwise this seems like a reasonable start.

I can hold off on any releases until you've had time to play around with the RustCrypto/elliptic-curves PR and are happy with the API.

mikelodder7 · 2022-01-07T02:12:10Z

Done w/nits @tarcieri

tarcieri · 2022-01-07T02:13:46Z

@mikelodder7 had one more suggestion for avoiding overflow here: #865 (comment)

daxpedda · 2022-01-07T02:21:52Z

elliptic-curve/src/hash2field/expand_msg_xmd.rs

-            .chain([(LEN_IN_BYTES >> 8) as u8, LEN_IN_BYTES as u8, 0u8])
-            .chain(dst)
-            .chain([dst.len() as u8])
+            .chain([(len_in_bytes >> 8) as u8, len_in_bytes as u8, 0u8])


If we want to solve overflows in this PR, this can overflow too.

Signed-off-by: Michael Lodder <[email protected]>

tarcieri · 2022-01-07T03:41:11Z

Going to go ahead and merge this and follow up with a small PR for some of the remaining nits

As noted on #865 there are some potential overflow problems constructing the messages used for expansion, particularly around input lengths. This commit changes `ExpandMsg::expand_message`, and all of its transitive callers, to be fallible in order to handle these cases.

daxpedda suggested changes Jan 5, 2022

View reviewed changes

mikelodder7 force-pushed the master branch from 5d3158b to ae5033a Compare January 5, 2022 22:28

daxpedda suggested changes Jan 6, 2022

View reviewed changes

mikelodder7 force-pushed the master branch 3 times, most recently from c59e45e to 302fb0a Compare January 6, 2022 15:55

feat: add traits for group digest

37bf44a

Signed-off-by: Michael Lodder <[email protected]>

mikelodder7 force-pushed the master branch from 302fb0a to 37bf44a Compare January 6, 2022 19:56

mikelodder7 requested a review from tarcieri January 6, 2022 20:03

tarcieri reviewed Jan 6, 2022

View reviewed changes

elliptic-curve/Cargo.toml Outdated Show resolved Hide resolved

tarcieri reviewed Jan 6, 2022

View reviewed changes

elliptic-curve/Cargo.toml Outdated Show resolved Hide resolved

tarcieri reviewed Jan 6, 2022

View reviewed changes

elliptic-curve/src/hash2curve.rs Show resolved Hide resolved

tarcieri reviewed Jan 6, 2022

View reviewed changes

elliptic-curve/src/lib.rs Show resolved Hide resolved

mikelodder7 force-pushed the master branch from 343f966 to 50fa9f4 Compare January 7, 2022 01:44

mikelodder7 force-pushed the master branch 2 times, most recently from ccd8569 to 83395bf Compare January 7, 2022 02:20

daxpedda reviewed Jan 7, 2022

View reviewed changes

make hash2curve module

9106c29

Signed-off-by: Michael Lodder <[email protected]>

mikelodder7 force-pushed the master branch from 83395bf to 9106c29 Compare January 7, 2022 02:25

tarcieri merged commit 4b5c1e4 into RustCrypto:master Jan 7, 2022

tarcieri mentioned this pull request Jan 7, 2022

elliptic-curve: make ExpandMsg fallible #871

Merged

daxpedda mentioned this pull request Jan 7, 2022

Implement GroupDigest for NistP256 and Secp256k1 RustCrypto/elliptic-curves#503

Merged

daxpedda mentioned this pull request Jan 9, 2022

elliptic-curve: hash-to-curve support #481

Closed

tarcieri mentioned this pull request Jan 14, 2022

elliptic-curve v0.11.7 #882

Merged

	fn hash_from_bytes<X: ExpandMsg, M: AsRef<[u8]>>(msg: M, dst: &'static [u8]) -> Self::Output {
	fn hash_from_bytes<X: ExpandMsg, M: AsRef<[u8]>>(msg: M, dst: &[u8]) -> Self::Output {

feat: add traits for group digest #865

feat: add traits for group digest #865

Uh oh!

Conversation

mikelodder7 commented Jan 5, 2022

Uh oh!

daxpedda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mikelodder7 commented Jan 6, 2022

Uh oh!

daxpedda commented Jan 6, 2022

Uh oh!

Uh oh!

tarcieri commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikelodder7 commented Jan 6, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tarcieri commented Jan 6, 2022

Uh oh!

mikelodder7 commented Jan 7, 2022

Uh oh!

tarcieri commented Jan 7, 2022

Uh oh!

daxpedda Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarcieri commented Jan 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tarcieri commented Jan 6, 2022 •

edited

Loading

daxpedda Jan 7, 2022 •

edited

Loading