commit bc4c91ab4655d10b55427ad7d4f1ae21fe2df06b
author David Benjamin <davidben@google.com> Tue May 18 16:06:30 2021 -0400
committer Adam Langley <agl@google.com> Thu Jun 03 20:43:22 2021 +0000
tree b780a36261f2aaf6c1629a27ba7d38b51cee2b2f
parent 4848294f43c34268181510a50a747e7b31bc8183

DTLS-SRTP is only defined for DTLS.

This avoids needing to worry about the interaction with renegotiation
which, in turn, means we can drop the init callback. (If we did support
DTLS renegotiation, we'd probably want to forbid the parameter from
changing anyway. Changing your SRTP parameters partway through will
likely confuse the RTP half of the application anyway.)

Change-Id: Ifef1e9479d9df296b69b0d296f6bef57b13da68e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47905
Reviewed-by: Adam Langley <agl@google.com>

ssl/t1_lib.cc

diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc
index e034bda..f6ba368 100644
--- a/ssl/t1_lib.cc
+++ b/ssl/t1_lib.cc
@@ -1713,17 +1713,13 @@
 //
 // https://tools.ietf.org/html/rfc5764
 
-
-static void ext_srtp_init(SSL_HANDSHAKE *hs) {
-  hs->ssl->s3->srtp_profile = NULL;
-}
-
 static bool ext_srtp_add_clienthello(SSL_HANDSHAKE *hs, CBB *out) {
   SSL *const ssl = hs->ssl;
   const STACK_OF(SRTP_PROTECTION_PROFILE) *profiles =
       SSL_get_srtp_profiles(ssl);
   if (profiles == NULL ||
-      sk_SRTP_PROTECTION_PROFILE_num(profiles) == 0) {
+      sk_SRTP_PROTECTION_PROFILE_num(profiles) == 0 ||
+      !SSL_is_dtls(ssl)) {
     return true;
   }
 
@@ -1759,6 +1755,7 @@
   // single uint16_t profile ID, then followed by a u8-prefixed srtp_mki field.
   //
   // See https://tools.ietf.org/html/rfc5764#section-4.1.1
+  assert(SSL_is_dtls(ssl));
   CBS profile_ids, srtp_mki;
   uint16_t profile_id;
   if (!CBS_get_u16_length_prefixed(contents, &profile_ids) ||
@@ -1793,7 +1790,8 @@
 static bool ext_srtp_parse_clienthello(SSL_HANDSHAKE *hs, uint8_t *out_alert,
                                        CBS *contents) {
   SSL *const ssl = hs->ssl;
-  if (contents == NULL) {
+  // DTLS-SRTP is only defined for DTLS.
+  if (contents == NULL || !SSL_is_dtls(ssl)) {
     return true;
   }
 
@@ -1837,6 +1835,7 @@
     return true;
   }
 
+  assert(SSL_is_dtls(ssl));
   CBB contents, profile_ids;
   if (!CBB_add_u16(out, TLSEXT_TYPE_srtp) ||
       !CBB_add_u16_length_prefixed(out, &contents) ||
@@ -3204,7 +3203,7 @@
   },
   {
     TLSEXT_TYPE_srtp,
-    ext_srtp_init,
+    NULL,
     ext_srtp_add_clienthello,
     ext_srtp_parse_serverhello,
     ext_srtp_parse_clienthello,

ssl/test/runner/runner.go

diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 4629a69..176fdd9 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -8061,6 +8061,40 @@
 					shouldFail:    true,
 					expectedError: ":BAD_SRTP_PROTECTION_PROFILE_LIST:",
 				})
+			} else {
+				// DTLS-SRTP is not defined for other protocols. Configuring it
+				// on the client and server should ignore the extension.
+				testCases = append(testCases, testCase{
+					protocol: protocol,
+					name:     "SRTP-Client-Ignore-" + suffix,
+					config: Config{
+						MaxVersion:             ver.version,
+						SRTPProtectionProfiles: []uint16{40, SRTP_AES128_CM_HMAC_SHA1_80, 42},
+					},
+					flags: []string{
+						"-srtp-profiles",
+						"SRTP_AES128_CM_SHA1_80:SRTP_AES128_CM_SHA1_32",
+					},
+					expectations: connectionExpectations{
+						srtpProtectionProfile: 0,
+					},
+				})
+				testCases = append(testCases, testCase{
+					protocol: protocol,
+					testType: serverTest,
+					name:     "SRTP-Server-Ignore-" + suffix,
+					config: Config{
+						MaxVersion:             ver.version,
+						SRTPProtectionProfiles: []uint16{40, SRTP_AES128_CM_HMAC_SHA1_80, 42},
+					},
+					flags: []string{
+						"-srtp-profiles",
+						"SRTP_AES128_CM_SHA1_80:SRTP_AES128_CM_SHA1_32",
+					},
+					expectations: connectionExpectations{
+						srtpProtectionProfile: 0,
+					},
+				})
 			}
 
 			// Test SCT list.