Probably https://golang.org/cl/10484 (efeeee3). /cc @vkrasnov

I suspect swapping the order of lines 158--162 and 164--169 of crypto/aes/aes_gcm.go would restore compatibility with Go 1.5. (Edit: No, I'm wrong; it's not that simple.)

Assuming I'm looking at the same code you're referring to, it looks like Open probably isn't modifying ciphertext per se, but that the caller is reusing the same memory as the dst argument and expecting Open to not modify dst if the MAC is invalid. Seemingly this is guaranteed by AEAD.Open's documentation:

    // Open decrypts and authenticates ciphertext, authenticates the
    // additional data and, if successful, appends the resulting plaintext
    // to dst, returning the updated slice. [...]
    //
    // The ciphertext and dst may alias exactly or not at all.

It looks like the new AES-GCM code is appending data to dst even when unsuccessful.

It was a design decision to not allow reusing the failed result or partial result, in case the MAC failed, we instead zero the output, to make the usage safer. The scenario where one might reuse ciphertext after failure (not on purpose), is exactly one we wanted to avoid.

That was my concern when I submited the patch. The code performs simultaneous decryption and authentication, and the alternative was to copy to a temp buffer.
I consulted @agl and he considered the zeroing to be better, and I agree.
The fact 1.5 did it, does not make it better. I don't think you should restore that behavior. It will be less safe for the intended use, and slower to boot.

The more serious issue I see, is that the non amd64 behavior is unchanged, and therefore the same version has different behavior for different architectures.

My 2c: If we're going to accept the new behavior on amd64, we should change the generic GCM code to also zero out dst on MAC failures.

Since the optimised code stitches together the decryption and authentication for performance, it needs to write the plaintext before it can check whether the authentication tag is correct. Thus, unlike the generic code, it cannot leave the destination buffer untouched in all failure cases.

Since it has written plaintext that it knows is dangerous, zeroing out is, I think, I fair behaviour.

However, it does still leave the destination buffer untouched in other failure cases so it's not the case that it'll always zero it out. BoringSSL does always zero it out but Go is different because it returns a slice and so I'm not immediately convinced that it needs to be the same.

Given that, I think I would like to keep the AESNI behaviour to save a copy, and thus update the generic code to act the same. I'll write some changes to do this.

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

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