From 265d9748d442388c671967dbe8da8fa7161d90d6 Mon Sep 17 00:00:00 2001
From: Tristan Matthews <tristan.matthews@savoirfairelinux.com>
Date: Mon, 2 Apr 2012 15:02:11 -0400
Subject: [PATCH] * #9641: audiortp: Fixed CryptoContext management

They are cached in AudioRtpFactory, but may be replaced in
AudioSrtpSession. Therefore, whenever we reinitialize these contexts
we must update the cached values in audiortpfactory. In addition, since
they will outlive a given AudioSrtpSession, they must NOT be deleted in
AudioSrtpSession's destructor but rather in AudioRtpFactory's.
---
 .../src/audio/audiortp/audio_rtp_factory.cpp  | 32 ++++----
 daemon/src/audio/audiortp/audio_rtp_factory.h |  4 +-
 .../src/audio/audiortp/audio_srtp_session.cpp | 82 +++++++++----------
 .../src/audio/audiortp/audio_srtp_session.h   | 18 ++--
 4 files changed, 64 insertions(+), 72 deletions(-)

diff --git a/daemon/src/audio/audiortp/audio_rtp_factory.cpp b/daemon/src/audio/audiortp/audio_rtp_factory.cpp
index a153e47f0c..2170b06e52 100644
--- a/daemon/src/audio/audiortp/audio_rtp_factory.cpp
+++ b/daemon/src/audio/audiortp/audio_rtp_factory.cpp
@@ -44,13 +44,15 @@ namespace sfl {
 
 AudioRtpFactory::AudioRtpFactory(SIPCall *ca) : rtpSession_(NULL),
     audioRtpThreadMutex_(), srtpEnabled_(false), helloHashEnabled_(false),
-    remoteContext_(NULL), localContext_(NULL), ca_(ca),
+    cachedRemoteContext_(0), cachedLocalContext_(0), ca_(ca),
     keyExchangeProtocol_(NONE)
 {}
 
 AudioRtpFactory::~AudioRtpFactory()
 {
     delete rtpSession_;
+    delete cachedLocalContext_;
+    delete cachedRemoteContext_;
 }
 
 void AudioRtpFactory::initConfig()
@@ -86,7 +88,7 @@ void AudioRtpFactory::initSession()
     ost::MutexLock m(audioRtpThreadMutex_);
 
     if (srtpEnabled_) {
-        std::string zidFilename(Manager::instance().voipPreferences.getZidFile());
+        const std::string zidFilename(Manager::instance().voipPreferences.getZidFile());
 
         switch (keyExchangeProtocol_) {
 
@@ -114,8 +116,8 @@ void AudioRtpFactory::start(AudioCodec* audiocodec)
     if (rtpSession_ == NULL)
         throw AudioRtpFactoryException("AudioRtpFactory: Error: RTP session was null when trying to start audio thread");
 
-    if (keyExchangeProtocol_ == SDES and localContext_ and remoteContext_)
-        static_cast<AudioSrtpSession *>(rtpSession_)->restoreCryptoContext(localContext_, remoteContext_);
+    if (keyExchangeProtocol_ == SDES and cachedLocalContext_ and cachedRemoteContext_)
+        static_cast<AudioSrtpSession *>(rtpSession_)->restoreCryptoContext(cachedLocalContext_, cachedRemoteContext_);
 
     if (rtpSession_->startRtpThread(*audiocodec) != 0)
         throw AudioRtpFactoryException("AudioRtpFactory: Error: Failed to start AudioRtpSession thread");
@@ -125,14 +127,6 @@ void AudioRtpFactory::stop()
 {
     ost::MutexLock mutex(audioRtpThreadMutex_);
 
-    if (rtpSession_ == NULL)
-        return;
-
-    if (keyExchangeProtocol_ == SDES) {
-        localContext_ = static_cast<AudioSrtpSession*>(rtpSession_)->localCryptoCtx_;
-        remoteContext_ = static_cast<AudioSrtpSession*>(rtpSession_)->remoteCryptoCtx_;
-    }
-
     delete rtpSession_;
     rtpSession_ = NULL;
 }
@@ -170,17 +164,19 @@ sfl::AudioZrtpSession * AudioRtpFactory::getAudioZrtpSession()
 void sfl::AudioRtpFactory::initLocalCryptoInfo()
 {
     if (rtpSession_ && keyExchangeProtocol_ == SDES) {
-        AudioSrtpSession *srtpSession = static_cast<AudioSrtpSession*>(rtpSession_);
-        srtpSession->initLocalCryptoInfo();
-        ca_->getLocalSDP()->setLocalSdpCrypto(srtpSession->getLocalCryptoInfo());
+        AudioSrtpSession *srtp = static_cast<AudioSrtpSession*>(rtpSession_);
+        // the context is invalidated and deleted by the call to initLocalCryptoInfo
+        cachedLocalContext_ = srtp->initLocalCryptoInfo();
+        ca_->getLocalSDP()->setLocalSdpCrypto(srtp->getLocalCryptoInfo());
     }
 }
 
 void AudioRtpFactory::setRemoteCryptoInfo(sfl::SdesNegotiator& nego)
 {
-    if (rtpSession_ and keyExchangeProtocol_ == SDES)
-        static_cast<AudioSrtpSession *>(rtpSession_)->setRemoteCryptoInfo(nego);
-    else
+    if (rtpSession_ and keyExchangeProtocol_ == SDES) {
+        AudioSrtpSession *srtp = static_cast<AudioSrtpSession *>(rtpSession_);
+        cachedRemoteContext_ = srtp->setRemoteCryptoInfo(nego);
+    } else
         throw AudioRtpFactoryException("RTP: Error: rtpSession_ is NULL in setRemoteCryptoInfo");
 }
 
diff --git a/daemon/src/audio/audiortp/audio_rtp_factory.h b/daemon/src/audio/audiortp/audio_rtp_factory.h
index f39d6519d3..fd15beb51e 100644
--- a/daemon/src/audio/audiortp/audio_rtp_factory.h
+++ b/daemon/src/audio/audiortp/audio_rtp_factory.h
@@ -153,10 +153,10 @@ class AudioRtpFactory {
         bool helloHashEnabled_;
 
         /** Remote srtp crypto context to be set into incoming data queue. */
-        ost::CryptoContext *remoteContext_;
+        ost::CryptoContext *cachedRemoteContext_;
 
         /** Local srtp crypto context to be set into outgoing data queue. */
-        ost::CryptoContext *localContext_;
+        ost::CryptoContext *cachedLocalContext_;
 
         SIPCall *ca_;
         KeyExchangeProtocol keyExchangeProtocol_;
diff --git a/daemon/src/audio/audiortp/audio_srtp_session.cpp b/daemon/src/audio/audiortp/audio_srtp_session.cpp
index 335dfda5fe..fb14463dbb 100644
--- a/daemon/src/audio/audiortp/audio_srtp_session.cpp
+++ b/daemon/src/audio/audiortp/audio_srtp_session.cpp
@@ -131,13 +131,8 @@ AudioSrtpSession::AudioSrtpSession(SIPCall &call) :
     remoteOfferIsSet_(false)
 {}
 
-AudioSrtpSession::~AudioSrtpSession()
-{
-    delete localCryptoCtx_;
-    delete remoteCryptoCtx_;
-}
 
-void AudioSrtpSession::initLocalCryptoInfo()
+ost::CryptoContext* AudioSrtpSession::initLocalCryptoInfo()
 {
     DEBUG("AudioSrtp: Set cryptographic info for this rtp session");
 
@@ -150,6 +145,7 @@ void AudioSrtpSession::initLocalCryptoInfo()
     localCryptoCtx_->deriveSrtpKeys(0);
 
     setOutQueueCryptoContext(localCryptoCtx_);
+    return localCryptoCtx_;
 }
 
 std::vector<std::string> AudioSrtpSession::getLocalCryptoInfo()
@@ -184,7 +180,8 @@ std::vector<std::string> AudioSrtpSession::getLocalCryptoInfo()
     return crypto_vector;
 }
 
-void AudioSrtpSession::setRemoteCryptoInfo(sfl::SdesNegotiator& nego)
+ost::CryptoContext*
+AudioSrtpSession::setRemoteCryptoInfo(sfl::SdesNegotiator& nego)
 {
     if (not remoteOfferIsSet_) {
         DEBUG("%s", nego.getKeyInfo().c_str());
@@ -203,9 +200,9 @@ void AudioSrtpSession::setRemoteCryptoInfo(sfl::SdesNegotiator& nego)
         initializeRemoteCryptoContext();
         setInQueueCryptoContext(remoteCryptoCtx_);
 
-        // initLocalCryptoInfo();
         remoteOfferIsSet_ = true;
     }
+    return remoteCryptoCtx_;
 }
 
 void AudioSrtpSession::initializeLocalMasterKey()
@@ -266,24 +263,20 @@ void AudioSrtpSession::initializeRemoteCryptoContext()
 
     CryptoSuiteDefinition crypto = sfl::CryptoSuites[remoteCryptoSuite_];
 
-    if (remoteCryptoCtx_) {
-        delete remoteCryptoCtx_;
-        remoteCryptoCtx_ = NULL;
-    }
-
+    delete remoteCryptoCtx_;
     remoteCryptoCtx_ = new ost::CryptoContext(0x0,
-            0,                               // roc,
-            0L,                              // keydr,
-            SrtpEncryptionAESCM,             // encryption algo
-            SrtpAuthenticationSha1Hmac,      // authtication algo
-            remoteMasterKey_,
-            remoteMasterKeyLength_,
-            remoteMasterSalt_,
-            remoteMasterSaltLength_,
-            crypto.encryptionKeyLength / 8,
-            crypto.srtpAuthKeyLength / 8,
-            crypto.masterSaltLength / 8,                         // session salt len
-            crypto.srtpAuthTagLength / 8);
+                                              0,    // roc,
+                                              0L,   // keydr,
+                                              SrtpEncryptionAESCM,
+                                              SrtpAuthenticationSha1Hmac,
+                                              remoteMasterKey_,
+                                              remoteMasterKeyLength_,
+                                              remoteMasterSalt_,
+                                              remoteMasterSaltLength_,
+                                              crypto.encryptionKeyLength / 8,
+                                              crypto.srtpAuthKeyLength / 8,
+                                              crypto.masterSaltLength / 8,
+                                              crypto.srtpAuthTagLength / 8);
 
 }
 
@@ -293,28 +286,31 @@ void AudioSrtpSession::initializeLocalCryptoContext()
 
     CryptoSuiteDefinition crypto = sfl::CryptoSuites[localCryptoSuite_];
 
-    if (localCryptoCtx_)
-        delete localCryptoCtx_;
-
+    delete localCryptoCtx_;
     localCryptoCtx_ = new ost::CryptoContext(OutgoingDataQueue::getLocalSSRC(),
-            0,                               // roc,
-            0L,                              // keydr,
-            SrtpEncryptionAESCM,             // encryption algo
-            SrtpAuthenticationSha1Hmac,      // authtication algo
-            localMasterKey_,
-            localMasterKeyLength_,
-            localMasterSalt_,
-            localMasterSaltLength_,
-            crypto.encryptionKeyLength / 8,
-            crypto.srtpAuthKeyLength / 8,
-            crypto.masterSaltLength / 8,                         // session salt len
-            crypto.srtpAuthTagLength / 8);
+                                             0,     // roc,
+                                             0L,    // keydr,
+                                             SrtpEncryptionAESCM,
+                                             SrtpAuthenticationSha1Hmac,
+                                             localMasterKey_,
+                                             localMasterKeyLength_,
+                                             localMasterSalt_,
+                                             localMasterSaltLength_,
+                                             crypto.encryptionKeyLength / 8,
+                                             crypto.srtpAuthKeyLength / 8,
+                                             crypto.masterSaltLength / 8,
+                                             crypto.srtpAuthTagLength / 8);
 }
 
-void AudioSrtpSession::restoreCryptoContext(ost::CryptoContext *localContext, ost::CryptoContext *remoteContext)
+void AudioSrtpSession::restoreCryptoContext(ost::CryptoContext *localContext,
+                                            ost::CryptoContext *remoteContext)
 {
-    setInQueueCryptoContext(remoteContext);
-    setOutQueueCryptoContext(localContext);
+    delete remoteCryptoCtx_;
+    remoteCryptoCtx_ = remoteContext;
+    delete localCryptoCtx_;
+    localCryptoCtx_ = localContext;
+    setInQueueCryptoContext(remoteCryptoCtx_);
+    setOutQueueCryptoContext(localCryptoCtx_);
 }
 
 }
diff --git a/daemon/src/audio/audiortp/audio_srtp_session.h b/daemon/src/audio/audiortp/audio_srtp_session.h
index 89be177508..97b9b2ce67 100644
--- a/daemon/src/audio/audiortp/audio_srtp_session.h
+++ b/daemon/src/audio/audiortp/audio_srtp_session.h
@@ -73,7 +73,6 @@ class AudioSrtpSession : public AudioSymmetricRtpSession {
          * Constructor for this rtp session
          */
         AudioSrtpSession(SIPCall &call);
-        ~AudioSrtpSession();
 
         /**
          * Used to get sdp crypto header to be included in sdp session. This
@@ -84,15 +83,17 @@ class AudioSrtpSession : public AudioSymmetricRtpSession {
 
         /**
          * Set remote crypto header from incoming sdp offer
+         * @return The new remote crypto context, to be cached by the caller
          */
-        void setRemoteCryptoInfo(sfl::SdesNegotiator &nego);
+        ost::CryptoContext* setRemoteCryptoInfo(sfl::SdesNegotiator &nego);
 
         /**
          * Init local crypto context for outgoing data
-        * this method must be called before sending first Invite request
-        * with SDP offer.
-        */
-        void initLocalCryptoInfo();
+         * this method must be called before sending first Invite request
+         * with SDP offer.
+         * @return The new local crypto context, to be cached by the caller
+         */
+        ost::CryptoContext* initLocalCryptoInfo();
 
         /**
          * Restore the cryptographic context. most likely useful to restore
@@ -100,6 +101,8 @@ class AudioSrtpSession : public AudioSymmetricRtpSession {
          */
         void restoreCryptoContext(ost::CryptoContext *, ost::CryptoContext *);
 
+    private:
+        NON_COPYABLE(AudioSrtpSession);
 
         /** Remote srtp crypto context to be set into incoming data queue. */
         ost::CryptoContext* remoteCryptoCtx_;
@@ -107,9 +110,6 @@ class AudioSrtpSession : public AudioSymmetricRtpSession {
         /** Local srtp crypto context to be set into outgoing data queue. */
         ost::CryptoContext* localCryptoCtx_;
 
-    private:
-        NON_COPYABLE(AudioSrtpSession);
-
         /**
          * Init local master key according to current crypto context
          * as defined in SdesNegotiator.h
-- 
GitLab