Repository navigation
srtp: pass the output space to the cipher on AEAD unprotect - #831
Open
alepacheco wants to merge 2 commits into
Open
alepacheco wants to merge 2 commits into
alepacheco wants to merge 2 commits into
Conversation
srtp_unprotect_aead() and srtp_unprotect_rtcp_aead() passed the length of the encrypted portion, ciphertext plus GCM tag, as the output length to srtp_cipher_decrypt(). With a separate output buffer the caller only has to provide room for the plaintext, which is what the length checks above enforce, so the cipher was told the buffer had a tag's length more space than it has. Pass the space the caller provided after the header instead, as the protect functions do. The mbedtls backend hands that length to psa_aead_decrypt(), which can write to all of the output buffer it is given and clears all of it when the tag does not match, so unprotecting into a buffer sized for the plaintext wrote past its end. Give psa_aead_decrypt() only the plaintext length, as the encrypt function already does. Add tests that unprotect SRTP and SRTCP packets into a separate buffer, given first exactly the plaintext length and then the whole buffer, and check that nothing is written past the plaintext.
psa_cipher_update() can write to all of the output buffer it is given, not only the encrypted data, so the AES-ICM encrypt function wrote past the data it returned when given a larger output buffer. Give it only the input length, as the AES-GCM functions do, and reword the AES-GCM encrypt comment to match. Check in cipher_driver that encrypt and decrypt into a larger output buffer write nothing past the data they return.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
srtp_unprotect()andsrtp_unprotect_rtcp()with a separate output buffer only need room for the decrypted packet, and their length checks enforce exactly that. The AEAD paths then passed the ciphertext-plus-tag length tosrtp_cipher_decrypt()as the output length, so the cipher was told the output buffer had a tag's length more space than the caller provided.With the mbedtls backend that length reaches
psa_aead_decrypt()as the plaintext size. In the default PSA configuration (mbedtls 3.6 and 4.0) it copies back the whole output size it is given, and it clears all of it when the tag does not match, so unprotecting into a buffer sized for the decrypted packet writes past its end. The OpenSSL, NSS and wolfSSL backends write only the plaintext.This is the same out-of-place family as #818 and #819.
Changes:
srtp_unprotect_aead()andsrtp_unprotect_rtcp_aead()pass the space the caller provided after the header, as the protect functions do.psa_aead_decrypt()only the plaintext length, as its encrypt already does. The AES-ICM encrypt likewise givespsa_cipher_update()only the input length (second commit).Tested with cmake on mbedtls 4.0.0, mbedtls 3.6.4 (ASan), OpenSSL 3, NSS (ASan) and wolfSSL 5.9.2: all tests pass, and each commit passes on its own. Without the fix the new tests fail on mbedtls with "overrun detected in buffer".