ascon-aead: zeroize buffer during decryption on failed tag check

[newpavlov]

No description provided.

[tarcieri]

I would suggest fixing it the same way as GHSA-423w-p2w9-r7vq by returning an unmodified buffer, as opposed to a zeroed one

[newpavlov]

IIUC the current implementation performs one-pass decryption. To return "unmodified buffer" we would need to encrypt the message back.

[tarcieri]

Yes, that was how it was solved in GHSA-423w-p2w9-r7vq as well

[newpavlov]

Zeroizing the buffer looks like a good, simple, and straightforward solution to me. Personally, I don't see the point in encrypting the data back, so I don't plan to work on it myself. I created the backport branch, so you could create an alternative PR.

[tarcieri]

If nothing else the solution we should choose should be consistent across crates.

You didn't make a similar argument with aes-gcm FWIW when we were choosing between zeroizing and re-encrypting (just a thumbs up).

I guess we could change both crates if you really want to use zeroing.

A zeroed message may be semantically meaningful in the context where an attacker is using it. If the risk is the caller ignoring the error somehow, I think a zeroed message has a higher chance of causing problems.

Keeping the original ciphertext is closer to a pseudorandom rejection symbol, which IMO is less useful to the attacker.

[tarcieri]

This could also use a regression test

[newpavlov]

In the aes-gcm case it did not matter to me, so I left it for you to decide (it was the intended meaning of the thumbs-up). As we discussed in other places, I am not sure about one-pass decryption in the first place. Ideally, we should not touch ciphertext until its integrity was verified. But I understand that from the performance perspective one-pass is important.

Encrypting the buffer back could also be weird together with the planned inout support.

If the risk is the caller ignoring the error somehow, I think a zeroed message has a higher chance of causing problems.

We can not protect users from all potential stupidity on the Earth.

This could also use a regression test

Yes, but I got stuck a bit with the "smart" spectral tests.

[tarcieri]

Ideally, we should not touch ciphertext until its integrity was verified. But I understand that from the performance perspective one-pass is important.

Returning the original ciphertext on verification failure is closer to this ideal than mutating it with zeros, even if it's being decrypted and re-encrypted behind the scenes.

[newpavlov]

What is done in other cryptographic libraries?

[tarcieri]

ring does not specify:

When open_in_place() returns Err(..), in_out may have been overwritten in an unspecified way.

The other crates in this repo beyond aes-gcm do not modify the buffer if the tag verification fails.

[newpavlov]

I meant other libraries outside of the Rust ecosystem.

[tarcieri]

I don't have time to do a thorough investigation. This is a security fix so I guess let's just get it out.

It seems easy enough to be have consistent behavior with the other crates I thought I'd point it out along with the past precedent and wasn't expecting this to become some kind of protracted debate, but in the interest of getting the fix out I'll give up for now and perhaps we can change it as a followup.