Code review - Issue 10551044: code review 10551044: crypto/elliptic: add constant-time, P-256 implementation.https://codereview.appspot.com/2013-06-27T17:31:26+00:00rietveld
Message from unknown
2013-06-25T18:23:48+00:00agl1urn:md5:7d564bd9f66b5534eabf4bc3fb47a1b7
Message from unknown
2013-06-25T18:23:58+00:00agl1urn:md5:b797f543adad61b7ed9a76229bf90ab9
Message from unknown
2013-06-25T18:39:43+00:00agl1urn:md5:f49eaac6f99bf176e092da77772ceb77
Message from unknown
2013-06-25T18:41:47+00:00agl1urn:md5:216d067554dde12f0a799ac823ef5d53
Message from agl@golang.org
2013-06-25T18:41:58+00:00agl1urn:md5:468ef400ceb3776a6b1e7ef5c98fe482
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
I'd like you to review this change to
https://code.google.com/p/go/
Message from rsc@golang.org
2013-06-25T21:06:09+00:00rscurn:md5:d774adc48e7bcac387f7aef2fe693ef6
LGTM
The tests look good. I trust you on the code given that the tests work.
https://codereview.appspot.com/10551044/diff/4002/src/pkg/crypto/elliptic/elliptic_test.go
File src/pkg/crypto/elliptic/elliptic_test.go (right):
https://codereview.appspot.com/10551044/diff/4002/src/pkg/crypto/elliptic/elliptic_test.go#newcode329
src/pkg/crypto/elliptic/elliptic_test.go:329: for i, e := range p224BaseMultTests {
Is there any worry that the numbers you're testing with, being 224-bit numbers, won't provoke important cases in the implementation? Is it worth adding some 256-bit tests?
https://codereview.appspot.com/10551044/diff/4002/src/pkg/crypto/elliptic/elliptic_test.go#newcode418
src/pkg/crypto/elliptic/elliptic_test.go:418: params := p256.Params()
Does this compile? I don't see where params is used.
Message from unknown
2013-06-27T17:31:02+00:00agl1urn:md5:e18ee9deaea3a562aa9991fbf4138fdf
Message from agl@golang.org
2013-06-27T17:31:26+00:00agl1urn:md5:281cfbf7342a638359b72013f3a4feaf
*** Submitted as https://code.google.com/p/go/source/detail?r=3a9551481ad1 ***
crypto/elliptic: add constant-time, P-256 implementation.
On my 64-bit machine, despite being 32-bit code, fixed-base
multiplications are 7.1x faster and arbitary multiplications are 2.6x
faster.
It is difficult to review this change. However, the code is essentially
the same as code that has been open-sourced in Chromium. There it has
been successfully performing P-256 operations for several months on
many machines so the arithmetic of the code should be sound.
R=golang-dev, rsc
CC=golang-dev
https://codereview.appspot.com/10551044
https://codereview.appspot.com/10551044/diff/4002/src/pkg/crypto/elliptic/elliptic_test.go
File src/pkg/crypto/elliptic/elliptic_test.go (right):
https://codereview.appspot.com/10551044/diff/4002/src/pkg/crypto/elliptic/elliptic_test.go#newcode329
src/pkg/crypto/elliptic/elliptic_test.go:329: for i, e := range p224BaseMultTests {
On 2013/06/25 21:06:09, rsc wrote:
> Is there any worry that the numbers you're testing with, being 224-bit numbers,
> won't provoke important cases in the implementation? Is it worth adding some
> 256-bit tests?
That's a fair point. I'll use the p224 values plus a huge value for P-256 to check that reduction in the scalar group is working.
https://codereview.appspot.com/10551044/diff/4002/src/pkg/crypto/elliptic/elliptic_test.go#newcode418
src/pkg/crypto/elliptic/elliptic_test.go:418: params := p256.Params()
On 2013/06/25 21:06:09, rsc wrote:
> Does this compile? I don't see where params is used.
Sorry, that was me failing to hg upload after some quick benchmarking. Fixed.