Fix input wiping on decryption failure

[SalusaSecondus]

PR #298 appears to have introduced a bug wherein when there is decryption failure can cause the input buffer to be wiped. Specifically, this happens for AES-GCM in-place decryption. (Ideally, the output buffer would remain unchanged in all failure cases, but that change is more understandable.)

This change:

• Returns to using a staging buffer to hold the unauthenticated plaintext
• Uses the pre-existing input buffer to avoid memory allocation. (This wasn’t possible prior to PR # 298 because the input buffer wasn’t always used.)
• Copies the plaintext over to the output buffer only upon successful decryption
• Introduces a unit tests to avoid regression

So, this new change does incur a new memory copy relative to the current situation (but the same as before PR # 298) but does not incur any new memory allocation (so we keep that win from # 298).

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[SalusaSecondus]

Closing and re-opening to poke the checks.

[geedo0]

Nice catch! I don't mind the additional I/O here. I do worry about the precedent set by taking this stance of considering what happens to the output buffer (and what impact that has if it also happens to be the input) on failure cases. This can get into really murky territory. The OpenSSL docs state that the contents of the output are "corrupt" and don't consider any of these factors. That flexibility is pretty useful in these circumstances.

[geedo0]

LGTM

[amirhosv]

Maybe we can avoid this copy in case the output buffer doesn't overlap input. Still in the case of failure the output buffer would potentially be modified, but it's output buffer after all.

[amirhosv]

In cases where the output buffer is distinct from the input, like here the copying back can be avoided by directly using the output buffer.

[SalusaSecondus]

I am willing to change this, however:

1. The current design never leaks unauthenticated plaintext outside the ACCP boundary.
2. The proposed change is more complicated and we're already seeing bugs creep in which suggest that we're nearing maximum complexity to safely work here.

So, if left to me, I would leave this as written, but I am willing to change it. (Or, to read it another way. I have one approval and one request for changes. Mind reaching consensus?)

[geedo0]

We spoke offline and I'm more hesitant now. My interpretation of the contract is that if the caller gives us an output buffer and a length we have no obligation to preserve the contents of that buffer even if the tag check fails. If we can preserve it for free, awesome, but if it adds O(n) memory I/O to all AES-GCM operations I'm less inclined. Even if we add the risky logic for overlap detection, what we'll find is that the perfect overlap case is the common case and it won't buy us much.

The remaining point of clarity for us is to look at how other libraries handle this case to determine if this is considered a bug as that's still a bit subjective to me. AFAICT, OpenSSL/AWS-LC will scribble over the output buffer and simply return an error code if the tag fails. I'd be hesitant to add new semantics at the ACCP layer which don't line up with the native library.

[SalusaSecondus]

My concern is us overwriting the input buffer (which happens to be the same).

I have personally seen decryption logic which attempts repeated in-place decryption using multiple keys. Unfortunately, I no longer have access to said code. However, wiping the in-place buffer on failure would absolutely break that code.

More interesting than how OpenSSL/AWS-LC behave is how other JCA providers behave as that's what ACCP is replacing.

[geedo0]

Alright, after many months, we discussed this as a team and decided to close this PR. Absent any specification that mandates a certain behavior we decided to keep the implementation choice which improves performance. This change in behavior has been deployed for several months and based on the evidence so far we are willing to tolerate the amount of breakage this may invite.