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: panic on invalid aliasing in various APIs #21624

Closed
rsc opened this issue Aug 25, 2017 · 10 comments
Closed

crypto: panic on invalid aliasing in various APIs #21624

rsc opened this issue Aug 25, 2017 · 10 comments

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Aug 25, 2017

The cipher.AEAD docs say for Open:

// Open decrypts and authenticates ciphertext, authenticates the
// additional data and, if successful, appends the resulting plaintext
// to dst, returning the updated slice. The nonce must be NonceSize()
// bytes long and both it and the additional data must match the
// value passed to Seal.
//
// The ciphertext and dst may alias exactly or not at all. To reuse
// ciphertext's storage for the decrypted output, use ciphertext[:0] as dst.

I just spent a few hours debugging a problem that ended up being a call to cipher.AEAD.Open
with inexact aliasing that was accepted and implemented "correctly" by one implementation
of cipher.AEAD (the crypto/aes GCM one) but rejected (as permitted) by a different implementation.

The docs above give a constraint on the nonce size and on the aliasing.
The AES-GCM implementation panics if the nonce size is incorrect but
does nothing if the aliasing is incorrect. I suggest it panic too.

More generally, I suggest that everywhere we document a restriction
about the aliasing of input and output buffers we enforce that restriction
with a check and panic.

We can add to crypto/subtle

// PanicOnAlias panics if dst and src point at aliased (overlapping) memory.
func PanicOnAlias(dst, src []byte)

// PanicOnInexactAlias panics if dst and src point at aliased (overlapping) memory,
// except in the case where dst and src point to the same starting address.
func PanicOnInexactAlias(dst, src []byte)

and implement these in a way that inlines.

/cc @agl

@rsc rsc added this to the Go1.10 milestone Aug 25, 2017
@agl
Copy link
Contributor

@agl agl commented Aug 29, 2017

We assert this property in BoringSSL, I agree that it would be good to do so in Go too. Doing that in C requires undefined behaviour. Is the obvious arithmetic comparison with uintptrs guaranteed to work in Go? (Or, at least, is it guaranteed not to false positive? It's possible that I shied away from doing this initially because of worries like “what if a GC happens between these two lines and moves the buffer?”)

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Aug 31, 2017

By my reading of https://golang.org/pkg/unsafe/#Pointer simply using uintptr is not guaranteed to work. (In practice I suspect it will work since the GC never moves heap allocations, and stacks move only on scheduler calls?)

Implementing it in assembly would definitely work. I guess the generic code could be left to a no-op not to require implementations for all platforms right away. But yeah, no inlining.

@rsc
Copy link
Contributor Author

@rsc rsc commented Aug 31, 2017

Writing it in Go is fine. Just put me on the review list. It's the standard library - we can assume things not guaranteed by the spec, and it's on us to keep them working. Go is much better than assembly times N architectures.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 14, 2017

Change https://golang.org/cl/63915 mentions this issue: [dev.boringcrypto] crypto/aes: panic on invalid dst, src overlap

gopherbot pushed a commit that referenced this issue Sep 18, 2017
I've now debugged multiple mysterious "inability to communicate"
bugs that manifest as a silent unexplained authentication failure but are
really crypto.AEAD.Open being invoked with badly aligned buffers.
In #21624 I suggested using a panic as the consequence of bad alignment,
so that this kind of failure is loud and clearly different from, say, a
corrupted or invalid message signature. Adding the panic here made
my failure very easy to track down, once I realized that was the problem.
I don't want to debug another one of these.

Also using this CL as an experiment to get data about the impact of
maybe applying this change more broadly in the master branch.

Change-Id: Id2e2d8e980439f8acacac985fc2674f7c96c5032
Reviewed-on: https://go-review.googlesource.com/63915
Reviewed-by: Adam Langley <agl@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 22, 2017

Change https://golang.org/cl/65486 mentions this issue: [dev.boringcrypto.go1.8] crypto/aes: panic on invalid dst, src overlap

gopherbot pushed a commit that referenced this issue Sep 22, 2017
I've now debugged multiple mysterious "inability to communicate"
bugs that manifest as a silent unexplained authentication failure but are
really crypto.AEAD.Open being invoked with badly aligned buffers.
In #21624 I suggested using a panic as the consequence of bad alignment,
so that this kind of failure is loud and clearly different from, say, a
corrupted or invalid message signature. Adding the panic here made
my failure very easy to track down, once I realized that was the problem.
I don't want to debug another one of these.

Also using this CL as an experiment to get data about the impact of
maybe applying this change more broadly in the master branch.

Change-Id: Id2e2d8e980439f8acacac985fc2674f7c96c5032
Reviewed-on: https://go-review.googlesource.com/63915
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-on: https://go-review.googlesource.com/65486
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@rsc
Copy link
Contributor Author

@rsc rsc commented Nov 22, 2017

I have pending CLs for this but they did not go out in time. Next cycle.

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@FiloSottile FiloSottile self-assigned this Apr 25, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 26, 2018

Change https://golang.org/cl/109697 mentions this issue: crypto/subtle: add AnyOverlap/InexactOverlap and apply them across crypto packages

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Apr 27, 2018

Here's my proposal for crypto/subtle, based on @rsc's CL.

// AnyOverlap returns true if x and y share memory at any (not necessarily
// corresponding) index. The memory beyond the slice length is ignored.
func AnyOverlap(x, y []byte) bool

// InexactOverlap returns true if x and y share memory at any non-corresponding
// index. The memory beyond the slice length is ignored.
//
// Note that x and y can have different lengths and still return false.
//
// InexactOverlap can be used to implement the requirements of the cypto/cipher
// AEAD, Block, BlockMode and Stream interfaces.
func InexactOverlap(x, y []byte) bool

Making it return a bool instead of panic'ing helps with inlining and lets us write better panic messages.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented May 4, 2018

Based on discussion with @agl, @bradfitz and @ianlancetaylor:

  • we will expose this in x/crypto/subtle
  • we will vendor it into the standard library
  • we will check aliasing in all the crypto APIs that require it
@gopherbot
Copy link

@gopherbot gopherbot commented May 9, 2018

Change https://golang.org/cl/112236 mentions this issue: subtle: add Any/InexactOverlap (new package) and apply them across packages

gopherbot pushed a commit to golang/crypto that referenced this issue Jun 13, 2018
…across packages

AnyOverlap and InexactOverlap implement checks for the aliasing
requirements defined by the crypto/cipher interfaces. Apply them to all
implementations as the actual requirement could be architecture-dependent
and user code should not rely on undefined behavior.

Updates golang/go#21624

Change-Id: I465de02fb3fec4e0c6f1fdee1ef6ae7ed5abff10
Reviewed-on: https://go-review.googlesource.com/112236
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot gopherbot closed this in 75d15a2 Jun 19, 2018
@golang golang locked and limited conversation to collaborators Jun 19, 2019
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
4 participants
You can’t perform that action at this time.