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/elliptic: possible bug with deriving a secret for ApplePay token #28723

Closed
importnil opened this issue Nov 11, 2018 · 2 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@importnil
Copy link

importnil commented Nov 11, 2018

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

go version go1.11.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/vadimyer/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/vadimyer/GoSpace"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11.2/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11.2/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/ty/jl_43d0n2vjckpwsct7k3g3m0000gn/T/go-build709033229=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I've got an AP (ApplePay) tokens decryption library (really like all the rest existing in terms of algorithms). It is based on this library: https://github.com/processout/applepay with a few refactors for my app.

The library almost successfully works on production. The problem is, not every token gets decrypted. Sometimes there are tokens which (as far as I've known) for which the secret gets broken. Why and how broken?

Well, every single token gets decrypted without any errors with this library: https://github.com/sidimansourjs/applepay-token, but this is Node and JS. The byte representation of a derived secret in Node (for "bad" tokens) differs from that one in Go by 1 single byte in the beginning, which is sometimes a zero (0), in cases where the Go implementation can't decrypt an AP token. So when Go application can't decode a token, it's always missing a zero in the beginning of a secret, which leads my thoughts there's something wrong with deriving a secret, so probably a bug in elliptic or similar.

Note, that it doesn't really happen with every token, only like 1/20 of all tokens I get, and those which decrypt successfully by default, do not have a leading zero byte for their derived secret in both implementations.

What did you expect to see?

The same derived secret as in Node JS case, with a fully decrypted plaintext as a result.

What did you see instead?

The secret without a leading zero byte, leading to a failed decrypt result.

@odeke-em
Copy link
Member

Thank you for filing this issue @vadimyer and welcome to the Go project!

Kindly paging some crypto experts @agl @FiloSottile @aead

@odeke-em odeke-em added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 11, 2018
@FiloSottile
Copy link
Contributor

Hi @vadimyer. That’s a common bug in Go applications due to the fact that the standard library returns a *big.Int, a type that has no fixed size. When it gets serialized to bytes, it will miss any expected leading zeroes, as it has no way of knowing how many.

Specifically, the bug is here

https://github.com/processout/applepay/blob/c63b59c3c1ec121ae3a177e45450128201262b51/token_encryption.go#L122

but I would recommend fixing it by returning a padded []byte from ecdheSharedSecret().

@golang golang locked and limited conversation to collaborators Nov 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants