search

paulmillr / noble-curves

Audited & minimal JS implementation of elliptic curve cryptography.
https://paulmillr.com/noble
MIT License
623 stars 56 forks source link

fix: check extraEntropy according to the spec #56

Closed mahnunchik closed 1 year ago

mahnunchik commented 1 year ago

55

paulmillr commented 1 year ago

Length of Fp.bytes are for compatibility with libsecp256k1: https://github.com/bitcoin-core/secp256k1/blob/67214f5f7d8e0bdf193ceb1f47ba8277d1a0871a/src/secp256k1.c#L476

Because the arguments have distinct fixed lengths it is not possible for different argument mixtures to emulate each other and result in the same nonces.

Do you need longer value for something, or this is just fixing spec non-compliance?

mahnunchik commented 1 year ago

I'm moving from our lovely elliptic module...

For the EOS crypto 1 byte additional data is used. So I checked the spec and realized that it is not mandatory for additional data to be 32 bytes.

According to the comment from libsecp256k1 https://github.com/bitcoin-core/secp256k1/blob/67214f5f7d8e0bdf193ceb1f47ba8277d1a0871a/src/secp256k1.c#L477 additional 16 bytes may be appended too.

paulmillr commented 1 year ago

For the EOS crypto 1 byte additional data is used. So I checked the spec and realized that it is not mandatory for additional data to be 32 bytes.

if the byte is e.g. de, you can do this: 00000000000000000000000000000000000000000000000000000000000000de. Does this work?

With regards to 16-byte algorithm name: that's true. I did not add it, because extraEntropy is used for low-r signatures, which is common in btc, while algo16 does not seem to be popular.

mahnunchik commented 1 year ago

Yes, it is possible to produce valid EOS signature by using 00000000000000000000000000000000000000000000000000000000000000de but not the same signature as original EOS code produces.

I think it is better to follow specification and check that extraEntropy is bytes but not check the size.

paulmillr commented 1 year ago

I agree with you.

but not the same signature as original EOS code produces

That's weird. I would have thought they are the same - which would have worked for now.

paulmillr commented 1 year ago

Could you add a test for this, please?

paulmillr commented 1 year ago

@mahnunchik ping

mahnunchik commented 1 year ago

@paulmillr done

paulmillr commented 1 year ago

Why was .json changed? I think we're having it in repo because it was copied from some other library. Can't we test the dynamic size without changing json?

mahnunchik commented 1 year ago

Yep, additional test may be added just to test different entropy sizes.

paulmillr commented 1 year ago

Ok, please add it and revert json change and we'll be good to go.

mahnunchik commented 1 year ago

@paulmillr please check the changes