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

HashToField does not follow RFC9380 depending on the curve modulus #849

Open
azixus opened this issue Jul 30, 2024 · 2 comments
Open

HashToField does not follow RFC9380 depending on the curve modulus #849

azixus opened this issue Jul 30, 2024 · 2 comments

Comments

@azixus
Copy link

azixus commented Jul 30, 2024

While implementing hash to curve for an unsupported curve, I've discovered a bug in the implementation of DefaultFieldHasher:

let len_per_base_elem = get_len_per_elem::<F, SEC_PARAM>();
let expander = ExpanderXmd {
hasher: PhantomData,
dst: dst.to_vec(),
block_size: len_per_base_elem,
};

When ExpanderXmd is initialized, len_per_base_elem is passed as the block size of the underlying hash function. It works seamlessly for curves with 381 bit modulus, because len_per_base_elem = ceil((MODULUS + 128)/8) == 64 which is the block size of SHA256. Since the block size is a parameter used in expand_message_xmd, the current implementation cannot pass test vectors when the above condition is false (e.g. 256 bits modulus).

See an example of the issue on secp256k1 at https://gist.github.com/azixus/2edc9485ecd386578aff0bd3f9a71b84

The digest crate provides a block_size() function through the OutputSizeUser trait. I wrote a fix that further restricts the generic parameter to that trait here

I'll happily raise a PR if that change seems acceptable.

@skaunov
Copy link

skaunov commented Nov 11, 2024

Let me add mine path here to promote the issue and record the finding (probably yours already has it though).
Tbh, I peeked that https://github.com/arkworks-rs/algebra/blob/master/ff/src/fields/field_hashers/expander/testdata/expand_message_xmd_SHA256_38.json has the values for the suite I use and hence was searching for a mistake in my code.
So.

use ark_ff::field_hashers::{DefaultFieldHasher, HashToField};
let defhasher: DefaultFieldHasher<Sha256> = 
    <DefaultFieldHasher::<Sha256> as HashToField<ark_secp256k1::Fq>>::new(
        b"QUUX-V01-CS02-with-secp256k1_XMD:SHA-256_SSWU_RO_"
    );
let u: [ark_secp256k1::Fq; 2] = defhasher.hash_to_field::<2>(&[]);
println!("{}", u[0]);

gives "48425033926223359121679389614872723077618800904285921194876400224709273202611"
the correct one is 4424703167977793061848381150098601127251783968775290647828255579651460940578
https://www.rfc-editor.org/rfc/rfc9380.html#name-expand_message_xmdsha-256

@burdges
Copy link
Contributor

burdges commented Nov 12, 2024

Yeah, I asked if digest::core_api::BlockSizeUser made sense when I attempted a refactor in #643, now very stale. I guess so..

As an aside, we still cannot afaik support field hashers besides sha256. In theory, this should become problematic, since shake128 xof is recommended for future curves. I still think some refactor around the digest::XofReader trait makes the most sense, but that's a large change. Also, we should expose the map_to_curve part, because if possible folks should use Posideon inside a SNARK. None of that is relevant here though.

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

No branches or pull requests

3 participants