Subject: Don't reject partial SNOM SRTP
From: Walter Doekes <wjdoekes@osso.nl>
Forwarded: ASTERISK-20234
Last-Update: 2016-07-19

When we load res_srtp.so, chan_sip starts picking up the a=crypto lines.
SNOM phones apparently use some kind of "optional SRTP" where it sends
regular RTP/AVP but adds a=crypto. Asterisk rejects this because it has
seen the crypto line, but didn't see any RTP/SAVP.

This patch changes chan_sip behaviour to discard the a=crypto line if no
secure RTP transport (SAVP) has been signaled.

--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -1658,7 +1658,8 @@ static void handle_response(struct sip_p
 
 /*------ SRTP Support -------- */
 static int setup_srtp(struct sip_srtp **srtp);
-static int process_crypto(struct sip_pvt *p, struct ast_rtp_instance *rtp, struct sip_srtp **srtp, const char *a);
+static int process_crypto(struct sip_pvt *p, struct ast_rtp_instance *rtp, struct sip_srtp **srtp,
+	const char *a, int secure_transport);
 
 /*------ T38 Support --------- */
 static int transmit_response_with_t38_sdp(struct sip_pvt *p, char *msg, struct sip_request *req, int retrans);
@@ -10349,7 +10350,7 @@ static int process_sdp(struct sip_pvt *p
 						}
 					} else if (process_sdp_a_sendonly(value, &sendonly)) {
 						processed = TRUE;
-					} else if (!processed_crypto && process_crypto(p, p->rtp, &p->srtp, value)) {
+					} else if (!processed_crypto && process_crypto(p, p->rtp, &p->srtp, value, secure_audio)) {
 						processed_crypto = TRUE;
 						processed = TRUE;
 					} else if (process_sdp_a_audio(value, p, &newaudiortp, &last_rtpmap_codec)) {
@@ -10366,7 +10367,7 @@ static int process_sdp(struct sip_pvt *p
 						if (p->vsrtp) {
 							ast_set_flag(p->vsrtp, SRTP_CRYPTO_OFFER_OK);
 						}
-					} else if (!processed_crypto && process_crypto(p, p->vrtp, &p->vsrtp, value)) {
+					} else if (!processed_crypto && process_crypto(p, p->vrtp, &p->vsrtp, value, secure_video)) {
 						processed_crypto = TRUE;
 						processed = TRUE;
 					} else if (process_sdp_a_video(value, p, &newvideortp, &last_rtpmap_codec)) {
@@ -10379,7 +10380,7 @@ static int process_sdp(struct sip_pvt *p
 						processed = TRUE;
 					} else if (process_sdp_a_text(value, p, &newtextrtp, red_fmtp, &red_num_gen, red_data_pt, &last_rtpmap_codec)) {
 						processed = TRUE;
-					} else if (!processed_crypto && process_crypto(p, p->trtp, &p->tsrtp, value)) {
+					} else if (!processed_crypto && process_crypto(p, p->trtp, &p->tsrtp, value, 1)) {
 						processed_crypto = TRUE;
 						processed = TRUE;
 					}
@@ -33609,7 +33610,8 @@ static int setup_srtp(struct sip_srtp **
 	return 0;
 }
 
-static int process_crypto(struct sip_pvt *p, struct ast_rtp_instance *rtp, struct sip_srtp **srtp, const char *a)
+static int process_crypto(struct sip_pvt *p, struct ast_rtp_instance *rtp, struct sip_srtp **srtp,
+	const char *a, int secure_transport)
 {
 	struct ast_rtp_engine_dtls *dtls;
 
@@ -33622,6 +33624,22 @@ static int process_crypto(struct sip_pvt
 	if (strncasecmp(a, "crypto:", 7)) {
 		return FALSE;
 	}
+	if (!secure_transport) {
+		/* > The Secure Real-time Transport Protocol (SRTP)
+		 * > [RFC3711] provides security services for RTP media
+		 * > and is signaled by use of secure RTP transport (e.g.,
+		 * > "RTP/SAVP" or "RTP/SAVPF") in an SDP media (m=) line.
+		 * > ...
+		 * > The "crypto" attribute MUST only appear at the SDP
+		 * > media level (not at the session level).
+		 *
+		 * Ergo, we can trust RTP/(S)AVP to be read from the m=
+		 * line before we get here. If it was RTP/AVP, then this
+		 * is SNOM-specific optional SRTP. Ignore it.
+		 */
+		ast_log(LOG_WARNING, "Ignoring crypto attribute in SDP because RTP transport is insecure\n");
+		return FALSE;
+	}
 	if (!*srtp) {
 		if (ast_test_flag(&p->flags[0], SIP_OUTGOING)) {
 			ast_log(LOG_WARNING, "Ignoring unexpected crypto attribute in SDP answer\n");