code review 10551044: crypto/elliptic: add constant-time, P-256 implementation.

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.

[agl1, 2013-06-25 18:41:58]

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/

[rsc, 2013-06-25 21:06:09]

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/ell...
File src/pkg/crypto/elliptic/elliptic_test.go (right):

https://codereview.appspot.com/10551044/diff/4002/src/pkg/crypto/elliptic/ell...
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/ell...
src/pkg/crypto/elliptic/elliptic_test.go:418: params := p256.Params()
Does this compile? I don't see where params is used.

[agl1, 2013-06-27 17:31:26]

*** 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/ell...
File src/pkg/crypto/elliptic/elliptic_test.go (right):

https://codereview.appspot.com/10551044/diff/4002/src/pkg/crypto/elliptic/ell...
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/ell...
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.