Revert "Remove code supporting the SDES crypto mode in SDP"

This reverts commit ee212a7.

Reason for revert: Don't remove until downstream issues resolved

Original change's description:
> Remove code supporting the SDES crypto mode in SDP
>
> Removes the ability to accept nonencrypted answers to encrypted offers.
> Fixes some logic around bundled sessions and requirement for
> transport parameters.
>
> Bug: webrtc:11066
> Change-Id: I56d8628d223614918a1e5260fdb8a117c8c02dbd
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/236344
> Commit-Queue: Harald Alvestrand <[email protected]>
> Reviewed-by: Niels Moller <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#35298}

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: webrtc:11066
Change-Id: I0c400ceffe1b08e0be7b44abbb54c8a032128f05
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/237223
Reviewed-by: Niels Moller <[email protected]>
Reviewed-by: Henrik Boström <[email protected]>
Commit-Queue: Harald Alvestrand <[email protected]>
Cr-Commit-Position: refs/heads/main@{#35312}

Author: Harald Alvestrand

api/BUILD.gn

@@ -133,6 +133,7 @@ rtc_library("libjingle_peerconnection_api") {
   sources = [
     "candidate.cc",
     "candidate.h",
+    "crypto_params.h",
     "data_channel_interface.cc",
     "data_channel_interface.h",
     "dtls_transport_interface.cc",

api/crypto_params.h

@@ -0,0 +1,41 @@
+/*
+ *  Copyright (c) 2004 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#ifndef API_CRYPTO_PARAMS_H_
+#define API_CRYPTO_PARAMS_H_
+
+#include <string>
+
+namespace cricket {
+
+// Parameters for SRTP negotiation, as described in RFC 4568.
+// TODO(benwright) - Rename to SrtpCryptoParams as these only apply to SRTP and
+// not generic crypto parameters for WebRTC.
+struct CryptoParams {
+  CryptoParams() : tag(0) {}
+  CryptoParams(int t,
+               const std::string& cs,
+               const std::string& kp,
+               const std::string& sp)
+      : tag(t), cipher_suite(cs), key_params(kp), session_params(sp) {}
+
+  bool Matches(const CryptoParams& params) const {
+    return (tag == params.tag && cipher_suite == params.cipher_suite);
+  }
+
+  int tag;
+  std::string cipher_suite;
+  std::string key_params;
+  std::string session_params;
+};
+
+}  // namespace cricket
+
+#endif  // API_CRYPTO_PARAMS_H_

p2p/base/transport_description.h

@@ -24,6 +24,17 @@
 
 namespace cricket {
 
+// SEC_ENABLED and SEC_REQUIRED should only be used if the session
+// was negotiated over TLS, to protect the inline crypto material
+// exchange.
+// SEC_DISABLED: No crypto in outgoing offer, ignore any supplied crypto.
+// SEC_ENABLED:  Crypto in outgoing offer and answer (if supplied in offer).
+// SEC_REQUIRED: Crypto in outgoing offer and answer. Fail any offer with absent
+//               or unsupported crypto.
+// TODO(deadbeef): Remove this or rename it to something more appropriate, like
+// SdesPolicy.
+enum SecurePolicy { SEC_DISABLED, SEC_ENABLED, SEC_REQUIRED };
+
 // Whether our side of the call is driving the negotiation, or the other side.
 enum IceRole { ICEROLE_CONTROLLING = 0, ICEROLE_CONTROLLED, ICEROLE_UNKNOWN };
 

p2p/base/transport_description_factory.cc

@@ -21,7 +21,8 @@
 
 namespace cricket {
 
-TransportDescriptionFactory::TransportDescriptionFactory() {}
+TransportDescriptionFactory::TransportDescriptionFactory()
+    : secure_(SEC_DISABLED) {}
 
 TransportDescriptionFactory::~TransportDescriptionFactory() = default;
 
@@ -46,7 +47,7 @@ std::unique_ptr<TransportDescription> TransportDescriptionFactory::CreateOffer(
   }
 
   // If we are trying to establish a secure transport, add a fingerprint.
-  if (IsEncrypted()) {
+  if (secure_ == SEC_ENABLED || secure_ == SEC_REQUIRED) {
     // Fail if we can't create the fingerprint.
     // If we are the initiator set role to "actpass".
     if (!SetSecurityInfo(desc.get(), CONNECTIONROLE_ACTPASS)) {
@@ -89,7 +90,7 @@ std::unique_ptr<TransportDescription> TransportDescriptionFactory::CreateAnswer(
   // Negotiate security params.
   if (offer && offer->identity_fingerprint.get()) {
     // The offer supports DTLS, so answer with DTLS, as long as we support it.
-    if (IsEncrypted()) {
+    if (secure_ == SEC_ENABLED || secure_ == SEC_REQUIRED) {
       ConnectionRole role = CONNECTIONROLE_NONE;
       // If the offer does not constrain the role, go with preference.
       if (offer->connection_role == CONNECTIONROLE_ACTPASS) {
@@ -115,7 +116,7 @@ std::unique_ptr<TransportDescription> TransportDescriptionFactory::CreateAnswer(
         return NULL;
       }
     }
-  } else if (require_transport_attributes && IsEncrypted()) {
+  } else if (require_transport_attributes && secure_ == SEC_REQUIRED) {
     // We require DTLS, but the other side didn't offer it. Fail.
     RTC_LOG(LS_WARNING) << "Failed to create TransportDescription answer "
                            "because of incompatible security settings";

p2p/base/transport_description_factory.h

@@ -40,20 +40,20 @@ class TransportDescriptionFactory {
   TransportDescriptionFactory();
   ~TransportDescriptionFactory();
 
+  SecurePolicy secure() const { return secure_; }
   // The certificate to use when setting up DTLS.
   const rtc::scoped_refptr<rtc::RTCCertificate>& certificate() const {
     return certificate_;
   }
 
-  // Specifies the certificate to use.
-  // When a certificate is set, transport will be encrypted.
+  // Specifies the transport security policy to use.
+  void set_secure(SecurePolicy s) { secure_ = s; }
+  // Specifies the certificate to use (only used when secure != SEC_DISABLED).
   void set_certificate(
       const rtc::scoped_refptr<rtc::RTCCertificate>& certificate) {
     certificate_ = certificate;
   }
 
-  bool IsEncrypted() const { return certificate_ != nullptr; }
-
   // Creates a transport description suitable for use in an offer.
   std::unique_ptr<TransportDescription> CreateOffer(
       const TransportOptions& options,
@@ -77,6 +77,7 @@ class TransportDescriptionFactory {
   bool SetSecurityInfo(TransportDescription* description,
                        ConnectionRole role) const;
 
+  SecurePolicy secure_;
   rtc::scoped_refptr<rtc::RTCCertificate> certificate_;
 };
 

p2p/base/transport_description_factory_unittest.cc

@@ -144,19 +144,17 @@ class TransportDescriptionFactoryTest : public ::testing::Test {
   }
 
  protected:
-  void SetDtls(bool f1_dtls, bool f2_dtls) {
-    if (f1_dtls) {
+  void SetDtls(bool dtls) {
+    if (dtls) {
+      f1_.set_secure(cricket::SEC_ENABLED);
+      f2_.set_secure(cricket::SEC_ENABLED);
       f1_.set_certificate(cert1_);
-    } else {
-      f1_.set_certificate(nullptr);
-    }
-    if (f2_dtls) {
       f2_.set_certificate(cert2_);
     } else {
-      f2_.set_certificate(nullptr);
+      f1_.set_secure(cricket::SEC_DISABLED);
+      f2_.set_secure(cricket::SEC_DISABLED);
     }
   }
-  void SetDtls(bool dtls) { SetDtls(dtls, dtls); }
 
   cricket::IceCredentialsIterator ice_credentials_;
   TransportDescriptionFactory f1_;
@@ -173,19 +171,33 @@ TEST_F(TransportDescriptionFactoryTest, TestOfferDefault) {
 }
 
 TEST_F(TransportDescriptionFactoryTest, TestOfferDtls) {
-  SetDtls(true);
+  f1_.set_secure(cricket::SEC_ENABLED);
+  f1_.set_certificate(cert1_);
   std::string digest_alg;
   ASSERT_TRUE(
       cert1_->GetSSLCertificate().GetSignatureDigestAlgorithm(&digest_alg));
   std::unique_ptr<TransportDescription> desc =
       f1_.CreateOffer(TransportOptions(), NULL, &ice_credentials_);
   CheckDesc(desc.get(), "", "", "", digest_alg);
+  // Ensure it also works with SEC_REQUIRED.
+  f1_.set_secure(cricket::SEC_REQUIRED);
+  desc = f1_.CreateOffer(TransportOptions(), NULL, &ice_credentials_);
+  CheckDesc(desc.get(), "", "", "", digest_alg);
+}
+
+// Test generating an offer with DTLS fails with no identity.
+TEST_F(TransportDescriptionFactoryTest, TestOfferDtlsWithNoIdentity) {
+  f1_.set_secure(cricket::SEC_ENABLED);
+  std::unique_ptr<TransportDescription> desc =
+      f1_.CreateOffer(TransportOptions(), NULL, &ice_credentials_);
+  ASSERT_TRUE(desc.get() == NULL);
 }
 
 // Test updating an offer with DTLS to pick ICE.
 // The ICE credentials should stay the same in the new offer.
 TEST_F(TransportDescriptionFactoryTest, TestOfferDtlsReofferDtls) {
-  SetDtls(true);
+  f1_.set_secure(cricket::SEC_ENABLED);
+  f1_.set_certificate(cert1_);
   std::string digest_alg;
   ASSERT_TRUE(
       cert1_->GetSSLCertificate().GetSignatureDigestAlgorithm(&digest_alg));
@@ -225,7 +237,8 @@ TEST_F(TransportDescriptionFactoryTest, TestReanswer) {
 
 // Test that we handle answering an offer with DTLS with no DTLS.
 TEST_F(TransportDescriptionFactoryTest, TestAnswerDtlsToNoDtls) {
-  SetDtls(true, false);
+  f1_.set_secure(cricket::SEC_ENABLED);
+  f1_.set_certificate(cert1_);
   std::unique_ptr<TransportDescription> offer =
       f1_.CreateOffer(TransportOptions(), NULL, &ice_credentials_);
   ASSERT_TRUE(offer.get() != NULL);
@@ -234,20 +247,31 @@ TEST_F(TransportDescriptionFactoryTest, TestAnswerDtlsToNoDtls) {
   CheckDesc(desc.get(), "", "", "", "");
 }
 
-// Test that we reject answering an offer without DTLS if we have DTLS enabled.
+// Test that we handle answering an offer without DTLS if we have DTLS enabled,
+// but fail if we require DTLS.
 TEST_F(TransportDescriptionFactoryTest, TestAnswerNoDtlsToDtls) {
-  SetDtls(false, true);
+  f2_.set_secure(cricket::SEC_ENABLED);
+  f2_.set_certificate(cert2_);
   std::unique_ptr<TransportDescription> offer =
       f1_.CreateOffer(TransportOptions(), NULL, &ice_credentials_);
   ASSERT_TRUE(offer.get() != NULL);
   std::unique_ptr<TransportDescription> desc = f2_.CreateAnswer(
       offer.get(), TransportOptions(), true, NULL, &ice_credentials_);
-  ASSERT_FALSE(desc);
+  CheckDesc(desc.get(), "", "", "", "");
+  f2_.set_secure(cricket::SEC_REQUIRED);
+  desc = f2_.CreateAnswer(offer.get(), TransportOptions(), true, NULL,
+                          &ice_credentials_);
+  ASSERT_TRUE(desc.get() == NULL);
 }
 
-// Test that we handle answering an DTLS offer with DTLS.
+// Test that we handle answering an DTLS offer with DTLS, both if we have
+// DTLS enabled and required.
 TEST_F(TransportDescriptionFactoryTest, TestAnswerDtlsToDtls) {
-  SetDtls(true, true);
+  f1_.set_secure(cricket::SEC_ENABLED);
+  f1_.set_certificate(cert1_);
+
+  f2_.set_secure(cricket::SEC_ENABLED);
+  f2_.set_certificate(cert2_);
   // f2_ produces the answer that is being checked in this test, so the
   // answer must contain fingerprint lines with cert2_'s digest algorithm.
   std::string digest_alg2;
@@ -260,6 +284,10 @@ TEST_F(TransportDescriptionFactoryTest, TestAnswerDtlsToDtls) {
   std::unique_ptr<TransportDescription> desc = f2_.CreateAnswer(
       offer.get(), TransportOptions(), true, NULL, &ice_credentials_);
   CheckDesc(desc.get(), "", "", "", digest_alg2);
+  f2_.set_secure(cricket::SEC_REQUIRED);
+  desc = f2_.CreateAnswer(offer.get(), TransportOptions(), true, NULL,
+                          &ice_credentials_);
+  CheckDesc(desc.get(), "", "", "", digest_alg2);
 }
 
 // Test that ice ufrag and password is changed in an updated offer and answer
@@ -326,7 +354,11 @@ TEST_F(TransportDescriptionFactoryTest, CreateAnswerIceCredentialsIterator) {
 }
 
 TEST_F(TransportDescriptionFactoryTest, CreateAnswerToDtlsActpassOffer) {
-  SetDtls(true);
+  f1_.set_secure(cricket::SEC_ENABLED);
+  f1_.set_certificate(cert1_);
+
+  f2_.set_secure(cricket::SEC_ENABLED);
+  f2_.set_certificate(cert2_);
   cricket::TransportOptions options;
   std::unique_ptr<TransportDescription> offer =
       f1_.CreateOffer(options, nullptr, &ice_credentials_);
@@ -337,7 +369,11 @@ TEST_F(TransportDescriptionFactoryTest, CreateAnswerToDtlsActpassOffer) {
 }
 
 TEST_F(TransportDescriptionFactoryTest, CreateAnswerToDtlsActiveOffer) {
-  SetDtls(true);
+  f1_.set_secure(cricket::SEC_ENABLED);
+  f1_.set_certificate(cert1_);
+
+  f2_.set_secure(cricket::SEC_ENABLED);
+  f2_.set_certificate(cert2_);
   cricket::TransportOptions options;
   std::unique_ptr<TransportDescription> offer =
       f1_.CreateOffer(options, nullptr, &ice_credentials_);
@@ -349,7 +385,11 @@ TEST_F(TransportDescriptionFactoryTest, CreateAnswerToDtlsActiveOffer) {
 }
 
 TEST_F(TransportDescriptionFactoryTest, CreateAnswerToDtlsPassiveOffer) {
-  SetDtls(true);
+  f1_.set_secure(cricket::SEC_ENABLED);
+  f1_.set_certificate(cert1_);
+
+  f2_.set_secure(cricket::SEC_ENABLED);
+  f2_.set_certificate(cert2_);
   cricket::TransportOptions options;
   std::unique_ptr<TransportDescription> offer =
       f1_.CreateOffer(options, nullptr, &ice_credentials_);

pc/BUILD.gn

@@ -81,6 +81,8 @@ rtc_library("rtc_pc_base") {
     "sctp_transport.h",
     "sctp_utils.cc",
     "sctp_utils.h",
+    "srtp_filter.cc",
+    "srtp_filter.h",
     "srtp_session.cc",
     "srtp_session.h",
     "srtp_transport.cc",
@@ -884,6 +886,7 @@ if (rtc_include_tests && !build_with_chromium) {
       "rtp_transport_unittest.cc",
       "sctp_transport_unittest.cc",
       "session_description_unittest.cc",
+      "srtp_filter_unittest.cc",
       "srtp_session_unittest.cc",
       "srtp_transport_unittest.cc",
       "test/rtp_transport_test_util.h",

pc/channel.h

@@ -47,6 +47,7 @@
 #include "pc/rtp_transport.h"
 #include "pc/rtp_transport_internal.h"
 #include "pc/session_description.h"
+#include "pc/srtp_filter.h"
 #include "pc/srtp_transport.h"
 #include "rtc_base/async_packet_socket.h"
 #include "rtc_base/async_udp_socket.h"

pc/dtls_srtp_transport.h

@@ -15,6 +15,7 @@
 #include <vector>
 
 #include "absl/types/optional.h"
+#include "api/crypto_params.h"
 #include "api/dtls_transport_interface.h"
 #include "api/rtc_error.h"
 #include "p2p/base/dtls_transport_internal.h"
@@ -48,6 +49,15 @@ class DtlsSrtpTransport : public SrtpTransport {
 
   void SetOnDtlsStateChange(std::function<void(void)> callback);
 
+  RTCError SetSrtpSendKey(const cricket::CryptoParams& params) override {
+    return RTCError(RTCErrorType::UNSUPPORTED_OPERATION,
+                    "Set SRTP keys for DTLS-SRTP is not supported.");
+  }
+  RTCError SetSrtpReceiveKey(const cricket::CryptoParams& params) override {
+    return RTCError(RTCErrorType::UNSUPPORTED_OPERATION,
+                    "Set SRTP keys for DTLS-SRTP is not supported.");
+  }
+
   // If `active_reset_srtp_params_` is set to be true, the SRTP parameters will
   // be reset whenever the DtlsTransports are reset.
   void SetActiveResetSrtpParams(bool active_reset_srtp_params) {