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

Fix/malleability sig #449

Merged
merged 24 commits into from
Sep 27, 2023
Merged

Fix/malleability sig #449

merged 24 commits into from
Sep 27, 2023

Conversation

ThomasPiellard
Copy link
Contributor

@ThomasPiellard ThomasPiellard commented Sep 15, 2023

Description

Deserialisation eddsa and ecdsa signature now ensures that the data is in [0, mod] where mod is the relevant modulus (field of definition of relevant group size of the curve).

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Credits to Verichains for raising the issue

@github-actions
Copy link

Summary

✅ Passed: 5571
❌ Failed: 0
🚧 Skipped: 7

🚧 Skipped

  • TestReference (github.com/consensys/gnark-crypto/ecc/bn254/fr/sis)
  • TestLimbDecomposition (github.com/consensys/gnark-crypto/ecc/bn254/fr/sis)
  • TestReference (github.com/consensys/gnark-crypto/ecc/bn254/fr/swifft)
  • TestLimbDecomposition (github.com/consensys/gnark-crypto/ecc/bn254/fr/swifft)
  • TestAppend (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)
  • TestAppendSis (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)
  • TestCommitmentSis (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)

ecc/bn254/fr/swifft/utils.tmp Outdated Show resolved Hide resolved
ecc/bls12-381/playground/main.go Outdated Show resolved Hide resolved
ecc/bls12-377/twistededwards/eddsa/marshal.go Outdated Show resolved Hide resolved
ecc/bls12-377/twistededwards/eddsa/marshal.go Show resolved Hide resolved
ecc/bls12-377/twistededwards/eddsa/marshal.go Show resolved Hide resolved
ecc/bls12-377/twistededwards/eddsa/marshal.go Show resolved Hide resolved
ecc/bls12-377/ecdsa/marshal.go Show resolved Hide resolved
ecc/bls12-377/ecdsa/marshal.go Show resolved Hide resolved
ecc/bls12-377/ecdsa/marshal.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

See comments.

@github-actions
Copy link

Summary

✅ Passed: 5561
❌ Failed: 0
🚧 Skipped: 5

🚧 Skipped

  • TestAppend (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)
  • TestReference (github.com/consensys/gnark-crypto/ecc/bn254/fr/sis)
  • TestLimbDecomposition (github.com/consensys/gnark-crypto/ecc/bn254/fr/sis)
  • TestAppendSis (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)
  • TestCommitmentSis (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)

@ThomasPiellard ThomasPiellard marked this pull request as ready for review September 15, 2023 15:11
@github-actions
Copy link

Summary

✅ Passed: 5607
❌ Failed: 0
🚧 Skipped: 5

🚧 Skipped

  • TestReference (github.com/consensys/gnark-crypto/ecc/bn254/fr/sis)
  • TestLimbDecomposition (github.com/consensys/gnark-crypto/ecc/bn254/fr/sis)
  • TestAppend (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)
  • TestAppendSis (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)
  • TestCommitmentSis (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)

1 similar comment
@github-actions
Copy link

Summary

✅ Passed: 5607
❌ Failed: 0
🚧 Skipped: 5

🚧 Skipped

  • TestReference (github.com/consensys/gnark-crypto/ecc/bn254/fr/sis)
  • TestLimbDecomposition (github.com/consensys/gnark-crypto/ecc/bn254/fr/sis)
  • TestAppend (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)
  • TestAppendSis (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)
  • TestCommitmentSis (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)

@ivokub
Copy link
Collaborator

ivokub commented Sep 19, 2023

Rereviewed - we still accept zero signatures. For example, following doesn't fail:

func TestZero(t *testing.T) {
	var signature Signature
	var bts [sizeSignature]byte
	_, err := signature.SetBytes(bts[:])
	if err == nil {
		t.Fatal("expected error")
	}
}

ecc/bn254/fr/sis/test_cases_backup.json Outdated Show resolved Hide resolved
@github-actions
Copy link

Summary

✅ Passed: 5607
❌ Failed: 0
🚧 Skipped: 5

🚧 Skipped

  • TestReference (github.com/consensys/gnark-crypto/ecc/bn254/fr/sis)
  • TestLimbDecomposition (github.com/consensys/gnark-crypto/ecc/bn254/fr/sis)
  • TestAppend (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)
  • TestAppendSis (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)
  • TestCommitmentSis (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)

@github-actions
Copy link

Summary

✅ Passed: 5883
❌ Failed: 0
🚧 Skipped: 5

🚧 Skipped

  • TestReference (github.com/consensys/gnark-crypto/ecc/bn254/fr/sis)
  • TestLimbDecomposition (github.com/consensys/gnark-crypto/ecc/bn254/fr/sis)
  • TestAppend (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)
  • TestAppendSis (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)
  • TestCommitmentSis (github.com/consensys/gnark-crypto/ecc/bn254/fr/tensor-commitment)

Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

I made some updates - particularly checking against 0 values as this leads to div by zero elsewhere.

Tests work, but would be nice to have a confirmation.

And I checked that negative values are not issues as incoming bytes are represented as unsigned ints.

@ThomasPiellard ThomasPiellard merged commit 81ffe9c into master Sep 27, 2023
7 checks passed
@ThomasPiellard ThomasPiellard deleted the fix/malleability_sig branch September 27, 2023 13:36
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

Successfully merging this pull request may close these issues.

None yet

2 participants