Fix SRTP ROC retry after transport reinit being a no-op

pjmedia/CMakeLists.txt

@@ -957,6 +957,7 @@ if(BUILD_TESTING)
     src/test/tone_detector_test.c
     src/test/sdp_neg_test.c
     src/test/sdp_attr_test.c
+    src/test/transport_srtp_test.c
   )
 
   target_link_libraries(pjmedia-test

pjmedia/build/Makefile

@@ -157,7 +157,7 @@ export PJMEDIA_TEST_SRCDIR = ../src/test
 export PJMEDIA_TEST_OBJS += codec_vectors.o conf_test.o jbuf_test.o main.o mips_test.o \
 			    vid_codec_test.o vid_dev_test.o vid_port_test.o \
 			    rtp_test.o test.o tone_detector_test.o
-export PJMEDIA_TEST_OBJS += sdp_neg_test.o sdp_attr_test.o
+export PJMEDIA_TEST_OBJS += sdp_neg_test.o sdp_attr_test.o transport_srtp_test.o
 export PJMEDIA_TEST_CFLAGS += $(_CFLAGS)
 export PJMEDIA_TEST_CXXFLAGS += $(_CXXFLAGS)
 export PJMEDIA_TEST_LDFLAGS += $(PJMEDIA_CODEC_LDLIB) \

pjmedia/build/pjmedia_test.vcproj

@@ -4938,6 +4938,10 @@
 					/>
 				</FileConfiguration>
 			</File>
+			<File
+				RelativePath="..\src\test\transport_srtp_test.c"
+				>
+			</File>
 			<File
 				RelativePath="..\src\test\test.c"
 				>

pjmedia/build/pjmedia_test.vcxproj

@@ -588,6 +588,7 @@
     </ClCompile>
     <ClCompile Include="..\src\test\sdp_attr_test.c" />
     <ClCompile Include="..\src\test\sdp_neg_test.c" />
+    <ClCompile Include="..\src\test\transport_srtp_test.c" />
     <ClCompile Include="..\src\test\session_test.c">
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Debug-Dynamic|Win32'">true</ExcludedFromBuild>
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Debug-Dynamic|x64'">true</ExcludedFromBuild>

pjmedia/build/pjmedia_test.vcxproj.filters

@@ -36,6 +36,9 @@
     <ClCompile Include="..\src\test\sdp_neg_test.c">
       <Filter>Source Files</Filter>
     </ClCompile>
+    <ClCompile Include="..\src\test\transport_srtp_test.c">
+      <Filter>Source Files</Filter>
+    </ClCompile>
     <ClCompile Include="..\src\test\sdptest.c">
       <Filter>Source Files</Filter>
     </ClCompile>

pjmedia/include/pjmedia/transport_srtp.h

@@ -249,6 +249,9 @@ typedef struct pjmedia_srtp_setting
      * RTP sequence rollover counter initialization value for incoming
      * direction. This is useful to maintain ROC after media transport
      * recreation such as in IP change scenario.
+     * Note that once a packet has been authenticated, the transport
+     * updates this to the ROC of that packet, so callers of
+     * #pjmedia_transport_srtp_get_setting() may see it changed.
      */
     pjmedia_srtp_roc             rx_roc;
 
@@ -258,7 +261,9 @@ typedef struct pjmedia_srtp_setting
      * ROC but for some reason, they don't. Thus, when we encounter
      * SRTP packet unprotect failure during probation, we will retry to
      * unprotect with this ROC value as well.
-     * Set prev_rx_roc.ssrc to 0 to disable this feature.
+     * Set prev_rx_roc.ssrc to 0 to disable this feature. The transport
+     * clears prev_rx_roc.ssrc once a packet of rx_roc.ssrc has been
+     * authenticated, closing the retry window.
      */
     pjmedia_srtp_roc             prev_rx_roc;
 

pjmedia/src/pjmedia/transport_srtp.c

@@ -349,6 +349,9 @@ typedef struct transport_srtp
 
     pj_uint32_t          tx_ssrc;
 
+    /* ROC retry: prev_rx_roc.roc is the candidate currently applied. */
+    pj_bool_t            rx_roc_retry_alt;
+
 } transport_srtp;
 
 
@@ -917,7 +920,11 @@ PJ_DEF(pj_status_t) pjmedia_transport_srtp_get_setting(
                                        pjmedia_srtp_setting *opt)
 {
     transport_srtp  *srtp = (transport_srtp*) tp;
+
+    pj_lock_acquire(srtp->mutex);
     *opt = srtp->setting;
+    pj_lock_release(srtp->mutex);
+
     return PJ_SUCCESS;
 }
 
@@ -929,7 +936,11 @@ PJ_DEF(pj_status_t) pjmedia_transport_srtp_modify_setting(
                                        const pjmedia_srtp_setting *opt)
 {
     transport_srtp  *srtp = (transport_srtp*) tp;
+
+    pj_lock_acquire(srtp->mutex);
     srtp->setting = *opt;
+    pj_lock_release(srtp->mutex);
+
     return PJ_SUCCESS;
 }
 
@@ -943,6 +954,7 @@ static pj_status_t create_srtp_ctx(transport_srtp *srtp,
 {
     srtp_policy_t    tx_;
     srtp_policy_t    rx_;
+    srtp_policy_t    rx_tmpl;
     srtp_err_status_t err;
     int              cr_tx_idx = 0;
     int              au_tx_idx = 0;
@@ -1054,8 +1066,10 @@ static pj_status_t create_srtp_ctx(transport_srtp *srtp,
     else
         rx_.rtp.sec_serv    = sec_serv_none;
     rx_.key                 = (uint8_t*)ctx->rx_key;
-    if (setting->rx_roc.roc != 0 &&
-        setting->rx_roc.ssrc != 0)
+    if (setting->rx_roc.ssrc != 0 &&
+        (setting->rx_roc.roc != 0 ||
+         (setting->prev_rx_roc.ssrc == setting->rx_roc.ssrc &&
+          setting->prev_rx_roc.roc != 0)))
     {
         rx_.ssrc.type       = ssrc_specific;
         rx_.ssrc.value      = setting->rx_roc.ssrc;
@@ -1072,6 +1086,16 @@ static pj_status_t create_srtp_ctx(transport_srtp *srtp,
     rx_.rtcp                = rx_.rtp;
     rx_.rtcp.auth_tag_len   = crypto_suites[au_rx_idx].srtcp_auth_tag_len;
     rx_.next                = NULL;
+    if (rx_.ssrc.type == ssrc_specific) {
+        /* Keep a wildcard template alongside the specific stream, so
+         * a remote SSRC change after probation is still accepted.
+         */
+        rx_tmpl             = rx_;
+        rx_tmpl.ssrc.type   = ssrc_any_inbound;
+        rx_tmpl.ssrc.value  = 0;
+        rx_tmpl.next        = NULL;
+        rx_.next            = &rx_tmpl;
+    }
     err = srtp_create(&ctx->srtp_rx_ctx, &rx_);
     if (err != srtp_err_status_ok) {
         srtp_dealloc(ctx->srtp_tx_ctx);
@@ -1089,6 +1113,8 @@ static pj_status_t create_srtp_ctx(transport_srtp *srtp,
                    setting->rx_roc.ssrc, setting->rx_roc.roc,
                    (err == srtp_err_status_ok)? "succeeded": "failed"));
     }
+    if (ctx == &srtp->srtp_ctx)
+        srtp->rx_roc_retry_alt = PJ_FALSE;
     ctx->rx_policy = *rx;
     pj_strset(&ctx->rx_policy.key,  ctx->rx_key, rx->key.slen);
     ctx->rx_policy.name=pj_str(crypto_suites[get_crypto_idx(&rx->name)].name);
@@ -1653,6 +1679,13 @@ static void srtp_rtp_cb(pjmedia_tp_cb_param *param)
         return;
     }
 
+    /* Runt packets cannot hold an RTP header. */
+    if (size < (pj_ssize_t)sizeof(pjmedia_rtp_hdr)) {
+        PJ_LOG(5, (srtp->pool->obj_name, "Runt RTP packet, size=%ld",
+                   (long)size));
+        return;
+    }
+
     /* Give the packet to keying first by invoking its send_rtp() op.
      * Yes, the usage of send_rtp() is rather hacky, but it is convenient
      * as the signature suits the purpose and it is ready to use
@@ -1751,6 +1784,7 @@ static void srtp_rtp_cb(pjmedia_tp_cb_param *param)
 
 #if PJMEDIA_SRTP_CHECK_ROC_ON_RESTART
     if (srtp->probation_cnt > 0 && err == srtp_err_status_auth_fail &&
+        rx_ssrc == srtp->setting.rx_roc.ssrc &&
         srtp->setting.prev_rx_roc.ssrc != 0 &&
         srtp->setting.prev_rx_roc.ssrc == srtp->setting.rx_roc.ssrc &&
         srtp->setting.prev_rx_roc.roc != srtp->setting.rx_roc.roc)
@@ -1790,28 +1824,40 @@ static void srtp_rtp_cb(pjmedia_tp_cb_param *param)
         }
 
     } else if (need_retry) {
-        unsigned roc, new_roc;
+        unsigned old_roc, new_roc;
         srtp_err_status_t status;
 
-        srtp_get_stream_roc(srtp->srtp_ctx.srtp_rx_ctx,
-                            srtp->setting.rx_roc.ssrc, &roc);
-        new_roc = (roc == srtp->setting.rx_roc.roc?
-                   srtp->setting.prev_rx_roc.roc: srtp->setting.rx_roc.roc);
+        /* Try the alternative candidate, the setting is only updated
+         * after a packet authenticates.
+         */
+        old_roc = (srtp->rx_roc_retry_alt? srtp->setting.prev_rx_roc.roc :
+                   srtp->setting.rx_roc.roc);
+        new_roc = (srtp->rx_roc_retry_alt? srtp->setting.rx_roc.roc :
+                   srtp->setting.prev_rx_roc.roc);
         status = srtp_set_stream_roc(srtp->srtp_ctx.srtp_rx_ctx,
                                      srtp->setting.rx_roc.ssrc, new_roc);
         if (status == srtp_err_status_ok) {
+            srtp->rx_roc_retry_alt = !srtp->rx_roc_retry_alt;
             PJ_LOG(4, (srtp->pool->obj_name,
                        "Retrying to unprotect SRTP from ROC %d to new ROC %d",
-                       roc, new_roc));
+                       old_roc, new_roc));
             err = srtp_unprotect(srtp->srtp_ctx.srtp_rx_ctx, (pj_uint8_t*)pkt,
                                  &len);
+        } else {
+            /* Cannot set the ROC, stop retrying. */
+            srtp->setting.prev_rx_roc.ssrc = 0;
+            PJ_LOG(4, (srtp->pool->obj_name,
+                       "Failed to set RX ROC %d for retry, err=%s",
+                       new_roc, get_libsrtp_errstr(status)));
         }
     }
 
     if (err != srtp_err_status_ok) {
         PJ_LOG(5,(srtp->pool->obj_name,
-                  "Failed to unprotect SRTP, pkt size=%ld, SSRC=%u, err=%s",
-                  size, rx_ssrc, get_libsrtp_errstr(err)));
+                  "Failed to unprotect SRTP, pkt size=%ld, SSRC=%u, "
+                  "seq=%u, err=%s",
+                  size, rx_ssrc, ntohs(((pjmedia_rtp_hdr*)pkt)->seq),
+                  get_libsrtp_errstr(err)));
     } else {
         cb = srtp->rtp_cb;
         cb2 = srtp->rtp_cb2;
@@ -1822,6 +1868,18 @@ static void srtp_rtp_cb(pjmedia_tp_cb_param *param)
 
         /* Save SSRC after successful SRTP unprotect */
         srtp->rx_ssrc = rx_ssrc;
+
+        /* Settle the ROC retry on the SSRC it belongs to: write back
+         * the winning ROC and close the retry window.
+         */
+        if (srtp->setting.prev_rx_roc.ssrc != 0 &&
+            rx_ssrc == srtp->setting.rx_roc.ssrc)
+        {
+            if (srtp->rx_roc_retry_alt)
+                srtp->setting.rx_roc.roc = srtp->setting.prev_rx_roc.roc;
+            srtp->setting.prev_rx_roc.ssrc = 0;
+            srtp->rx_roc_retry_alt = PJ_FALSE;
+        }
     }
 
     pj_lock_release(srtp->mutex);

pjmedia/src/test/test.c

@@ -115,6 +115,10 @@ int test_main(int argc, char *argv[])
 #if HAS_TONE_DETECTOR_TEST
     UT_ADD_TEST(&test_app.ut_app, tone_detector_test, 0);
 #endif
+#if HAS_TRANSPORT_SRTP_TEST
+    /* Creates/destroys a local pjmedia_endpt (see codec_test_vectors). */
+    UT_ADD_TEST(&test_app.ut_app, transport_srtp_test, PJ_TEST_EXCLUSIVE);
+#endif
 #if HAS_CODEC_VECTOR_TEST
     /* Run in exclusive mode: creates/destroys a local pjmedia_endpt which
      * sets/clears the global def_codec_mgr. If sdp_neg_test runs

pjmedia/src/test/test.h

@@ -39,6 +39,8 @@
 #define HAS_MIPS_TEST           WITH_BENCHMARK
 #define HAS_CODEC_VECTOR_TEST   1
 #define HAS_TONE_DETECTOR_TEST  1
+#define HAS_TRANSPORT_SRTP_TEST (PJMEDIA_HAS_SRTP && \
+                                 PJMEDIA_SRTP_CHECK_ROC_ON_RESTART)
 
 int session_test(void);
 int rtp_test(void);
@@ -53,6 +55,7 @@ int vid_codec_test(void);
 int vid_dev_test(void);
 int vid_port_test(void);
 int tone_detector_test(void);
+int transport_srtp_test(void);
 
 extern pj_pool_factory *mem;
 void app_perror(pj_status_t status, const char *title);