Filippo version of Ed25519 #462
Conversation
A deal with a bogus signature would still get stored (before producing
an error). That would prevent a properly signed deal coming later from
being stored. This fixes the bug.
The test was also written in a flawed way. It measured two failure
situations ("wrong index" and "wrong deal") after a success situation. But
written that far down, those measurements could well end up measuring
failures due to a duplicate deal instead. This changeset also patches
the test flaw.
pierluca
left a comment
pierluca
left a comment
There was a problem hiding this comment.
Thanks for the great work. There seem to be a few rough edges that still need taking care of, but I'm in no way a security expert... I'll gladly let my colleagues chime in if they spot other issues :)
| } | ||
|
|
||
| // NewKeyAndSeedWithInput returns a formatted Ed25519 key (avoid subgroup attack by | ||
| // requiring it to be a multiple of 8). It also returns the input and the digest used |
There was a problem hiding this comment.
requiring it to be a multiple of 8 where is this check being done ?
If I understand correctly, lines 53-54 do not seem to guarantee a multiple of 8, counter example: 0
There was a problem hiding this comment.
If I am correct then zero would also be a valid output for what we are trying to do.
There was a problem hiding this comment.
This function seems to have different problems:
- It is not documented.
- I don't understand why it uses
sha512when only 32 bytes are needed;. - I seems to try to implement clamping, but it is wrong.
- It uses
digest[:32]to initialize the scalar, but returnsdigest[32:], which is not the digest used to generate the key.
IMHO the best way to implement this function would be to use SetBytesWithClamping with sha256 to hash the buffer, assuming that's what the function is trying to do. If @parinayc20 adds the appropriate documentation we can continue the discussion.
group/filippo_ed25519/point.go
Outdated
| if data == nil { | ||
| p.Mul(filippoCofactorScalar, p) | ||
| if p.Equal(&filippoNullPoint) { | ||
| continue |
There was a problem hiding this comment.
I assume there's an expectation here that we're just "unlucky", could you document that?
Also, should the for loop provide guarantees that we're going to eventually come out of this loop ? (e.g. throw some kind of error if we've looped over N times)
|
Kudos, SonarCloud Quality Gate passed! |
pierluca
left a comment
pierluca
left a comment
There was a problem hiding this comment.
Besides minor issues, this code seems to be lacking tests.
@si-co do you think we can reuse the existing curve25519 test suite ? Is it sufficient ?
| // If we're using the full group, | ||
| // we just need any point on the curve, so we're done. | ||
| // if c.full { | ||
| // return P,data[dl:] | ||
| // } |
| } | ||
|
|
||
| // NewKeyAndSeedWithInput returns a formatted Ed25519 key (avoid subgroup attack by | ||
| // requiring it to be a multiple of 8). It also returns the input and the digest used |
| } | ||
|
|
||
| func (s *Scalar) String() string { | ||
| b, _ := s.MarshalBinary() |
There was a problem hiding this comment.
here we're not processing the error return ...
| if s.scalar == nil { | ||
| s.scalar = new(filippo_ed25519.Scalar) | ||
| } | ||
| _, err := s.scalar.SetCanonicalBytes(b[:]) |
There was a problem hiding this comment.
What is SetCanonicalBytes being used for here and elsewhere ? The (canonicalized) return value is ignored... 🤔
| if s.scalar == nil { | ||
| s.scalar = new(filippo_ed25519.Scalar) | ||
| } | ||
| _ = s.scalar.Set(a.(*Scalar).scalar) |
There was a problem hiding this comment.
this is a legitimate use of _, as s.scalar.Set just returns s.scalar . A comment would clarify it though.
To manually browse the doc start from sidebar.md. Otherwise visit https://dedis.github.io/kyber/
This PR deals with the addition of the Filippo version of ed25519. It includes wrappers for certain functions and Filippo based definition of others. The table below provides a summary of the functions of Kyber Point and Scalar, which were present in Filippo and which had to be defined.
Point.Equal()YesScalar.Equal()YesPoint.Null()YesScalar.Zero()YesPoint.Base()YesScalar.One()NoPoint.Pick()NoScalar.Pick()NoPoint.Set()YesScalar.Set()YesPoint.Clone()NoScalar.Clone()NoPoint.Add()YesScalar.Add()YesPoint.Sub()YesScalar.Sub()YesPoint.Neg()YesScalar.Neg()YesPoint.Mul()YesScalar.Mul()YesPoint.EmbedLen()NoScalar.Inv()YesPoint.Embed()NoScalar.Div()NoPoint.Data()NoScalar.SetInt64()NoPoint.MarshalBinary()YesScalar.MarshalBinary()YesPoint.UnmarshalBinary()YesScalar.UnmarshalBinary()YesPoint.String()NoScalar.String()NoPoint.MarshalSize()NoScalar.MarshalSize()NoPoint.MarshalTo()NoScalar.MarshalTo()NoPoint.UnmarshalFrom()NoScalar.UnmarshalFrom()NoScalar.SetBytes()YesScalar Addition97.9161Scalar Substraction90.0167Scalar Negation87.0172Scalar Multiplication173181Scalar Division6303550218Scalar Inversion5779046310Scalar Picking74038269Scalar Encoding40046.2Scalar Decoding7.1917.0Point Addition865395Point Substraction911402Point Negation63.042.5Point Multiplication28267272528Point Base Multiplication10399825268Point Picking31695389023Point Encoding125985171Point Decoding133575877I would like to get it reviewed by the engineers before adding it to the main branch.
Thanks,
Parinay