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

crypto/elliptic: carry bug in x86-64 P-256 #20040

Closed
agl opened this Issue Apr 19, 2017 · 12 comments

Comments

Projects
None yet
9 participants
@agl
Contributor

agl commented Apr 19, 2017

Cloudflare reported a carry bug in the P-256 implementation that they submitted for x86-64 in 7bacfc6. I can reproduce this via random testing against BoringSSL and, after applying the patch that they provided, can no longer do so, even after ~231 iterations.

This issue is not obviously exploitable, although we cannot rule out the possibility of someone managing to squeeze something through this hole. (It would be a cool paper.) Thus this should be treated as something to fix, but not something on fire, based on what we currently know.

Fix will be coming in just a second.

@agl agl self-assigned this Apr 19, 2017

@ianlancetaylor ianlancetaylor added this to the Go1.8.2 milestone Apr 19, 2017

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Apr 19, 2017

Contributor

@agl This sounds like something we should fix in 1.8.2 and 1.9, but it is not necessary to release a new version of 1.6 or 1.7 with a fix. Does that sound right to you?

Contributor

ianlancetaylor commented Apr 19, 2017

@agl This sounds like something we should fix in 1.8.2 and 1.9, but it is not necessary to release a new version of 1.6 or 1.7 with a fix. Does that sound right to you?

@agl

This comment has been minimized.

Show comment
Hide comment
@agl

agl Apr 19, 2017

Contributor

I'm not very familiar with the convention for what gets backported and how far, but I agree that this is suitable for 1.8.2, certainly should be in 1.9 and it seems reasonable that it's not so important to warrant a respin for older versions, yes.

Contributor

agl commented Apr 19, 2017

I'm not very familiar with the convention for what gets backported and how far, but I agree that this is suitable for 1.8.2, certainly should be in 1.9 and it seems reasonable that it's not so important to warrant a respin for older versions, yes.

@gopherbot gopherbot closed this in 9294fa2 Apr 19, 2017

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Apr 19, 2017

Contributor

Reopening for backport.

Contributor

ianlancetaylor commented Apr 19, 2017

Reopening for backport.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented Apr 21, 2017

CL https://golang.org/cl/41070 mentions this issue.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented May 19, 2017

CL https://golang.org/cl/43770 mentions this issue.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented May 19, 2017

CL https://golang.org/cl/43773 mentions this issue.

gopherbot pushed a commit that referenced this issue May 23, 2017

[release-branch.go1.7] crypto/elliptic: fix carry bug in x86-64 P-256…
… implementation.

Patch from Vlad Krasnov and confirmed to be under CLA.

Fixes #20040.

Change-Id: Ieb8436c4dcb6669a1620f1e0d257efd047b1b87c
Reviewed-on: https://go-review.googlesource.com/41070
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 9294fa2)
Reviewed-on: https://go-review.googlesource.com/43773
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>

gopherbot pushed a commit that referenced this issue May 23, 2017

[release-branch.go1.8] crypto/elliptic: fix carry bug in x86-64 P-256…
… implementation.

Patch from Vlad Krasnov and confirmed to be under CLA.

Fixes #20040.

Change-Id: Ieb8436c4dcb6669a1620f1e0d257efd047b1b87c
Reviewed-on: https://go-review.googlesource.com/41070
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 9294fa2)
Reviewed-on: https://go-review.googlesource.com/43770
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
@agl

This comment has been minimized.

Show comment
Hide comment
@agl

agl May 23, 2017

Contributor

(This issue is CVE-2017-8932.)

Contributor

agl commented May 23, 2017

(This issue is CVE-2017-8932.)

@pierrre

This comment has been minimized.

Show comment
Hide comment
@pierrre

pierrre May 23, 2017

I have a suggestion about the release note.
Maybe we should include a notice that strongly encourage to upgrade especially if net/http (or other package that require crypto/elliptic) is imported.

As an app developer, I don't use crypto/elliptic in my code.
However it's a package imported by package I use.

pierrre commented May 23, 2017

I have a suggestion about the release note.
Maybe we should include a notice that strongly encourage to upgrade especially if net/http (or other package that require crypto/elliptic) is imported.

As an app developer, I don't use crypto/elliptic in my code.
However it's a package imported by package I use.

@FiloSottile

This comment has been minimized.

Show comment
Hide comment
@FiloSottile

FiloSottile May 23, 2017

Member
Member

FiloSottile commented May 23, 2017

@broady

This comment has been minimized.

Show comment
Hide comment
@broady

broady May 24, 2017

Member

Announcement was sent before I read your comment, @FiloSottile, but yes... for those following along, agl's statement in the first post is still true for TLS (as used by net/http).

If you're using the elliptic package directly, such as working with JWTs, then you probably want to update. If you're not, then wait for Go 1.8.3, which should be released tomorrow.

Member

broady commented May 24, 2017

Announcement was sent before I read your comment, @FiloSottile, but yes... for those following along, agl's statement in the first post is still true for TLS (as used by net/http).

If you're using the elliptic package directly, such as working with JWTs, then you probably want to update. If you're not, then wait for Go 1.8.3, which should be released tomorrow.

@broady broady closed this May 24, 2017

@cryptohazard

This comment has been minimized.

Show comment
Hide comment
@cryptohazard

cryptohazard May 24, 2017

Can someone please summarize what the issue actually means?
What are the odds of the problem appearing on an unpatched version?
How come it slipped through the tests?

cryptohazard commented May 24, 2017

Can someone please summarize what the issue actually means?
What are the odds of the problem appearing on an unpatched version?
How come it slipped through the tests?

@nielsole

This comment has been minimized.

Show comment
Hide comment

nielsole commented Dec 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment