crypto: panic on invalid aliasing in various APIs

[rsc]

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

[agl]

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]

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]

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]

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

[gopherbot]

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

[rsc]

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

[gopherbot]

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

[FiloSottile]

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]

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]

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