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

Filippo version of Ed25519 #462

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Filippo version of Ed25519 #462

wants to merge 20 commits into from

Conversation

parinayc20
Copy link
Contributor

@parinayc20 parinayc20 commented Jun 9, 2022

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 Functions Present in Filippo Scalar Functions Present in Filippo
Point.Equal() Yes Scalar.Equal() Yes
Point.Null() Yes Scalar.Zero() Yes
Point.Base() Yes Scalar.One() No
Point.Pick() No Scalar.Pick() No
Point.Set() Yes Scalar.Set() Yes
Point.Clone() No Scalar.Clone() No
Point.Add() Yes Scalar.Add() Yes
Point.Sub() Yes Scalar.Sub() Yes
Point.Neg() Yes Scalar.Neg() Yes
Point.Mul() Yes Scalar.Mul() Yes
Point.EmbedLen() No Scalar.Inv() Yes
Point.Embed() No Scalar.Div() No
Point.Data() No Scalar.SetInt64() No
Point.MarshalBinary() Yes Scalar.MarshalBinary() Yes
Point.UnmarshalBinary() Yes Scalar.UnmarshalBinary() Yes
Point.String() No Scalar.String() No
Point.MarshalSize() No Scalar.MarshalSize() No
Point.MarshalTo() No Scalar.MarshalTo() No
Point.UnmarshalFrom() No Scalar.UnmarshalFrom() No
Scalar.SetBytes() Yes

The purpose for changing to the filippo version was the more optimized approach used in Filippo which brings down the time limit by a large factor for time intensive operations which is indicated by the performance table given below:

Function Kyber Time (ns/operation) Filippo Time (ns/operation)
Scalar Addition 97.9 161
Scalar Substraction 90.0 167
Scalar Negation 87.0 172
Scalar Multiplication 173 181
Scalar Division 63035 50218
Scalar Inversion 57790 46310
Scalar Picking 7403 8269
Scalar Encoding 400 46.2
Scalar Decoding 7.19 17.0
Point Addition 865 395
Point Substraction 911 402
Point Negation 63.0 42.5
Point Multiplication 282672 72528
Point Base Multiplication 103998 25268
Point Picking 316953 89023
Point Encoding 12598 5171
Point Decoding 13357 5877

I would like to get it reviewed by the engineers before adding it to the main branch.

Thanks,
Parinay

parinayc20 and others added 15 commits July 7, 2022 18:20
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.
Copy link
Contributor

@pierluca pierluca left a comment

Choose a reason for hiding this comment

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

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 :)

.gitignore Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
group/edwards25519/curve_test.go Outdated Show resolved Hide resolved
group/filippo_ed25519/const.go Outdated Show resolved Hide resolved
}

// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I am correct then zero would also be a valid output for what we are trying to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

@si-co do you agree ?

Copy link

@si-co si-co Mar 13, 2023

Choose a reason for hiding this comment

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

This function seems to have different problems:

  • It is not documented.
  • I don't understand why it uses sha512 when 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 returns digest[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 Show resolved Hide resolved
if data == nil {
p.Mul(filippoCofactorScalar, p)
if p.Equal(&filippoNullPoint) {
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

group/filippo_ed25519/point.go Outdated Show resolved Hide resolved
group/filippo_ed25519/point.go Outdated Show resolved Hide resolved
group/filippo_ed25519/scalar.go Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Jul 21, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

82.8% 82.8% Coverage
0.0% 0.0% Duplication

@pierluca pierluca requested review from pierluca and si-co March 9, 2023 08:31
Copy link
Contributor

@pierluca pierluca left a comment

Choose a reason for hiding this comment

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

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 ?

Comment on lines +119 to +123
// 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:]
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

commented out code

}

// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

@si-co do you agree ?

}

func (s *Scalar) String() string {
b, _ := s.MarshalBinary()
Copy link
Contributor

Choose a reason for hiding this comment

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

here we're not processing the error return ...

if s.scalar == nil {
s.scalar = new(filippo_ed25519.Scalar)
}
_, err := s.scalar.SetCanonicalBytes(b[:])
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a legitimate use of _, as s.scalar.Set just returns s.scalar . A comment would clarify it though.

@K1li4nL K1li4nL self-assigned this May 22, 2024
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.

7 participants