Bug 1413709 - add tests to detect improper ice restart by answer. r=bwc
authorMichael Froman <mfroman@mozilla.com>
Wed, 01 Nov 2017 11:56:35 -0500
changeset 389932 6f3b7e0969c97c5c085b52272cccfc9bc3e9c37d
parent 389931 399dbcf3b66e3c4c71127635b5fe6c46dc340018
child 389933 43e56a29736611ceadaa613d2be8168b12f7c45f
push id32802
push usernbeleuzu@mozilla.com
push dateFri, 03 Nov 2017 09:59:33 +0000
treeherdermozilla-central@e6d86b7284ba [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbwc
bugs1413709, 1405940
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1413709 - add tests to detect improper ice restart by answer. r=bwc Adding tests that would have shown the issue fixed in Bug 1405940. If the answer during a renegotiation has modified ICE credentials, it should cause an error. These tests check for that error. MozReview-Commit-ID: 9u8GGpslDdK
dom/media/tests/mochitest/mochitest.ini
dom/media/tests/mochitest/test_peerConnection_restartIceBadAnswer.html
media/webrtc/signaling/gtest/jsep_session_unittest.cpp
--- a/dom/media/tests/mochitest/mochitest.ini
+++ b/dom/media/tests/mochitest/mochitest.ini
@@ -192,16 +192,18 @@ skip-if = toolkit == 'android'
 [test_peerConnection_restartIceNoBundleNoRtcpMux.html]
 skip-if = toolkit == 'android'
 [test_peerConnection_restartIceNoRtcpMux.html]
 skip-if = toolkit == 'android'
 [test_peerConnection_restartIceLocalRollback.html]
 skip-if = toolkit == 'android'
 [test_peerConnection_restartIceLocalAndRemoteRollback.html]
 skip-if = toolkit == 'android'
+[test_peerConnection_restartIceBadAnswer.html]
+skip-if = toolkit == 'android'
 [test_peerConnection_scaleResolution.html]
 skip-if = (android_version == '18') # android(Bug 1189784, timeouts on 4.3 emulator)
 [test_peerConnection_simulcastOffer.html]
 skip-if = android_version # no simulcast support on android
 [test_peerConnection_simulcastAnswer.html]
 skip-if = android_version # no simulcast support on android
 [test_peerConnection_relayOnly.html]
 disabled=
new file mode 100644
--- /dev/null
+++ b/dom/media/tests/mochitest/test_peerConnection_restartIceBadAnswer.html
@@ -0,0 +1,58 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <script type="application/javascript" src="pc.js"></script>
+</head>
+<body>
+<pre id="test">
+<script type="application/javascript">
+  createHTML({
+    bug: "1413709",
+    title: "Renegotiation: bad answer ICE credentials"
+  });
+
+  var test;
+  runNetworkTest(function (options) {
+    test = new PeerConnectionTest(options);
+
+    addRenegotiation(test.chain,
+      [
+        function PC_LOCAL_ADD_SECOND_STREAM(test) {
+          test.setMediaConstraints([{audio: true}],
+                                   []);
+          return test.pcLocal.getAllUserMedia([{audio: true}]);
+        },
+      ]
+    );
+
+    // If the offerer hasn't indicated ICE restart, then an answer
+    // arriving during renegotiation that has modified ICE credentials
+    // should cause an error
+    test.chain.replaceAfter("PC_LOCAL_GET_ANSWER",
+      [
+        function PC_LOCAL_REWRITE_REMOTE_SDP_ICE_CREDS(test) {
+          test._remote_answer.sdp =
+            test._remote_answer.sdp.replace(/a=ice-pwd:.*\r\n/g,
+                                            "a=ice-pwd:bad-pwd\r\n")
+                                   .replace(/a=ice-ufrag:.*\r\n/g,
+                                            "a=ice-ufrag:bad-ufrag\r\n");
+        },
+
+        function PC_LOCAL_EXPECT_SET_REMOTE_DESCRIPTION_FAIL(test) {
+          return test.setRemoteDescription(test.pcLocal,
+                                           test._remote_answer,
+                                           STABLE)
+           .then(() => ok(false, "setRemoteDescription must fail"),
+                 e => is(e.name, "InvalidSessionDescriptionError",
+                         "setRemoteDescription must fail and did"));
+         }
+      ], 1 // replace after the second PC_LOCAL_GET_ANSWER
+    );
+
+    test.setMediaConstraints([{audio: true}], []);
+    test.run();
+  });
+</script>
+</pre>
+</body>
+</html>
--- a/media/webrtc/signaling/gtest/jsep_session_unittest.cpp
+++ b/media/webrtc/signaling/gtest/jsep_session_unittest.cpp
@@ -78,36 +78,56 @@ public:
 
     mOffCandidates = MakeUnique<CandidateSet>();
     mAnsCandidates = MakeUnique<CandidateSet>();
   }
 protected:
   struct TransportData {
     std::string mIceUfrag;
     std::string mIcePwd;
+    int iceCredentialSerial;
     std::map<std::string, std::vector<uint8_t> > mFingerprints;
   };
 
   void
+  GenerateNewIceCredentials(const JsepSessionImpl& session,
+                            TransportData& tdata)
+  {
+    std::ostringstream ostr;
+    ostr << session.GetName() << "-" << ++tdata.iceCredentialSerial;
+
+    // Values here semi-borrowed from JSEP draft.
+    tdata.mIceUfrag = ostr.str() + "-ufrag";
+    tdata.mIcePwd = ostr.str() + "-1234567890";
+  }
+
+  void
+  ModifyOffererIceCredentials()
+  {
+    GenerateNewIceCredentials(*mSessionOff, *mOffererTransport);
+    mSessionOff->SetIceCredentials(mOffererTransport->mIceUfrag,
+                                   mOffererTransport->mIcePwd);
+  }
+
+  void
   AddDtlsFingerprint(const std::string& alg, JsepSessionImpl& session,
                      TransportData& tdata)
   {
     std::vector<uint8_t> fp;
     fp.assign((alg == "sha-1") ? 20 : 32,
               (session.GetName() == "Offerer") ? 0x4f : 0x41);
     session.AddDtlsFingerprint(alg, fp);
     tdata.mFingerprints[alg] = fp;
   }
 
   void
   AddTransportData(JsepSessionImpl& session, TransportData& tdata)
   {
-    // Values here semi-borrowed from JSEP draft.
-    tdata.mIceUfrag = session.GetName() + "-ufrag";
-    tdata.mIcePwd = session.GetName() + "-1234567890";
+    tdata.iceCredentialSerial = 0;
+    GenerateNewIceCredentials(session, tdata);
     session.SetIceCredentials(tdata.mIceUfrag, tdata.mIcePwd);
     AddDtlsFingerprint("sha-1", session, tdata);
     AddDtlsFingerprint("sha-256", session, tdata);
   }
 
   std::string
   CreateOffer(const Maybe<JsepOfferOptions>& options = Nothing())
   {
@@ -519,16 +539,24 @@ protected:
     }
   }
 
   std::string
   CreateAnswer()
   {
     JsepAnswerOptions options;
     std::string answer;
+
+    // detect ice restart and generate new ice credentials (like
+    // PeerConnectionImpl does).
+    if (mSessionAns->RemoteIceIsRestarting()) {
+      GenerateNewIceCredentials(*mSessionAns, *mAnswererTransport);
+      mSessionAns->SetIceCredentials(mAnswererTransport->mIceUfrag,
+                                     mAnswererTransport->mIcePwd);
+    }
     nsresult rv = mSessionAns->CreateAnswer(options, &answer);
     EXPECT_EQ(NS_OK, rv);
 
     std::cerr << "ANSWER: " << answer << std::endl;
 
     ValidateTransport(*mAnswererTransport, answer);
 
     return answer;
@@ -4098,16 +4126,103 @@ TEST_F(JsepSessionTest, TestIceOptions)
 
   ASSERT_EQ(1U, mSessionOff->GetIceOptions().size());
   ASSERT_EQ("trickle", mSessionOff->GetIceOptions()[0]);
 
   ASSERT_EQ(1U, mSessionAns->GetIceOptions().size());
   ASSERT_EQ("trickle", mSessionAns->GetIceOptions()[0]);
 }
 
+TEST_F(JsepSessionTest, TestIceRestart)
+{
+  AddTracks(*mSessionOff, "audio");
+  AddTracks(*mSessionAns, "audio");
+  std::string offer = CreateOffer();
+  SetLocalOffer(offer, CHECK_SUCCESS);
+  SetRemoteOffer(offer, CHECK_SUCCESS);
+  std::string answer = CreateAnswer();
+  SetLocalAnswer(answer, CHECK_SUCCESS);
+  SetRemoteAnswer(answer, CHECK_SUCCESS);
+
+  JsepOfferOptions options;
+  options.mIceRestart = Some(true);
+  ModifyOffererIceCredentials();
+
+  std::string reoffer = CreateOffer(Some(options));
+  SetLocalOffer(reoffer, CHECK_SUCCESS);
+  SetRemoteOffer(reoffer, CHECK_SUCCESS);
+  std::string reanswer = CreateAnswer();
+  SetLocalAnswer(reanswer, CHECK_SUCCESS);
+  SetRemoteAnswer(reanswer, CHECK_SUCCESS);
+
+  UniquePtr<Sdp> parsedOffer(Parse(offer));
+  ASSERT_EQ(1U, parsedOffer->GetMediaSectionCount());
+  UniquePtr<Sdp> parsedReoffer(Parse(reoffer));
+  ASSERT_EQ(1U, parsedReoffer->GetMediaSectionCount());
+
+  UniquePtr<Sdp> parsedAnswer(Parse(answer));
+  ASSERT_EQ(1U, parsedAnswer->GetMediaSectionCount());
+  UniquePtr<Sdp> parsedReanswer(Parse(reanswer));
+  ASSERT_EQ(1U, parsedReanswer->GetMediaSectionCount());
+
+  // verify ice pwd/ufrag are present in offer/answer and reoffer/reanswer
+  auto& offerMediaAttrs = parsedOffer->GetMediaSection(0).GetAttributeList();
+  ASSERT_TRUE(offerMediaAttrs.HasAttribute(SdpAttribute::kIcePwdAttribute));
+  ASSERT_TRUE(offerMediaAttrs.HasAttribute(SdpAttribute::kIceUfragAttribute));
+
+  auto& reofferMediaAttrs = parsedReoffer->GetMediaSection(0).GetAttributeList();
+  ASSERT_TRUE(reofferMediaAttrs.HasAttribute(SdpAttribute::kIcePwdAttribute));
+  ASSERT_TRUE(reofferMediaAttrs.HasAttribute(SdpAttribute::kIceUfragAttribute));
+
+  auto& answerMediaAttrs = parsedAnswer->GetMediaSection(0).GetAttributeList();
+  ASSERT_TRUE(answerMediaAttrs.HasAttribute(SdpAttribute::kIcePwdAttribute));
+  ASSERT_TRUE(answerMediaAttrs.HasAttribute(SdpAttribute::kIceUfragAttribute));
+
+  auto& reanswerMediaAttrs = parsedReanswer->GetMediaSection(0).GetAttributeList();
+  ASSERT_TRUE(reanswerMediaAttrs.HasAttribute(SdpAttribute::kIcePwdAttribute));
+  ASSERT_TRUE(reanswerMediaAttrs.HasAttribute(SdpAttribute::kIceUfragAttribute));
+
+  // make sure offer/reoffer ice pwd/ufrag changed on ice restart
+  ASSERT_NE(offerMediaAttrs.GetIcePwd().c_str(),
+            reofferMediaAttrs.GetIcePwd().c_str());
+  ASSERT_NE(offerMediaAttrs.GetIceUfrag().c_str(),
+            reofferMediaAttrs.GetIceUfrag().c_str());
+
+  // make sure answer/reanswer ice pwd/ufrag changed on ice restart
+  ASSERT_NE(answerMediaAttrs.GetIcePwd().c_str(),
+            reanswerMediaAttrs.GetIcePwd().c_str());
+  ASSERT_NE(answerMediaAttrs.GetIceUfrag().c_str(),
+            reanswerMediaAttrs.GetIceUfrag().c_str());
+}
+
+TEST_F(JsepSessionTest, TestAnswererIndicatingIceRestart)
+{
+  AddTracks(*mSessionOff, "audio");
+  AddTracks(*mSessionAns, "audio");
+  std::string offer = CreateOffer();
+  SetLocalOffer(offer, CHECK_SUCCESS);
+  SetRemoteOffer(offer, CHECK_SUCCESS);
+  std::string answer = CreateAnswer();
+  SetLocalAnswer(answer, CHECK_SUCCESS);
+  SetRemoteAnswer(answer, CHECK_SUCCESS);
+
+  // reoffer, but we'll improperly indicate an ice restart in the answer by
+  // modifying the ice pwd and ufrag
+  std::string reoffer = CreateOffer();
+  SetLocalOffer(reoffer, CHECK_SUCCESS);
+  SetRemoteOffer(reoffer, CHECK_SUCCESS);
+  std::string reanswer = CreateAnswer();
+
+  // change the ice pwd and ufrag
+  ReplaceInSdp(&reanswer, "Answerer-1-", "bad-2-");
+  SetLocalAnswer(reanswer, CHECK_SUCCESS);
+  nsresult rv = mSessionOff->SetRemoteDescription(kJsepSdpAnswer, reanswer);
+  ASSERT_NE(NS_OK, rv); // NS_ERROR_INVALID_ARG
+}
+
 TEST_F(JsepSessionTest, TestExtmap)
 {
   AddTracks(*mSessionOff, "audio");
   AddTracks(*mSessionAns, "audio");
   // ssrc-audio-level will be extmap 1 for both
   mSessionOff->AddAudioRtpExtension("foo"); // Default mapping of 3
   mSessionOff->AddAudioRtpExtension("bar"); // Default mapping of 4
   mSessionAns->AddAudioRtpExtension("bar"); // Default mapping of 4