Revert security cherry pick #5

jim-signal · 2020-04-29T20:17:17Z

Reverting the usrsctp dependency since it now requires a iOS 10.1 target.

Unit test added to verify root cause is fixed. Scenario test added to verify high-level behavior. (cherry picked from commit 576db1b) Bug: webrtc:11654, chromium:1092476 Change-Id: I1ad6e2750f5272e86b4198749edbbf5dfd8315c4 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176564 Commit-Queue: Erik Språng <sprang@webrtc.org> Reviewed-by: Erik Språng <sprang@webrtc.org> Reviewed-by: Sebastian Jansson <srte@webrtc.org> Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org> Cr-Original-Commit-Position: refs/heads/master@{#31462} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176840 Cr-Commit-Position: refs/branch-heads/4147@{#5} Cr-Branched-From: 2b7d969-refs/heads/master@{#31262}

… large temporal idx value on the wire // Because ios_sim_x64_dbg_ios10 is broken on release brunch: // https://bugs.chromium.org/p/webrtc/issues/detail?id=11277 No-Try: true Bug: chromium:1042933 Change-Id: Ide37812a73b72e744f45b671918dc9817775e1f4 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/166521 Commit-Queue: Ilya Nikolaevskiy <ilnik@webrtc.org> Reviewed-by: Sergey Silkin <ssilkin@webrtc.org> Cr-Commit-Position: refs/branch-heads/3987@{#5} Cr-Branched-From: 1256d9b-refs/heads/master@{#30022}

…solution If e.g. CPU adaptation reduces input video size too much, video pipeline would reduce the number of used simulcast streams/spatial layers. This may result in disabled video if some streams are disabled by Rtp encoding parameters API. (cherry picked from commit 03d9096) Bug: webrtc:11319, chromium:1052313 Change-Id: Id7f157255599dcb6f494129b83477cda4bea982a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/168480 Reviewed-by: Evan Shrubsole <eshr@google.com> Commit-Queue: Ilya Nikolaevskiy <ilnik@webrtc.org> Cr-Original-Commit-Position: refs/heads/master@{#30498} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/168600 Cr-Commit-Position: refs/branch-heads/4044@{#5} Cr-Branched-From: be99ee8-refs/heads/master@{#30432}

This functionally no-op change adds the methods to allow an active ICE controller to manipulate the connection used by the ICE transport. Most methods reuse existing code, this will be explicitly marked for cleanup with a follow-up CL which adds active ICE controller support. Non-trivial changes are needed for P2PTransportChannel unit tests to cover the new code, and these are also being added in a follow-up CL. Bug: webrtc:14367, webrtc:14131 Change-Id: I4f012efcd8cb5766eb8c6f0872de50f8375f3a73 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/275301 Reviewed-by: Jonas Oreland <jonaso@webrtc.org> Commit-Queue: Sameer Vijaykar <samvi@google.com> Cr-Commit-Position: refs/heads/main@{#38081}

This check was added here: https://webrtc-review.googlesource.com/c/src/+/300544 When createOffer is used before createAnswer, this check would cause SetupDataChannelTransport_n to not be called for the remote channel. (cherry picked from commit 299cdc9) Bug: webrtc:15258, chromium:1458937 Change-Id: Ifdab35d1b0260ff03fef4beff13acf8090d59d8f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/310460 Reviewed-by: Harald Alvestrand <hta@webrtc.org> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org> Cr-Original-Commit-Position: refs/heads/main@{#40357} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/312123 Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org> Cr-Commit-Position: refs/branch-heads/5845@{#5} Cr-Branched-From: f80cf81-refs/heads/main@{#40319}

This was a fun bug which proved to be challenging to find a good solution for. The issue comes from the combination of partial reliability and stream resetting, which are covered in different RFCs, and where they don't refer to each other... Stream resetting (RFC 6525) is used in WebRTC for closing a Data Channel, and is done by signaling to the receiver that the stream sequence number (SSN) should be set to zero (0) at some time. Partial reliability (RFC 3758) - and expiring messages that will not be retransmitted - is done by signaling that the SSN should be set to a certain value at a certain TSN, as the messages up until the provided SSN are not to be expected to be sent again. As these two functionalities both work by signaling to the receiver what the next expected SSN should be, they need to do it correctly not to overwrite each others' intent. And here was the bug. An example scenario where this caused issues, where we are Z (the receiver), getting packets from the sender (A): 5 A->Z DATA (TSN=30, B, SID=2, SSN=0) 6 Z->A SACK (Ack=30) 7 A->Z DATA (TSN=31, E, SID=2, SSN=0) 8 A->Z RE_CONFIG (REQ=30, TSN=31, SID=2) 9 Z->A RE_CONFIG (RESP=30, Performed) 10 Z->A SACK (Ack=31) 11 A->Z DATA (TSN=32, SID=1) 12 A->Z FORWARD_TSN (TSN=32, SID=2, SSN=0) Let's assume that the path Z->A had packet loss and A never really received our responses (#6, #9, #10) in time. At #5, Z receives a DATA fragment, which it acks, and at #7 the end of that message. The stream is then reset (#8) which it signals that it was performed (#9) and acked (#10), and data on another stream (2) was received (#11). Since A hasn't received any ACKS yet, and those chunks on SID=2 all expired, A sends a FORWARD-TSN saying that "Skip to TSN=32, and don't expect SID=2, SSN=0". That makes the receiver expect the SSN on SID=2 to be SSN=1 next time at TSN=32. But that's not good at all - A reset the stream at #8 and will want to send the next message on SID=2 using SSN=0 - not 1. The FORWARD-TSN clearly can't have a TSN that is beyond the stream reset TSN for that stream. This is just one example - combining stream resetting and partial reliability, together with a lossy network, and different variants of this can occur, which results in the receiver possibly not delivering packets because it expects a different SSN than the one the sender is later using. So this CL adds "breakpoints" to how far a FORWARD-TSN can stretch. It will simply not cross any Stream Reset last assigned TSNs, and only when a receiver has acked that all TSNs up till the Stream Reset last assigned TSN has been received, it will proceed expiring chunks after that. Bug: webrtc:14600 Change-Id: Ibae8c9308f5dfe8d734377d42cce653e69e95731 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/321600 Commit-Queue: Victor Boivie <boivie@webrtc.org> Reviewed-by: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#40829}

The new version of MSan (rolled by [1]) detects the following: ``` ==39908==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x5591400a52ef in GetPlayoutDelayMs ./../../modules/audio_coding/neteq/decision_logic.cc:466:35 #1 0x5591400a52ef in webrtc::DecisionLogic::ExpectedPacketAvailable(webrtc::NetEqController::NetEqStatus) ./../../modules/audio_coding/neteq/decision_logic.cc:311:36 #2 0x5591400a39e9 in webrtc::DecisionLogic::GetDecision(webrtc::NetEqController::NetEqStatus const&, bool*) ./../../modules/audio_coding/neteq/decision_logic.cc:0:0 #3 0x55913cf590c9 in webrtc::DecisionLogicTest_PreemptiveExpand_Test::TestBody() ./../../modules/audio_coding/neteq/decision_logic_unittest.cc:139:3 #4 0x55913ef28283 in HandleExceptionsInMethodIfSupported<testing::Test, void> ./../../third_party/googletest/src/googletest/src/gtest.cc:0:3 #5 0x55913ef28283 in testing::Test::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2710:5 #6 0x55913ef2ab46 in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2856:11 #7 0x55913ef2da34 in testing::TestSuite::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:3034:30 #8 0x55913ef621e8 in testing::internal::UnitTestImpl::RunAllTests() ./../../third_party/googletest/src/googletest/src/gtest.cc:5964:44 #9 0x55913ef60f54 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0 #10 0x55913ef60f54 in testing::UnitTest::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:5543:10 #11 0x55913ee1a944 in RUN_ALL_TESTS ./../../third_party/googletest/src/googletest/include/gtest/gtest.h:2334:73 #12 0x55913ee1a944 in webrtc::(anonymous namespace)::TestMainImpl::Run(int, char**) ./../../test/test_main_lib.cc:203:21 #13 0x55913cbd36b8 in main ./../../test/test_main.cc:72:16 #14 0x7fdb18c73082 in __libc_start_main /build/glibc-LcI20x/glibc-2.31/csu/../csu/libc-start.c:308:16 #15 0x55913cb3e1a9 in _start ??:0:0 ``` [1] - https://webrtc-review.googlesource.com/c/src/+/353620 Bug: b/344970813 Change-Id: I9b5d7791e68b4c494168ba9f007a3099ae21fed4 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/353581 Auto-Submit: Mirko Bonadei <mbonadei@webrtc.org> Reviewed-by: Jakob Ivarsson‎ <jakobi@webrtc.org> Commit-Queue: Jakob Ivarsson‎ <jakobi@webrtc.org> Cr-Commit-Position: refs/heads/main@{#42433}

This commit fixes the issue of H26xPacketBuffer not supporting the RTP padding packet. (cherry picked from commit 6f866347ff0c0d1544938974ee8f66e722dffc15) Bug: webrtc:383841353, chromium:386311884 Change-Id: Ibd87cd9c18577d990fa56a2fdfed1552d33b58a2 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/371840 Reviewed-by: Sergey Silkin <ssilkin@webrtc.org> Reviewed-by: Erik Språng <sprang@webrtc.org> Commit-Queue: Sergey Silkin <ssilkin@webrtc.org> Cr-Original-Commit-Position: refs/heads/main@{#43586} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/372780 Reviewed-by: Danil Chapovalov <danilchap@webrtc.org> Commit-Queue: Danil Chapovalov <danilchap@webrtc.org> Cr-Commit-Position: refs/branch-heads/6834@{#5} Cr-Branched-From: a5d7100-refs/heads/main@{#43387}

Revert security cherry pick

6e9ca93

Reverting the usrsctp dependency since it now requires a iOS 10.1 target.

jim-signal requested a review from peter-signal April 29, 2020 20:17

peter-signal merged commit 402f9f2 into signalapp:master Apr 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert security cherry pick #5

Revert security cherry pick #5

jim-signal commented Apr 29, 2020

Revert security cherry pick #5

Revert security cherry pick #5

Conversation

jim-signal commented Apr 29, 2020