srtp: pass the output space to the cipher on AEAD unprotect

crypto/cipher/aes_gcm_mbedtls.c

@@ -396,11 +396,8 @@ static srtp_err_status_t srtp_aes_gcm_mbedtls_encrypt(void *cv,
     }
 
     /*
-      There are tests that check a buffer is only written too as mush as needed
-      even if there is space in buffer. psa_aead_encrypt uses that extra space,
-      probable for performance reasons. For adjust the dst_len to be only what
-      is required to avoid the tests failing. The tests should be changed as
-      there is nothing wrong with using the free buffer space.
+      psa_aead_encrypt can write to all of the output buffer it is given, not
+      only the encrypted data and tag, so limit it to the length required.
     */
     *dst_len = src_len + c->tag_len;
 
@@ -452,6 +449,13 @@ static srtp_err_status_t srtp_aes_gcm_mbedtls_decrypt(void *cv,
         return srtp_err_status_buffer_small;
     }
 
+    /*
+      psa_aead_decrypt can write to all of the output buffer it is given, not
+      only the plaintext, and clears all of it if the tag does not match, so
+      limit it to the plaintext length.
+    */
+    *dst_len = src_len - c->tag_len;
+
     debug_print(srtp_mod_aes_gcm, "AAD: %s",
                 srtp_octet_string_hex_string(c->aad, c->aad_size));
 

crypto/cipher/aes_icm_mbedtls.c

@@ -448,8 +448,12 @@ static srtp_err_status_t srtp_aes_icm_mbedtls_encrypt(void *cv,
         return srtp_err_status_buffer_small;
     }
 
+    /*
+      psa_cipher_update can write to all of the output buffer it is given, not
+      only the encrypted data, so limit it to the length of the input.
+    */
     status =
-        psa_cipher_update(&(c->ctx->op), src, src_len, dst, *dst_len, &out_len);
+        psa_cipher_update(&(c->ctx->op), src, src_len, dst, src_len, &out_len);
 
     if (status != PSA_SUCCESS) {
         debug_print(srtp_mod_aes_icm, "encrypt error: %d", status);

crypto/test/cipher_driver.c

@@ -459,9 +459,11 @@ srtp_err_status_t cipher_driver_test_api(srtp_cipher_type_t *ct,
     // dst len larger than src len
     src_len = sizeof(plaintext);
     dst_len = sizeof(encrypted);
+    overrun_check_prepare(encrypted, 0, sizeof(encrypted));
     status = srtp_cipher_encrypt(c, plaintext, src_len, encrypted, &dst_len);
     CHECK_OK(status);
     CHECK(dst_len == src_len + tag_len);
+    CHECK_OVERRUN(encrypted, dst_len, sizeof(encrypted));
 
     reint_cipher(c, test_key, iv, srtp_direction_encrypt);
 
@@ -514,10 +516,12 @@ srtp_err_status_t cipher_driver_test_api(srtp_cipher_type_t *ct,
     // dst len larger than src len
     src_len = encrypted_len;
     dst_len = sizeof(decrypted);
+    overrun_check_prepare(decrypted, 0, sizeof(decrypted));
     status = srtp_cipher_decrypt(c, encrypted, src_len, decrypted, &dst_len);
     CHECK_OK(status);
     CHECK(dst_len == sizeof(plaintext));
     CHECK_BUFFER_EQUAL(plaintext, decrypted, sizeof(plaintext));
+    CHECK_OVERRUN(decrypted, dst_len, sizeof(decrypted));
 
     reint_cipher(c, test_key, iv, srtp_direction_decrypt);
 

srtp/srtp.c

@@ -2493,9 +2493,10 @@ static srtp_err_status_t srtp_unprotect_aead(srtp_ctx_t *ctx,
 
     /* Decrypt the ciphertext.  This also checks the auth tag based
      * on the AAD we just specified above */
-    status =
-        srtp_cipher_decrypt(session_keys->rtp_cipher, srtp + enc_start,
-                            enc_octet_len, rtp + enc_start, &enc_octet_len);
+    size_t outlen = *rtp_len - enc_start;
+    status = srtp_cipher_decrypt(session_keys->rtp_cipher, srtp + enc_start,
+                                 enc_octet_len, rtp + enc_start, &outlen);
+    enc_octet_len = outlen;
     if (status) {
         return status;
     }
@@ -4220,9 +4221,10 @@ static srtp_err_status_t srtp_unprotect_rtcp_aead(
 
     /* if we're decrypting, exor keystream into the message */
     if (*trailer_p & SRTCP_E_BYTE_BIT) {
-        status = srtp_cipher_decrypt(session_keys->rtcp_cipher,
-                                     srtcp + enc_start, enc_octet_len,
-                                     rtcp + enc_start, &enc_octet_len);
+        size_t out_len = *rtcp_len - enc_start;
+        status =
+            srtp_cipher_decrypt(session_keys->rtcp_cipher, srtcp + enc_start,
+                                enc_octet_len, rtcp + enc_start, &out_len);
         if (status) {
             return status;
         }

test/srtp_driver.c

@@ -111,6 +111,10 @@ srtp_err_status_t srtp_test_empty_payload_gcm(void);
 srtp_err_status_t srtp_test_short_packet_gcm_mki(void);
 
 srtp_err_status_t srtp_test_rtcp_gcm_not_encrypted(void);
+
+srtp_err_status_t srtp_test_gcm_unprotect_not_in_place(void);
+
+srtp_err_status_t srtp_test_rtcp_gcm_unprotect_not_in_place(void);
 #endif
 
 srtp_err_status_t srtp_test_remove_stream(void);
@@ -1028,6 +1032,22 @@ int main(int argc, char *argv[])
             printf("failed\n");
             exit(1);
         }
+
+        printf("testing gcm unprotect not in place()...");
+        if (srtp_test_gcm_unprotect_not_in_place() == srtp_err_status_ok) {
+            printf("passed\n");
+        } else {
+            printf("failed\n");
+            exit(1);
+        }
+
+        printf("testing rtcp gcm unprotect not in place()...");
+        if (srtp_test_rtcp_gcm_unprotect_not_in_place() == srtp_err_status_ok) {
+            printf("passed\n");
+        } else {
+            printf("failed\n");
+            exit(1);
+        }
 #endif
     }
 
@@ -4140,6 +4160,115 @@ srtp_err_status_t srtp_test_rtcp_gcm_not_encrypted(void)
     return srtp_err_status_ok;
 }
 
+srtp_err_status_t srtp_test_gcm_unprotect_not_in_place(void)
+{
+    /*
+     * Unprotecting into a separate output buffer only needs room for the RTP
+     * packet, not for the GCM tag. The output length is given first as
+     * exactly the RTP packet length and then as the whole buffer; the area
+     * after the RTP packet is a guard that detects writes beyond it.
+     */
+    uint32_t ssrc = 0xcafebabe;
+    size_t tag_len = 16;
+    srtp_policy_t policy;
+    srtp_t sender, receiver;
+
+    CHECK_OK(srtp_policy_create(&policy));
+    CHECK_OK(srtp_policy_set_profile(policy, srtp_profile_aead_aes_128_gcm));
+    CHECK_OK(
+        srtp_policy_set_ssrc(policy, (srtp_ssrc_t){ ssrc_specific, ssrc }));
+    CHECK_OK(policy_set_key(policy, test_key_gcm));
+    CHECK_OK(srtp_create(&sender, policy));
+    CHECK_OK(srtp_create(&receiver, policy));
+
+    for (uint16_t seq = 1; seq <= 2; seq++) {
+        size_t rtp_len, buffer_len;
+        uint8_t *rtp = create_rtp_test_packet(28, ssrc, seq, 1234, false,
+                                              &rtp_len, &buffer_len);
+        uint8_t *srtp = (uint8_t *)malloc(buffer_len);
+        uint8_t *output = (uint8_t *)malloc(buffer_len);
+        CHECK(srtp != NULL && output != NULL);
+
+        size_t srtp_len = buffer_len;
+        CHECK_OK(srtp_protect(sender, rtp, rtp_len, srtp, &srtp_len, 0));
+        CHECK(srtp_len == rtp_len + tag_len);
+
+        overrun_check_prepare(output, 0, buffer_len);
+        size_t output_len = seq == 1 ? rtp_len : buffer_len;
+        CHECK_OK(srtp_unprotect(receiver, srtp, srtp_len, output, &output_len));
+        CHECK(output_len == rtp_len);
+        CHECK_BUFFER_EQUAL(output, rtp, rtp_len);
+        CHECK_OVERRUN(output, rtp_len, buffer_len);
+
+        free(output);
+        free(srtp);
+        free(rtp);
+    }
+
+    CHECK_OK(srtp_dealloc(sender));
+    CHECK_OK(srtp_dealloc(receiver));
+    srtp_policy_destroy(policy);
+
+    return srtp_err_status_ok;
+}
+
+srtp_err_status_t srtp_test_rtcp_gcm_unprotect_not_in_place(void)
+{
+    /*
+     * As above for an encrypted (E=1) SRTCP packet: the output only needs
+     * room for the RTCP packet, not for the GCM tag or the trailer.
+     */
+    uint32_t ssrc = 0xcafebabe;
+    size_t tag_len = 16;
+    size_t trailer_len = 4;
+    srtp_policy_t policy;
+    srtp_t sender, receiver;
+
+    CHECK_OK(srtp_policy_create(&policy));
+    CHECK_OK(srtp_policy_set_profile(policy, srtp_profile_aead_aes_128_gcm));
+    CHECK_OK(srtp_policy_set_sec_serv(policy, sec_serv_conf_and_auth,
+                                      sec_serv_conf_and_auth));
+    CHECK_OK(
+        srtp_policy_set_ssrc(policy, (srtp_ssrc_t){ ssrc_specific, ssrc }));
+    CHECK_OK(policy_set_key(policy, test_key_gcm));
+    CHECK_OK(srtp_create(&sender, policy));
+    CHECK_OK(srtp_create(&receiver, policy));
+
+    for (int i = 0; i < 2; i++) {
+        size_t rtcp_len, buffer_len;
+        uint8_t *rtcp =
+            create_rtcp_test_packet(28, ssrc, &rtcp_len, &buffer_len);
+        uint8_t *srtcp = (uint8_t *)malloc(buffer_len);
+        uint8_t *output = (uint8_t *)malloc(buffer_len);
+        CHECK(srtcp != NULL && output != NULL);
+
+        size_t srtcp_len = buffer_len;
+        CHECK_OK(
+            srtp_protect_rtcp(sender, rtcp, rtcp_len, srtcp, &srtcp_len, 0));
+        CHECK(srtcp_len == rtcp_len + tag_len + trailer_len);
+        // the E flag is the top bit of the trailer that follows the GCM tag
+        CHECK((srtcp[srtcp_len - trailer_len] & 0x80) != 0);
+
+        overrun_check_prepare(output, 0, buffer_len);
+        size_t output_len = i == 0 ? rtcp_len : buffer_len;
+        CHECK_OK(srtp_unprotect_rtcp(receiver, srtcp, srtcp_len, output,
+                                     &output_len));
+        CHECK(output_len == rtcp_len);
+        CHECK_BUFFER_EQUAL(output, rtcp, rtcp_len);
+        CHECK_OVERRUN(output, rtcp_len, buffer_len);
+
+        free(output);
+        free(srtcp);
+        free(rtcp);
+    }
+
+    CHECK_OK(srtp_dealloc(sender));
+    CHECK_OK(srtp_dealloc(receiver));
+    srtp_policy_destroy(policy);
+
+    return srtp_err_status_ok;
+}
+
 #endif
 
 /*