Revert "Fix data channel message integrity violation"

This reverts commit 6cbff752f52bf3f70168d551c33ce719bd8e0663.

Reason for revert: breaking downstream projects, Win MSVC x86 dbg and Win x86 Clang rel

Original change's description:
> Fix data channel message integrity violation
> 
> SCTP message chunks and notifications are being delivered interleaved.
> However, the way the code was structured previously, a notification
> would interrupt reassembly of a message chunk and hand out the partial
> message, thereby violating message integrity. This patch separates the
> handling of notifications and reassembly of messages.
> 
> Additional changes:
> 
> - Remove illegal cast from non-validated u32 to enum (PPID)
> - Drop partial messages if the SID has been changed but EOR not yet
>   received instead of delivering them. (This should never happen
>   anyway.)
> - Don't treat TSN as timestamp (wat)
> 
> Bug: webrtc:11708
> Change-Id: I4e2fe2262feda2a96d2ae3f6ce9b06370d9878ae
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/177527
> Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
> Reviewed-by: Taylor <deadbeef@webrtc.org>
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Commit-Queue: Tommi <tommi@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#31605}

TBR=deadbeef@webrtc.org,kwiberg@webrtc.org,tommi@webrtc.org,hta@webrtc.org,lennart.grahl@gmail.com

Change-Id: I6d6c5a11835f155f8c449b996d034f43b8db452c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:11708
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/178488
Reviewed-by: Alessio Bazzica <alessiob@webrtc.org>
Commit-Queue: Alessio Bazzica <alessiob@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31606}

AUTHORS

@@ -90,7 +90,6 @@ CZ Theng <cz.theng@gmail.com>
 Miguel Paris <mparisdiaz@gmail.com>
 Raman Budny <budnyjj@gmail.com>
 Stephan Hartmann <stha09@googlemail.com>
-Lennart Grahl <lennart.grahl@gmail.com>
 
 &yet LLC <*@andyet.com>
 8x8 Inc. <*@sip-communicator.org>

media/BUILD.gn

@@ -659,14 +659,5 @@ if (rtc_include_tests) {
     if (is_ios) {
       deps += [ ":rtc_media_unittests_bundle_data" ]
     }
-
-    if (rtc_enable_sctp && rtc_build_usrsctp) {
-      include_dirs = [
-        # TODO(jiayl): move this into the public_configs of
-        # //third_party/usrsctp/BUILD.gn.
-        "//third_party/usrsctp/usrsctplib",
-      ]
-      deps += [ "//third_party/usrsctp" ]
-    }
   }
 }

media/sctp/sctp_transport.cc

@@ -62,7 +62,7 @@ ABSL_CONST_INIT rtc::GlobalLock g_usrsctp_lock_;
 // http://www.iana.org/assignments/sctp-parameters/sctp-parameters.xml
 // The value is not used by SCTP itself. It indicates the protocol running
 // on top of SCTP.
-enum {
+enum PayloadProtocolIdentifier {
   PPID_NONE = 0,  // No protocol is specified.
   // Matches the PPIDs in mozilla source and
   // https://datatracker.ietf.org/doc/draft-ietf-rtcweb-data-protocol Sec. 9
@@ -143,7 +143,7 @@ void DebugSctpPrintf(const char* format, ...) {
 }
 
 // Get the PPID to use for the terminating fragment of this type.
-uint32_t GetPpid(cricket::DataMessageType type) {
+PayloadProtocolIdentifier GetPpid(cricket::DataMessageType type) {
   switch (type) {
     default:
     case cricket::DMT_NONE:
@@ -157,7 +157,8 @@ uint32_t GetPpid(cricket::DataMessageType type) {
   }
 }
 
-bool GetDataMediaType(uint32_t ppid, cricket::DataMessageType* dest) {
+bool GetDataMediaType(PayloadProtocolIdentifier ppid,
+                      cricket::DataMessageType* dest) {
   RTC_DCHECK(dest != NULL);
   switch (ppid) {
     case PPID_BINARY_PARTIAL:
@@ -381,10 +382,77 @@ class SctpTransport::UsrSctpWrapper {
                                  int flags,
                                  void* ulp_info) {
     SctpTransport* transport = static_cast<SctpTransport*>(ulp_info);
-    int result =
+    // Post data to the transport's receiver thread (copying it).
-        transport->OnDataOrNotificationFromSctp(data, length, rcv, flags);
+    // TODO(ldixon): Unclear if copy is needed as this method is responsible for
+    // memory cleanup. But this does simplify code.
+    const PayloadProtocolIdentifier ppid =
+        static_cast<PayloadProtocolIdentifier>(
+            rtc::NetworkToHost32(rcv.rcv_ppid));
+    DataMessageType type = DMT_NONE;
+    if (!GetDataMediaType(ppid, &type) && !(flags & MSG_NOTIFICATION)) {
+      // It's neither a notification nor a recognized data packet.  Drop it.
+      RTC_LOG(LS_ERROR) << "Received an unknown PPID " << ppid
+                        << " on an SCTP packet.  Dropping.";
       free(data);
-    return result;
+    } else {
+      ReceiveDataParams params;
+
+      params.sid = rcv.rcv_sid;
+      params.seq_num = rcv.rcv_ssn;
+      params.timestamp = rcv.rcv_tsn;
+      params.type = type;
+
+      // Expect only continuation messages belonging to the same sid, the sctp
+      // stack should ensure this.
+      if ((transport->partial_incoming_message_.size() != 0) &&
+          (rcv.rcv_sid != transport->partial_params_.sid)) {
+        // A message with a new sid, but haven't seen the EOR for the
+        // previous message. Deliver the previous partial message to avoid
+        // merging messages from different sid's.
+        transport->invoker_.AsyncInvoke<void>(
+            RTC_FROM_HERE, transport->network_thread_,
+            rtc::Bind(&SctpTransport::OnInboundPacketFromSctpToTransport,
+                      transport, transport->partial_incoming_message_,
+                      transport->partial_params_, transport->partial_flags_));
+
+        transport->partial_incoming_message_.Clear();
+      }
+
+      transport->partial_incoming_message_.AppendData(
+          reinterpret_cast<uint8_t*>(data), length);
+      transport->partial_params_ = params;
+      transport->partial_flags_ = flags;
+
+      free(data);
+
+      // Merge partial messages until they exceed the maximum send buffer size.
+      // This enables messages from a single send to be delivered in a single
+      // callback. Larger messages (originating from other implementations) will
+      // still be delivered in chunks.
+      if (!(flags & MSG_EOR) &&
+          (transport->partial_incoming_message_.size() < kSctpSendBufferSize)) {
+        return 1;
+      }
+
+      if (!(flags & MSG_EOR)) {
+        // TODO(bugs.webrtc.org/7774): We currently chunk messages if they are
+        // >= kSctpSendBufferSize. The better thing to do here is buffer up to
+        // the size negotiated in the SDP, and if a larger message is received
+        // close the channel and report the error. See discussion in the bug.
+        RTC_LOG(LS_WARNING) << "Chunking SCTP message without the EOR bit set.";
+      }
+
+      // The ownership of the packet transfers to |invoker_|. Using
+      // CopyOnWriteBuffer is the most convenient way to do this.
+      transport->invoker_.AsyncInvoke<void>(
+          RTC_FROM_HERE, transport->network_thread_,
+          rtc::Bind(&SctpTransport::OnInboundPacketFromSctpToTransport,
+                    transport, transport->partial_incoming_message_, params,
+                    flags));
+
+      transport->partial_incoming_message_.Clear();
+    }
+    return 1;
   }
 
   static SctpTransport* GetTransportFromSocket(struct socket* sock) {
@@ -1064,122 +1132,33 @@ void SctpTransport::OnPacketFromSctpToNetwork(
                          rtc::PacketOptions(), PF_NORMAL);
 }
 
-int SctpTransport::InjectDataOrNotificationFromSctpForTesting(
+void SctpTransport::OnInboundPacketFromSctpToTransport(
-    void* data,
+    const rtc::CopyOnWriteBuffer& buffer,
-    size_t length,
+    ReceiveDataParams params,
-    struct sctp_rcvinfo rcv,
     int flags) {
-  return OnDataOrNotificationFromSctp(data, length, rcv, flags);
+  RTC_DCHECK_RUN_ON(network_thread_);
-}
+  RTC_LOG(LS_VERBOSE) << debug_name_
-
+                      << "->OnInboundPacketFromSctpToTransport(...): "
-int SctpTransport::OnDataOrNotificationFromSctp(void* data,
+                         "Received SCTP data:"
-                                                size_t length,
+                         " sid="
-                                                struct sctp_rcvinfo rcv,
+                      << params.sid
-                                                int flags) {
+                      << " notification: " << (flags & MSG_NOTIFICATION)
-  // If data is NULL, the SCTP association has been closed.
+                      << " length=" << buffer.size();
-  if (!data) {
+  // Sending a packet with data == NULL (no data) is SCTPs "close the
+  // connection" message. This sets sock_ = NULL;
+  if (!buffer.size() || !buffer.data()) {
     RTC_LOG(LS_INFO) << debug_name_
-                     << "->OnSctpInboundPacket(...): "
+                     << "->OnInboundPacketFromSctpToTransport(...): "
                         "No data, closing.";
-    return 1;
+    return;
   }
-
-  // Handle notifications early.
-  // Note: Notifications are never split into chunks, so they can and should
-  //       be handled early and entirely separate from the reassembly
-  //       process.
   if (flags & MSG_NOTIFICATION) {
-    RTC_LOG(LS_VERBOSE) << debug_name_
+    OnNotificationFromSctp(buffer);
-                        << "->OnSctpInboundPacket(...): SCTP notification"
-                        << " length=" << length;
-
-    // Copy and dispatch asynchronously
-    rtc::CopyOnWriteBuffer notification(reinterpret_cast<uint8_t*>(data),
-                                        length);
-    invoker_.AsyncInvoke<void>(
-        RTC_FROM_HERE, network_thread_,
-        rtc::Bind(&SctpTransport::OnNotificationFromSctp, this, notification));
-    return 1;
-  }
-
-  // Log data chunk
-  const uint32_t ppid = rtc::NetworkToHost32(rcv.rcv_ppid);
-  RTC_LOG(LS_VERBOSE) << debug_name_
-                      << "->OnSctpInboundPacket(...): SCTP data chunk"
-                      << " length=" << length << ", sid=" << rcv.rcv_sid
-                      << ", ppid=" << ppid << ", ssn=" << rcv.rcv_ssn
-                      << ", cum-tsn=" << rcv.rcv_cumtsn
-                      << ", eor=" << ((flags & MSG_EOR) ? "y" : "n");
-
-  // Validate payload protocol identifier
-  DataMessageType type = DMT_NONE;
-  if (!GetDataMediaType(ppid, &type)) {
-    // Unexpected PPID, dropping
-    RTC_LOG(LS_ERROR) << "Received an unknown PPID " << ppid
-                      << " on an SCTP packet.  Dropping.";
-    return 1;
-  }
-
-  // Expect only continuation messages belonging to the same SID. The SCTP
-  // stack is expected to ensure this as long as the User Message
-  // Interleaving extension (RFC 8260) is not explicitly enabled, so this
-  // merely acts as a safeguard.
-  if ((partial_incoming_message_.size() != 0) &&
-      (rcv.rcv_sid != partial_params_.sid)) {
-    RTC_LOG(LS_ERROR) << "Received a new SID without EOR in the previous"
-                      << " SCTP packet. Discarding the previous packet.";
-    partial_incoming_message_.Clear();
-  }
-
-  // Copy metadata of interest
-  ReceiveDataParams params;
-  params.type = type;
-  params.sid = rcv.rcv_sid;
-  // Note that the SSN is identical for each chunk of the same message.
-  // Furthermore, it is increased per stream and not on the whole
-  // association.
-  params.seq_num = rcv.rcv_ssn;
-  // There is no timestamp field in the SCTP API
-  params.timestamp = 0;
-
-  // Append the chunk's data to the message buffer
-  partial_incoming_message_.AppendData(reinterpret_cast<uint8_t*>(data),
-                                       length);
-  partial_params_ = params;
-  partial_flags_ = flags;
-
-  // If the message is not yet complete...
-  if (!(flags & MSG_EOR)) {
-    if (partial_incoming_message_.size() < kSctpSendBufferSize) {
-      // We still have space in the buffer. Continue buffering chunks until
-      // the message is complete before handing it out.
-      return 1;
   } else {
-      // The sender is exceeding the maximum message size that we announced.
+    OnDataFromSctpToTransport(params, buffer);
-      // Spit out a warning but still hand out the partial message. Note that
-      // this behaviour is undesirable, see the discussion in issue 7774.
-      //
-      // TODO(lgrahl): Once sufficient time has passed and all supported
-      // browser versions obey the announced maximum message size, we should
-      // abort the SCTP association instead to prevent message integrity
-      // violation.
-      RTC_LOG(LS_ERROR) << "Handing out partial SCTP message.";
   }
 }
 
-  // Dispatch the complete message.
-  // The ownership of the packet transfers to |invoker_|. Using
-  // CopyOnWriteBuffer is the most convenient way to do this.
-  invoker_.AsyncInvoke<void>(
-      RTC_FROM_HERE, network_thread_,
-      rtc::Bind(&SctpTransport::OnDataFromSctpToTransport, this, params,
-                partial_incoming_message_));
-
-  // Reset the message buffer
-  partial_incoming_message_.Clear();
-  return 1;
-}
-
 void SctpTransport::OnDataFromSctpToTransport(
     const ReceiveDataParams& params,
     const rtc::CopyOnWriteBuffer& buffer) {

media/sctp/sctp_transport.h

@@ -34,7 +34,6 @@
 // Defined by "usrsctplib/usrsctp.h"
 struct sockaddr_conn;
 struct sctp_assoc_change;
-struct sctp_rcvinfo;
 struct sctp_stream_reset_event;
 struct sctp_sendv_spa;
 
@@ -59,8 +58,8 @@ struct SctpInboundPacket;
 //  8.  usrsctp_conninput(wrapped_data)
 // [network thread returns; sctp thread then calls the following]
 //  9.  OnSctpInboundData(data)
-//  10. SctpTransport::OnDataFromSctpToTransport(data)
 // [sctp thread returns having async invoked on the network thread]
+//  10. SctpTransport::OnInboundPacketFromSctpToTransport(inboundpacket)
 //  11. SctpTransport::OnDataFromSctpToTransport(data)
 //  12. SctpTransport::SignalDataReceived(data)
 // [from the same thread, methods registered/connected to
@@ -95,10 +94,6 @@ class SctpTransport : public SctpTransportInternal,
   void set_debug_name_for_testing(const char* debug_name) override {
     debug_name_ = debug_name;
   }
-  int InjectDataOrNotificationFromSctpForTesting(void* data,
-                                                 size_t length,
-                                                 struct sctp_rcvinfo rcv,
-                                                 int flags);
 
   // Exposed to allow Post call from c-callbacks.
   // TODO(deadbeef): Remove this or at least make it return a const pointer.
@@ -178,16 +173,14 @@ class SctpTransport : public SctpTransportInternal,
 
   // Called using |invoker_| to send packet on the network.
   void OnPacketFromSctpToNetwork(const rtc::CopyOnWriteBuffer& buffer);
-
+  // Called using |invoker_| to decide what to do with the packet.
-  // Called on the SCTP thread
+  // The |flags| parameter is used by SCTP to distinguish notification packets
-  int OnDataOrNotificationFromSctp(void* data,
+  // from other types of packets.
-                                   size_t length,
+  void OnInboundPacketFromSctpToTransport(const rtc::CopyOnWriteBuffer& buffer,
-                                   struct sctp_rcvinfo rcv,
+                                          ReceiveDataParams params,
                                           int flags);
-  // Called using |invoker_| to decide what to do with the data.
   void OnDataFromSctpToTransport(const ReceiveDataParams& params,
                                  const rtc::CopyOnWriteBuffer& buffer);
-  // Called using |invoker_| to decide what to do with the notification.
   void OnNotificationFromSctp(const rtc::CopyOnWriteBuffer& buffer);
   void OnNotificationAssocChange(const sctp_assoc_change& change);
 

media/sctp/sctp_transport_unittest.cc

@@ -25,7 +25,6 @@
 #include "rtc_base/logging.h"
 #include "rtc_base/thread.h"
 #include "test/gtest.h"
-#include "usrsctplib/usrsctp.h"
 
 namespace {
 static const int kDefaultTimeout = 10000;  // 10 seconds.
@@ -239,73 +238,6 @@ class SctpTransportTest : public ::testing::Test, public sigslot::has_slots<> {
   void OnChan2ReadyToSend() { ++transport2_ready_to_send_count_; }
 };
 
-TEST_F(SctpTransportTest, MessageInterleavedWithNotification) {
-  FakeDtlsTransport fake_dtls1("fake dtls 1", 0);
-  FakeDtlsTransport fake_dtls2("fake dtls 2", 0);
-  SctpFakeDataReceiver recv1;
-  SctpFakeDataReceiver recv2;
-  std::unique_ptr<SctpTransport> transport1(
-      CreateTransport(&fake_dtls1, &recv1));
-  std::unique_ptr<SctpTransport> transport2(
-      CreateTransport(&fake_dtls2, &recv2));
-
-  // Add a stream.
-  transport1->OpenStream(1);
-  transport2->OpenStream(1);
-
-  // Start SCTP transports.
-  transport1->Start(kSctpDefaultPort, kSctpDefaultPort, kSctpSendBufferSize);
-  transport2->Start(kSctpDefaultPort, kSctpDefaultPort, kSctpSendBufferSize);
-
-  // Connect the two fake DTLS transports.
-  fake_dtls1.SetDestination(&fake_dtls2, false);
-
-  // Ensure the SCTP association has been established
-  // Note: I'd rather watch for an assoc established state here but couldn't
-  //       find any exposed...
-  SendDataResult result;
-  ASSERT_TRUE(SendData(transport2.get(), 1, "meow", &result));
-  EXPECT_TRUE_WAIT(ReceivedData(&recv1, 1, "meow"), kDefaultTimeout);
-
-  // Detach the DTLS transport to ensure only we will inject packets from here
-  // on.
-  transport1->SetDtlsTransport(nullptr);
-
-  // Prepare chunk buffer and metadata
-  auto chunk = rtc::CopyOnWriteBuffer(32);
-  struct sctp_rcvinfo meta = {0};
-  meta.rcv_sid = 1;
-  meta.rcv_ssn = 1337;
-  meta.rcv_ppid = rtc::HostToNetwork32(51);  // text (complete)
-
-  // Inject chunk 1/2.
-  meta.rcv_tsn = 42;
-  meta.rcv_cumtsn = 42;
-  chunk.SetData("meow?", 5);
-  EXPECT_EQ(1, transport1->InjectDataOrNotificationFromSctpForTesting(
-                   chunk.data(), chunk.size(), meta, 0));
-
-  // Inject a notification in between chunks.
-  union sctp_notification notification;
-  memset(&notification, 0, sizeof(notification));
-  // Type chosen since it's not handled apart from being logged
-  notification.sn_header.sn_type = SCTP_PEER_ADDR_CHANGE;
-  notification.sn_header.sn_flags = 0;
-  notification.sn_header.sn_length = sizeof(notification);
-  EXPECT_EQ(1, transport1->InjectDataOrNotificationFromSctpForTesting(
-                   &notification, sizeof(notification), {0}, MSG_NOTIFICATION));
-
-  // Inject chunk 2/2
-  meta.rcv_tsn = 42;
-  meta.rcv_cumtsn = 43;
-  chunk.SetData(" rawr!", 6);
-  EXPECT_EQ(1, transport1->InjectDataOrNotificationFromSctpForTesting(
-                   chunk.data(), chunk.size(), meta, MSG_EOR));
-
-  // Expect the message to contain both chunks.
-  EXPECT_TRUE_WAIT(ReceivedData(&recv1, 1, "meow? rawr!"), kDefaultTimeout);
-}
-
 // Test that data can be sent end-to-end when an SCTP transport starts with one
 // transport (which is unwritable), and then switches to another transport. A
 // common scenario due to how BUNDLE works.