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

bls12-381: Add edge case signature #549

Merged
merged 2 commits into from
Sep 23, 2024

Conversation

Kubuxu
Copy link
Contributor

@Kubuxu Kubuxu commented Sep 17, 2024

This signature does not validate when using kilic@master
Add it to tests to highlight the issue and prevent this implementation from updating to it.
It was detected in a live system.

The issue appears in this commit: kilic/bls12-381@7536ae1 and is not related to assembly code (it reproduces even on M series Macs).

Additionally, synchronise the default DST values between circl and kilic.

circl was using nil DST while kilic is using RFC DST, standardise on RFC
DST

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
@CLAassistant
Copy link

CLAassistant commented Sep 17, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer.

Copy link

🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer.

func (p *G2Elt) Hash(msg []byte) kyber.Point { p.inner.Hash(msg, nil); return p }
var domainG2 = []byte("BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_NUL_")

func (p *G2Elt) Hash(msg []byte) kyber.Point { p.inner.Hash(msg, domainG2); return p }
func (p *G2Elt) Hash2(msg, dst []byte) kyber.Point { p.inner.Hash(msg, dst); return p }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we really need Hash2 still in V4 if we make the Hash take a DST arg by default. Cc @K1li4nL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there are good reasons to have this flexibility I suppose we can get rid of Hash2() and have a default DST yes

@@ -97,5 +97,7 @@ func (p *G1Elt) Mul(s kyber.Scalar, q kyber.Point) kyber.Point {

func (p *G1Elt) IsInCorrectGroup() bool { return p.inner.IsOnG1() }

func (p *G1Elt) Hash(msg []byte) kyber.Point { p.inner.Hash(msg, nil); return p }
var domainG1 = []byte("BLS_SIG_BLS12381G1_XMD:SHA-256_SSWU_RO_NUL_")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the DST for the curves shouldn't be the ones as per RFC9380, and the BLS signature DST shouldn't be specified in the BLS and BDN packages rather.
Cc @K1li4nL as well.

Copy link
Contributor

@K1li4nL K1li4nL Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally yes, this would allow us to include the RFC test vectors easily for these curves

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was to match the behaviour of circl to kilic. If there is anything I can do here, please don't hesitate to let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the current interface (I guess that is the change you discussed in the other thread), there is no way for the BLS package to specify the DST, otherwise I would have done it there.

This signature does not validate when using kilic@master
Add it to tests to highlight the issue and prevent this implementation
from updating to it.

The issue appears in this commit: kilic/bls12-381@7536ae1
and is not related to assembly code (it reproduces even on M series macs)

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
Copy link

sonarcloud bot commented Sep 18, 2024

@AnomalRoil AnomalRoil merged commit dbb811a into dedis:master Sep 23, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants