x/crypto/chacha20poly1305: overlap of dst and additionalData should panic

[n2vi]

Go version

go1.25.2

Output of go env in your module/workspace:

specific version of Go or env is not relevant; comment has been unchanged for a long time.

What did you do?

I would like to be able to write:

aead, err := chacha20poly1305.NewX()
plaintext := []byte("hello, world")
dst := make([]byte, 0, 26+len(plaintext)+16)
dst[0] = 'a'
dst[1] = 'D'
rand.Read(dst[2:26])
dst = aead.Seal(dst, dst[2:26], plaintext, dst[0:2])

What did you see happen?

This does seem to work for the library AEAD versions.

What did you expect to see?

But the comment in type AEAD says dst and additionalData may not overlap.

Ok to change this to On input, dst[len(dst):cap(dst)] and additionalData may not overlap.?

[gabyhelp]

Related Issues

• crypto/cipher: update documentation for aead Open #69108 (closed)
• proposal: crypto/cipher: detached mode AEAD Seal/Open #24990 (closed)

Related Code Changes

• crypto/cipher: improve documentation for AEAD
• [dev.boringcrypto] crypto/aes: panic on invalid dst, src overlap

Related Discussions

• overlap of dst and additionalData in AEAD.Seal

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[seankhliao]

see https://go-review.googlesource.com/c/go/+/624736/comment/ab5fd1a3_06d70af4/

In general, restrictions like these cannot be relaxed as other implementations may depend on it, potentially clobbering data. See previously #21624

[seankhliao]

I think this is a missing overlap check for chacha20poly1305, gcm does enforce that the argument don't overlap.

[gopherbot]

Change https://go.dev/cl/712860 mentions this issue: chacha20poly1305: panic on dst and additionalData overlap

[glycerine]

I think this is just trying to clarify the language that seems contradictory, because its not clear that the last sentence only applies to the plaintext[:0] case -- it is not qualified, and therefore seems like a universal. Even though the paragraph is all about the using dst = plaintext[:0] case, @n2vi reads it as being universal, as his example shows.

On the one hand,

[Seal] appends the result to dst

on the other hand

dst and additionalData may not overlap.

If Seal is actually appending to data that is already there, why cannot the data contain the additionalData already? Its not going to get overwritten anyway, right, because it is going to append?

edit to suggest Pre-pending the [bracketed text] to the last sentence:

"[The remaining as yet unused capacity of] dst and additionalData may not overlap."

edit2: It might also be worth pointing out that doing what @n2vi is wanting to do is going to result in very poor performance, if it results in allocating every time you encrypt rather than reusing your plaintext buffer exactly -- this will balloon your memory use. But it seems well within the API spec, because append in Go is not destructive. I'm sympathetic to any clarifications, because this was a very difficult API to figure out how to use well when I first put it to use last year.

[n2vi]

@seankhliao:

In case it helps convey what I meant, I'll quote a bit more from my query to go-nuts:

I'm puzzled why crypto/cipher type AEAD has the comment
dst and additionalData may not overlap.
Just as it is idiomatic to provide a dst that already contains a nonce prefix,
I find it useful to include an additionalData prefix. This seems to work
and does not trigger a warning even after the fixes from issue #21624.
...
In the language of crypto internal function sliceForAppend, I understand
that ad may not overlap tail, but why not allow it to be part of head?
Is there some subtle timing side-channel that I'm overlooking?

So indeed I had already looked at #21624 and not found an answer. I have
no problem with CL#712860 panic on overlap of ad and out, and for sure
it is good to harmonize AES GCM and chacha20poly1305.

But my point in raising the issue was that it should be ok for ad to overlap ret.

@glycerine:
With regard to performance, it is hard to convey the entire context in a tiny
example. In practice, the way this gets used is to move the allocation of aead
and dst outside the loop and allocate dst as a maxPacketSize byte array
that gets overwritten for each packet. Plaintext never overlaps at all with dst.

[seankhliao]

The point of #21624 is that there may be correct implementations based on the existing guarantees that the slices do not overlap. relaxing the restriction would break those.

[n2vi]

Yes, I understood Russ' point that the input/output overlap problem is so severe that the library code should panic rather than only return an error. I thank him for his CL and I thank you for yours that copies this check to the newer library code.

But my intent in opening this issue was to raise a different, nearby, observation.

One purpose of the dst parameter in AEAD.Seal is to allow the caller to pre-allocate space for the ciphertext+tag. Another purpose, expressed in append to dst, is to allow that pre-allocated space to include at the beginning non-secret information such as the nonce that may be useful for Alice to send to Bob in the crypto protocol. Another such piece of information could be a message counter, which is also not secret but ought to be included in the authentication provided by Seal. This associatedData may be assembled directly into the pre-allocated space next to nonce and before the ciphertext. All I'm saying is that currently the wording of the comment seems to prohibit that, but the library properly allows it and there is sound cryptographic protocol practice for doing so.

@FiloSottile, perhaps you can weigh in here?

[FiloSottile]

Yeah, sorry, "dst and additionalData may not overlap" is sloppy wording on my part. What I meant is "the remaining capacity of dst may not overlap additionalData."

Technically, changing this would be a breaking change, but I can't imagine implementations actually requiring the incorrect wording, but they probably do require the corrected one.

[glycerine]

@FiloSottile @n2vi This seems to have been closed prematurely.

I don't know what that pull request is that referenced this is, but it has nothing to do with the issue of improving the documentation text -- it made no change to the documentation.

@seankhliao this is the third time I have observed very rash and inconsiderate repo management actions from you. Please show more restraint -- actually understand and clarify the issue at hand before rushing to an incorrect conclusion.

Edit: case in point, the change made in the merged PR looks problematic on two points:

a) it is in a performance sensitive hot code path. There are no benchmarks to show how much slowdown this extra check will cost;

b) it does not actually check the un-used capacacity overlap that is being discussed here; the code for AnyOverlap is

// AnyOverlap reports whether 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 {
    return len(x) > 0 && len(y) > 0 &&
        uintptr(unsafe.Pointer(&x[0])) <= uintptr(unsafe.Pointer(&y[len(y)-1])) &&
        uintptr(unsafe.Pointer(&y[0])) <= uintptr(unsafe.Pointer(&x[len(x)-1]))
}

which only checks within len and not over cap.