Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(8)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 4 months ago by agl1
Modified:
4 years, 4 months ago
Reviewers:
rsc
CC:
golang-dev, rsc
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : diff -r 13e16bc02f6d https://code.google.com/p/go/ #

Patch Set 3 : diff -r 13e16bc02f6d https://code.google.com/p/go/ #

Patch Set 4 : diff -r 13e16bc02f6d https://code.google.com/p/go/ #

Total comments: 4

Patch Set 5 : diff -r 4fe0404effd1 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1243 lines, -12 lines) Patch
M src/pkg/crypto/elliptic/elliptic.go View 1 2 chunks +0 lines, -12 lines 0 comments Download
M src/pkg/crypto/elliptic/elliptic_test.go View 1 2 3 4 2 chunks +57 lines, -0 lines 0 comments Download
A src/pkg/crypto/elliptic/p256.go View 1 2 3 4 1 chunk +1186 lines, -0 lines 0 comments Download

Messages

Total messages: 3
agl1
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/
4 years, 4 months ago (2013-06-25 18:41:58 UTC) #1
rsc
LGTM The tests look good. I trust you on the code given that the tests ...
4 years, 4 months ago (2013-06-25 21:06:09 UTC) #2
agl1
4 years, 4 months ago (2013-06-27 17:31:26 UTC) #3
*** 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld c59f3c4