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

Point Decompression does not enforce field points are less than field modulus. #15

Closed
kirk-baird opened this issue Jul 29, 2020 · 8 comments

Comments

@kirk-baird
Copy link
Contributor

What is the issue?

When decompression a point each specific field point should be strictly less than the field modulus.
For example a min PublicKey should have the x coordinate strictly less than the field modulus. PublicKey.X < P

If this is not enforced, signature malleability may occur by taking Signature.X[0] from a valid signature and doing Signature.X[0] + P which will equate to the same point thus providing signature malleability.

What needs to be done?

I think this check out be easiest to do on the limbs just after converting bytes to words. That is in src/vect.h::limbs_from_be_bytes(). That way a simple loop can be made checking each word from the most significant word is less than, equal to or greater than the equivalent word in the field modulus.
This could break the loop if a word is less than, error/exit if a word is greater than and continue if a word is equal to the field modulus word.

@dot-asm
Copy link
Collaborator

dot-asm commented Jul 29, 2020

As for suggested solution, no, it would be more appropriate to add zero and see if the result is different from input. I'm also a bit torn on suggestion that signature would be malleable. I mean malleability implies that at least it would be a signature for a different message, and at most you'd be able to compute this different message. But signature modified as suggested would be valid for the original message...

This is not to say that non-reduced coordinates should be accepted, but then why would it apply just to X in compressed format? It sounds more like that all coordinates should be vetted and "bad encoding" returned if non-reduced one is encountered.

@JustinDrake
Copy link

each specific field point should be strictly less than the field modulus

Is this constraint specified in the IETF spec?

@dot-asm
Copy link
Collaborator

dot-asm commented Jul 29, 2020

Is this constraint specified in the IETF spec?

If this is a reference to bls signature draft, then it probably can be classified as kind of a trick question. Because the [original] question here is rather a matter of deserialization, while it, the serdes, is not direct part of the said spec. It's specified only indirectly as "whatever [ZCash] says." And the referred section doesn't say much about how stringent the deserializtion procedure should be. As result one can probably argue both ways, i.e. non-reduced inputs should be rejected or may be tolerated. Naturally as long as it doesn't matter from cryptographic viewpoint, which is arguably the case. At least it's the case in the 'blst' context, because the moment non-reduced coordinates are converted to internal representation, they are implicitly reduced. In other words X and X+P yield exactly same internal representation. I naturally can't claim that it's universally the case, but as long as inputs are treated in "true modulo-arithmetic" manner, it shouldn't matter. (Just in case, the quotes are because it's not a real term:-) However, one can naturally argue in favour of "should be rejected" from purely pragmatic purposes, the serialization should be unambiguous.

@JustinDrake
Copy link

JustinDrake commented Jul 29, 2020

It's specified only indirectly as "whatever [ZCash] says." And the referred section doesn't say much about how stringent the deserializtion procedure should be.

What does the ZCash implementation do?

@dot-asm
Copy link
Collaborator

dot-asm commented Jul 29, 2020

What does the ZCash implementation do?

Note that the draft says "whatever it defines", not what any particular implementation does:-) Either way, the referred section refers to "field elements" and one can legitimately say that the term describes a value less than corresponding modulus. And consequently conclude that non-reduced values should not be tolerated. But this is not what I kind of objected, I objected assertion about malleability and applying the restriction to compressed format alone.

@kirk-baird
Copy link
Contributor Author

Yep you're correct this isn't malleability as both signatures are related to a single message. It is actually just an matter of the injectiveness of field element (and thus also elliptic curve point) deserialisation.

Sorry if I didn't communicate this clearly, I intended to raise this as an issue of field element deserialisation in general rather than strictly the uncompress() function. I made an example of the uncompress() function as deserialise() is already not injective (as you may have compressed or uncompressed forms of points).

The ZCash implementation enforces that a finite field element is less than the modulus here.

I think it's beneficial to have uncompress() and compress() functions which are injective. This allows you to take full advantage of the fact the signatures are deterministic. For example drand use a threshold bls signature as input to a hash function to generate randomness, having two representations of the signatures allows for two different outputs of the randomness function. Other benefits are that a network packet with a message and signature will always have the same bytes if the signature uncompression is injective. Thus this will prevent accidental introduction of bugs in implementations for assuming two messages are different due to having different bytes.

@dot-asm
Copy link
Collaborator

dot-asm commented Aug 1, 2020

having two representations of the signatures

"Having" suggests that blst serialization could emit either. And this is not the case, it always produces fully reduced data. More appropriate wording would be "accepting two representations." And implication would be that non-reduced data would be modified by an adversary.

@dot-asm
Copy link
Collaborator

dot-asm commented Aug 3, 2020

Fix is committed. Closing this. Thanks!

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