Skip to content
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/tls: Unnecessary check whether a point is on a curve #20496

Closed
aead opened this issue May 25, 2017 · 1 comment
Closed

crypto/tls: Unnecessary check whether a point is on a curve #20496

aead opened this issue May 25, 2017 · 1 comment

Comments

@aead
Copy link
Contributor

@aead aead commented May 25, 2017

What version of Go are you using (go version)?

go version go1.8.1 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/andreas/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build282450971=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

I looked at processServerKeyExchange and processClientKeyExchange while investigating #20482.

It's actually not necessary to check explicitly whether the (x,y) coordinate is on the curve because the Unmarshal function does this anyway and the returned error (on failure) is the same.

Of course it's valid to check this again and maybe it's worth to do so for explicitness.
On the other side omitting the (unnecessary) IsOnCurve check would speed-up client/server key-exchange processing. A comment could make it clear that Unmarshal checks the point anyway - maybe @agl should decide about this?

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 26, 2017

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

@gopherbot gopherbot closed this in d38d357 Aug 15, 2017
@golang golang locked and limited conversation to collaborators Aug 15, 2018
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
The processClientKeyExchange and processServerKeyExchange functions unmarshal an
encoded EC point and explicitly check whether the point is on the curve. The explicit
check can be omitted because elliptic.Unmarshal fails if the point is not on the curve
and the returned error would always be the same.

Fixes golang#20496

Change-Id: I5231a655eace79acee2737dd036a0c255ed42dbb
Reviewed-on: https://go-review.googlesource.com/44311
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Avelino <t@avelino.xxx>
Run-TryBot: Adam Langley <agl@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
The processClientKeyExchange and processServerKeyExchange functions unmarshal an
encoded EC point and explicitly check whether the point is on the curve. The explicit
check can be omitted because elliptic.Unmarshal fails if the point is not on the curve
and the returned error would always be the same.

Fixes golang#20496

Change-Id: I5231a655eace79acee2737dd036a0c255ed42dbb
Reviewed-on: https://go-review.googlesource.com/44311
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Avelino <t@avelino.xxx>
Run-TryBot: Adam Langley <agl@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.