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.

Sign up for GitHub

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

Jump to bottom

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
Labels
FrozenDueToAge Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone
Go1.11

Comments

@rsc
Copy link
Contributor

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 commented Aug 29, 2017 •
edited
Loading

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 FiloSottile mentioned this issue Aug 29, 2017
Closed
@FiloSottile
Copy link
Contributor

FiloSottile commented Aug 31, 2017 •
edited
Loading

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 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
Contributor

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
@rsc
[dev.boringcrypto] crypto/aes: panic on invalid dst, src overlap
89ba9e3 
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
Contributor

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
@rsc
[dev.boringcrypto.go1.8] crypto/aes: panic on invalid dst, src overlap
52f2e65 
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 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 added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 25, 2018
@FiloSottile FiloSottile self-assigned this Apr 25, 2018
@gopherbot
Copy link
Contributor

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

@FiloSottile
Copy link
Contributor

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 FiloSottile added Proposal Proposal-Crypto Proposal related to crypto packages or other security issues and removed NeedsFix The path to resolution is known, but the work has not been done. labels Apr 27, 2018
@FiloSottile
Copy link
Contributor

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
Contributor

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
@FiloSottile
internal/subtle: add Any/InexactOverlap (new package) and apply them …
37a17fe 
…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>
@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.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
4 participants
@agl @rsc @FiloSottile @gopherbot