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
--- a/dom/media/tests/mochitest/mochitest.ini
+++ b/dom/media/tests/mochitest/mochitest.ini
@@ -191,16 +191,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