-
Notifications
You must be signed in to change notification settings - Fork 167
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
Constant-time scalars in ed25519, and variable-time option flag #173
Changes from 26 commits
9f4aa73
bb3867e
bce2b89
6eaf519
a1133fc
51b50f6
6bb64f7
bad1135
31a5cbb
c6cdb80
9e83790
7d5f0fd
a76faaf
3c81136
f36ae2e
0e78656
59a897f
2d953ff
6b77576
b3da6d2
294eef9
f5608af
f5f7ebb
e72ba4d
20e3545
3baf54d
1946b38
0bcf70c
0d09f3c
28a0871
bc5f664
ebd2462
644dc22
7b0afeb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
// This package exists runs comparative benchmarks | ||
// Package bench runs comparative benchmarks | ||
// across several alternative Cipher implementations. | ||
package bench |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ import ( | |
"fmt" | ||
|
||
"gopkg.in/dedis/kyber.v1" | ||
"gopkg.in/dedis/kyber.v1/group/nist" | ||
"gopkg.in/dedis/kyber.v1/group/edwards25519" | ||
) | ||
|
||
type Suite interface { | ||
|
@@ -54,7 +54,7 @@ func SchnorrSign(suite Suite, random cipher.Stream, message []byte, | |
// And check that hashElgamal for T and the message == c | ||
buf := bytes.Buffer{} | ||
sig := basicSig{c, r} | ||
suite.Write(&buf, &sig) | ||
_ = suite.Write(&buf, &sig) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again error-checking: either remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Golint. |
||
return buf.Bytes() | ||
} | ||
|
||
|
@@ -87,9 +87,9 @@ func SchnorrVerify(suite Suite, message []byte, publicKey kyber.Point, | |
} | ||
|
||
// Example of using Schnorr | ||
func ExampleSchnorr() { | ||
func Example_schnorr() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please follow https://golang.org/pkg/testing/#pkg-examples There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Golint do not pass with that so I choose golint. |
||
// Crypto setup | ||
group := nist.NewAES128SHA256P256() | ||
group := edwards25519.NewAES128SHA256Ed25519(false) | ||
rand := group.Cipher([]byte("example")) | ||
|
||
// Create a public/private keypair (X,x) | ||
|
@@ -110,9 +110,9 @@ func ExampleSchnorr() { | |
|
||
// Output: | ||
// Signature: | ||
// 00000000 c1 7a 91 74 06 48 5d 53 d4 92 27 71 58 07 eb d5 |.z.t.H]S..'qX...| | ||
// 00000010 75 a5 89 92 78 67 fc b1 eb 36 55 63 d1 32 12 20 |u...xg...6Uc.2. | | ||
// 00000020 2c 78 84 81 04 0d 2a a8 fa 80 d0 e8 c3 14 65 e3 |,x....*.......e.| | ||
// 00000030 7f f2 7c 55 c5 d2 c6 70 51 89 40 cd 63 50 bf c6 |..|U...[email protected]..| | ||
// 00000000 d4 64 bd ac 8a 06 d9 71 f4 ae a1 da e1 c5 55 d5 |.d.....q......U.| | ||
// 00000010 f7 89 50 10 a5 d9 99 52 b0 c4 f2 ba f9 37 67 02 |..P....R.....7g.| | ||
// 00000020 35 3e 9b ac e6 dd d1 98 f6 19 88 37 4d e3 4f 5c |5>.........7M.O\| | ||
// 00000030 36 de a7 bf b9 f0 06 2b 72 6f 81 b7 59 19 c6 00 |6......+ro..Y...| | ||
// Signature verified against correct message. | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,11 +53,18 @@ type Scalar interface { | |
// Set to a fresh random or pseudo-random scalar | ||
Pick(rand cipher.Stream) Scalar | ||
|
||
// SetBytes will take bytes and create a scalar out of it | ||
// SetBytes sets the scalar from a big-endian byte-slice, | ||
// reducing if necessary to the appropriate modulus. | ||
SetBytes([]byte) Scalar | ||
|
||
// Bytes returns the raw internal representation | ||
// Bytes returns a big-Endian representation of the scalar | ||
Bytes() []byte | ||
|
||
// Allow or disallow use of faster variable-time implementations | ||
// of operations on this Point, returning the old flag value. | ||
// This flag always defaults to false (constant-time only) | ||
// in implementations that can provide constant-time operations. | ||
SetVarTime(varTime bool) bool | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just realized that we are not providing the variable time implementation anymore for What about if we change the signature to type Scalar interface {
...
// VarTime returns a Scalar using variable time arithmetic operations if varTime is true.
// It returns a constant time implementation of a Scalar if varTime is false.
VarTime(varTime bool) Scalar // plus the old value if needed ?
} so in our implementation we can have a clear distinction between the two implementations type scalarVarTime mod.Int
type scalarConstTime struct {
v [32]byte
} I see at least two main advantages here:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought of this approach, and I'm open to it but still on the fence about it. The main downsides I see are:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps it would be worth taking a bit of a "census" throughout the crypto and/or onet code to see where all we do Scalar arithmetic and how much of that arithmetic might conceivably benefit from being able to use a faster variable-time version for non-sensitive calculations? If it turns out there's not all that much, and/or it proves hard to identify and disentangle which are the "sensitive" vs "non-sensitive" scalars, then I would propose in this case it's probably best (certainly safest) just to make the scalars always use constant-time arithmetic and take whatever performance hit that means. Given that point.Mul is the main performance-critical operation we're concerned about in general, perhaps keeping that as the only one we bother with having constant-time and variable-time versions of would be perfectly fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, it's a valid counter argument indeed. I am totally in favor of dropping variable time implementation for From my experience with the kyber library and the algorithms I coded, I don't think it should be that hard to distinguish between sensitive or non sensitive scalars if one really takes the time at each individual operations. Nevertheless, my main concern is that screw-ups can and will happen (Murphy's law!). Removing vartime implementation will at least prevent a large range of potential weaknesses in existant and future implementations: that's a huge benefit. While this library must not be slow, its aim is not to be the fastest also, it's to be a generic low-level and secure cryptographic library, so the performance cost is totally bearable in my opinion in constrast to the benefits. |
||
} | ||
|
||
/* | ||
|
@@ -111,13 +118,19 @@ type Point interface { | |
// Set to the negation of point a | ||
Neg(a Point) Point | ||
|
||
// Encrypt point p by multiplying with scalar s. | ||
// If p == nil, encrypt the standard base point Base(). | ||
// Multiply point p by the scalar s. | ||
// If p == nil, multiply with the standard base point Base(). | ||
Mul(s Scalar, p Point) Point | ||
|
||
// Allow or disallow use of faster variable-time implementations | ||
// of operations on this Point. Returns the old flag value. | ||
// This flag always defaults to false (constant-time only) | ||
// in implementations that can provide constant-time operations. | ||
SetVarTime(varTime bool) bool | ||
} | ||
|
||
/* | ||
This interface represents an kyber.cryptographic group | ||
Group interface represents an kyber.cryptographic group | ||
usable for Diffie-Hellman key exchange, ElGamal encryption, | ||
and the related body of public-key cryptographic algorithms | ||
and zero-knowledge proof methods. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in fact an error-check here would be very nice. But as the main method does not return any error, I'm not sure what is best to do here.
So either remove the
_, _ =
or do some error-checking.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the
XXX
can go away.