From 2e7ee4b28bbdf92bdf804b600ae33679d1799788 Mon Sep 17 00:00:00 2001 From: "pthatcher@webrtc.org" Date: Mon, 27 Oct 2014 16:10:29 +0000 Subject: [PATCH] Fix the SrtpFilter crash caused by two local offers. BUG=http://crbug.com/421774 R=juberti@webrtc.org Review URL: https://webrtc-codereview.appspot.com/30789004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@7530 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/session/media/srtpfilter.cc | 15 ++++++--- talk/session/media/srtpfilter_unittest.cc | 40 +++++++++++++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/talk/session/media/srtpfilter.cc b/talk/session/media/srtpfilter.cc index c79bf1323a..5a774db794 100644 --- a/talk/session/media/srtpfilter.cc +++ b/talk/session/media/srtpfilter.cc @@ -150,7 +150,7 @@ bool SrtpFilter::SetRtpParams(const std::string& send_cs, const uint8* send_key, int send_key_len, const std::string& recv_cs, const uint8* recv_key, int recv_key_len) { - if (state_ == ST_ACTIVE) { + if (IsActive()) { LOG(LS_ERROR) << "Tried to set SRTP Params when filter already active"; return false; } @@ -212,6 +212,7 @@ bool SrtpFilter::ProtectRtp(void* p, int in_len, int max_len, int* out_len) { LOG(LS_WARNING) << "Failed to ProtectRtp: SRTP not active"; return false; } + ASSERT(send_session_ != NULL); return send_session_->ProtectRtp(p, in_len, max_len, out_len); } @@ -221,7 +222,7 @@ bool SrtpFilter::ProtectRtp(void* p, int in_len, int max_len, int* out_len, LOG(LS_WARNING) << "Failed to ProtectRtp: SRTP not active"; return false; } - + ASSERT(send_session_ != NULL); return send_session_->ProtectRtp(p, in_len, max_len, out_len, index); } @@ -233,6 +234,7 @@ bool SrtpFilter::ProtectRtcp(void* p, int in_len, int max_len, int* out_len) { if (send_rtcp_session_) { return send_rtcp_session_->ProtectRtcp(p, in_len, max_len, out_len); } else { + ASSERT(send_session_ != NULL); return send_session_->ProtectRtcp(p, in_len, max_len, out_len); } } @@ -242,6 +244,7 @@ bool SrtpFilter::UnprotectRtp(void* p, int in_len, int* out_len) { LOG(LS_WARNING) << "Failed to UnprotectRtp: SRTP not active"; return false; } + ASSERT(recv_session_ != NULL); return recv_session_->UnprotectRtp(p, in_len, out_len); } @@ -253,6 +256,7 @@ bool SrtpFilter::UnprotectRtcp(void* p, int in_len, int* out_len) { if (recv_rtcp_session_) { return recv_rtcp_session_->UnprotectRtcp(p, in_len, out_len); } else { + ASSERT(recv_session_ != NULL); return recv_session_->UnprotectRtcp(p, in_len, out_len); } } @@ -263,13 +267,16 @@ bool SrtpFilter::GetRtpAuthParams(uint8** key, int* key_len, int* tag_len) { return false; } + ASSERT(send_session_ != NULL); return send_session_->GetRtpAuthParams(key, key_len, tag_len); } void SrtpFilter::set_signal_silent_time(uint32 signal_silent_time_in_ms) { signal_silent_time_in_ms_ = signal_silent_time_in_ms; - if (state_ == ST_ACTIVE) { + if (IsActive()) { + ASSERT(send_session_ != NULL); send_session_->set_signal_silent_time(signal_silent_time_in_ms); + ASSERT(recv_session_ != NULL); recv_session_->set_signal_silent_time(signal_silent_time_in_ms); if (send_rtcp_session_) send_rtcp_session_->set_signal_silent_time(signal_silent_time_in_ms); @@ -292,7 +299,7 @@ bool SrtpFilter::StoreParams(const std::vector& params, offer_params_ = params; if (state_ == ST_INIT) { state_ = (source == CS_LOCAL) ? ST_SENTOFFER : ST_RECEIVEDOFFER; - } else { // state >= ST_ACTIVE + } else if (state_ == ST_ACTIVE) { state_ = (source == CS_LOCAL) ? ST_SENTUPDATEDOFFER : ST_RECEIVEDUPDATEDOFFER; } diff --git a/talk/session/media/srtpfilter_unittest.cc b/talk/session/media/srtpfilter_unittest.cc index 19efa2a7b2..8b859f88f2 100644 --- a/talk/session/media/srtpfilter_unittest.cc +++ b/talk/session/media/srtpfilter_unittest.cc @@ -82,9 +82,12 @@ class SrtpFilterTest : public testing::Test { const std::vector& params2) { EXPECT_TRUE(f1_.SetOffer(params1, CS_LOCAL)); EXPECT_TRUE(f2_.SetOffer(params1, CS_REMOTE)); + EXPECT_FALSE(f1_.IsActive()); + EXPECT_FALSE(f2_.IsActive()); EXPECT_TRUE(f2_.SetAnswer(params2, CS_LOCAL)); EXPECT_TRUE(f1_.SetAnswer(params2, CS_REMOTE)); EXPECT_TRUE(f1_.IsActive()); + EXPECT_TRUE(f2_.IsActive()); } void TestProtectUnprotect(const std::string& cs1, const std::string& cs2) { char rtp_packet[sizeof(kPcmuFrame) + 10]; @@ -139,6 +142,7 @@ class SrtpFilterTest : public testing::Test { // Test that we can set up the session and keys properly. TEST_F(SrtpFilterTest, TestGoodSetupOneCipherSuite) { EXPECT_TRUE(f1_.SetOffer(MakeVector(kTestCryptoParams1), CS_LOCAL)); + EXPECT_FALSE(f1_.IsActive()); EXPECT_TRUE(f1_.SetAnswer(MakeVector(kTestCryptoParams2), CS_REMOTE)); EXPECT_TRUE(f1_.IsActive()); } @@ -153,6 +157,7 @@ TEST_F(SrtpFilterTest, TestGoodSetupMultipleCipherSuites) { answer[0].tag = 2; answer[0].cipher_suite = CS_AES_CM_128_HMAC_SHA1_32; EXPECT_TRUE(f1_.SetOffer(offer, CS_LOCAL)); + EXPECT_FALSE(f1_.IsActive()); EXPECT_TRUE(f1_.SetAnswer(answer, CS_REMOTE)); EXPECT_TRUE(f1_.IsActive()); } @@ -188,6 +193,7 @@ TEST_F(SrtpFilterTest, TestBadSetup) { TEST_F(SrtpFilterTest, TestGoodSetupMultipleOffers) { EXPECT_TRUE(f1_.SetOffer(MakeVector(kTestCryptoParams1), CS_LOCAL)); EXPECT_TRUE(f1_.SetOffer(MakeVector(kTestCryptoParams2), CS_LOCAL)); + EXPECT_FALSE(f1_.IsActive()); EXPECT_TRUE(f1_.SetAnswer(MakeVector(kTestCryptoParams2), CS_REMOTE)); EXPECT_TRUE(f1_.IsActive()); EXPECT_TRUE(f1_.SetOffer(MakeVector(kTestCryptoParams1), CS_LOCAL)); @@ -196,6 +202,7 @@ TEST_F(SrtpFilterTest, TestGoodSetupMultipleOffers) { EXPECT_TRUE(f2_.SetOffer(MakeVector(kTestCryptoParams1), CS_REMOTE)); EXPECT_TRUE(f2_.SetOffer(MakeVector(kTestCryptoParams2), CS_REMOTE)); + EXPECT_FALSE(f2_.IsActive()); EXPECT_TRUE(f2_.SetAnswer(MakeVector(kTestCryptoParams2), CS_LOCAL)); EXPECT_TRUE(f2_.IsActive()); EXPECT_TRUE(f2_.SetOffer(MakeVector(kTestCryptoParams1), CS_REMOTE)); @@ -206,6 +213,7 @@ TEST_F(SrtpFilterTest, TestGoodSetupMultipleOffers) { TEST_F(SrtpFilterTest, TestBadSetupMultipleOffers) { EXPECT_TRUE(f1_.SetOffer(MakeVector(kTestCryptoParams1), CS_LOCAL)); EXPECT_FALSE(f1_.SetOffer(MakeVector(kTestCryptoParams2), CS_REMOTE)); + EXPECT_FALSE(f1_.IsActive()); EXPECT_TRUE(f1_.SetAnswer(MakeVector(kTestCryptoParams1), CS_REMOTE)); EXPECT_TRUE(f1_.IsActive()); EXPECT_TRUE(f1_.SetOffer(MakeVector(kTestCryptoParams2), CS_LOCAL)); @@ -214,6 +222,7 @@ TEST_F(SrtpFilterTest, TestBadSetupMultipleOffers) { EXPECT_TRUE(f2_.SetOffer(MakeVector(kTestCryptoParams2), CS_REMOTE)); EXPECT_FALSE(f2_.SetOffer(MakeVector(kTestCryptoParams1), CS_LOCAL)); + EXPECT_FALSE(f2_.IsActive()); EXPECT_TRUE(f2_.SetAnswer(MakeVector(kTestCryptoParams2), CS_LOCAL)); EXPECT_TRUE(f2_.IsActive()); EXPECT_TRUE(f2_.SetOffer(MakeVector(kTestCryptoParams2), CS_REMOTE)); @@ -402,6 +411,8 @@ TEST_F(SrtpFilterTest, TestProvisionalAnswer) { EXPECT_TRUE(f1_.SetOffer(offer, CS_LOCAL)); EXPECT_TRUE(f2_.SetOffer(offer, CS_REMOTE)); + EXPECT_FALSE(f1_.IsActive()); + EXPECT_FALSE(f2_.IsActive()); EXPECT_TRUE(f2_.SetProvisionalAnswer(answer, CS_LOCAL)); EXPECT_TRUE(f1_.SetProvisionalAnswer(answer, CS_REMOTE)); EXPECT_TRUE(f1_.IsActive()); @@ -425,6 +436,8 @@ TEST_F(SrtpFilterTest, TestProvisionalAnswerWithoutCrypto) { EXPECT_TRUE(f1_.SetOffer(offer, CS_LOCAL)); EXPECT_TRUE(f2_.SetOffer(offer, CS_REMOTE)); + EXPECT_FALSE(f1_.IsActive()); + EXPECT_FALSE(f2_.IsActive()); EXPECT_TRUE(f2_.SetProvisionalAnswer(answer, CS_LOCAL)); EXPECT_TRUE(f1_.SetProvisionalAnswer(answer, CS_REMOTE)); EXPECT_FALSE(f1_.IsActive()); @@ -438,6 +451,33 @@ TEST_F(SrtpFilterTest, TestProvisionalAnswerWithoutCrypto) { TestProtectUnprotect(CS_AES_CM_128_HMAC_SHA1_80, CS_AES_CM_128_HMAC_SHA1_80); } +// Test that if we get a new local offer after a provisional answer +// with no crypto, that we are in an inactive state. +TEST_F(SrtpFilterTest, TestLocalOfferAfterProvisionalAnswerWithoutCrypto) { + std::vector offer(MakeVector(kTestCryptoParams1)); + std::vector answer; + + EXPECT_TRUE(f1_.SetOffer(offer, CS_LOCAL)); + EXPECT_TRUE(f2_.SetOffer(offer, CS_REMOTE)); + EXPECT_TRUE(f1_.SetProvisionalAnswer(answer, CS_REMOTE)); + EXPECT_TRUE(f2_.SetProvisionalAnswer(answer, CS_LOCAL)); + EXPECT_FALSE(f1_.IsActive()); + EXPECT_FALSE(f2_.IsActive()); + // The calls to set an offer after a provisional answer fail, so the + // state doesn't change. + EXPECT_FALSE(f1_.SetOffer(offer, CS_LOCAL)); + EXPECT_FALSE(f2_.SetOffer(offer, CS_REMOTE)); + EXPECT_FALSE(f1_.IsActive()); + EXPECT_FALSE(f2_.IsActive()); + + answer.push_back(kTestCryptoParams2); + EXPECT_TRUE(f2_.SetAnswer(answer, CS_LOCAL)); + EXPECT_TRUE(f1_.SetAnswer(answer, CS_REMOTE)); + EXPECT_TRUE(f1_.IsActive()); + EXPECT_TRUE(f2_.IsActive()); + TestProtectUnprotect(CS_AES_CM_128_HMAC_SHA1_80, CS_AES_CM_128_HMAC_SHA1_80); +} + // Test that we can disable encryption. TEST_F(SrtpFilterTest, TestDisableEncryption) { std::vector offer(MakeVector(kTestCryptoParams1));