ice: avoid forced ICE in answers to non-ICE offers (#2161)

daemon/call.c

@@ -1426,6 +1426,14 @@ static void __ice_offer(const sdp_ng_flags *flags, struct call_media *this,
 	if (flags->ice_reject)
 		MEDIA_CLEAR(other, ICE);
 
+	// An answer must not introduce ICE where the original offer did not
+	// advertise it. A locally created ICE agent means we intentionally
+	// offered ICE on this leg, in which case keep the negotiated state.
+	// In other cases ignore ICE=force on the answer (see issue #2161).
+	if (flags->opmode == OP_ANSWER && flags->ice_option == ICE_FORCE &&
+			!MEDIA_ISSET(other, ICE) && !this->ice_agent)
+		MEDIA_CLEAR(this, ICE);
+
 	if (flags->passthrough_on) {
 		ilog(LOG_DEBUG, "enabling passthrough mode");
 		MEDIA_SET(this, PASSTHRU);

docs/ng_control_protocol.md

@@ -439,6 +439,11 @@ Optionally included keys are:
 	had ICE attributes listed. If the incoming SDP did not contain ICE attributes, then no
 	ICE attributes are added.
 
+	For `answer` messages, an endpoint must not negotiate ICE that was absent from
+	the corresponding offer. Prefer `ICE=default` when handling an answer from
+	a peer which might not support ICE. `ICE=force` on an answer cannot invent
+	ICE support when no ICE was offered on that leg.
+
 	With `force-relay`, existing ICE candidates are left in place except `relay`
 	type candidates, and *rtpengine* inserts itself as a `relay` candidate. It will also leave SDP
 	c= and m= lines unchanged.

t/Makefile

@@ -87,7 +87,7 @@ include ../lib/common.Makefile
 	daemon-tests-templ-def daemon-tests-templ-def-offer daemon-tests-t38 daemon-tests-evs-dtx \
 	daemon-tests-transform daemon-tests-http daemon-tests-heuristic daemon-tests-asymmetric \
 	daemon-tests-dtx-no-shift daemon-tests-rtcp daemon-tests-redis-subscribe daemon-tests-rtp-ext daemon-tests-ssrc \
-	daemon-tests-bundle daemon-tests-dtls daemon-tests-rollback \
+	daemon-tests-bundle daemon-tests-dtls daemon-tests-ice-force-answer daemon-tests-rollback \
 	daemon-tests-rollback-redis daemon-tests-redis-srtp-index \
 	daemon-tests-recording daemon-tests-create daemon-tests-created-subscriptions daemon-tests-alias \
 	daemon-tests-kernel-api \
@@ -140,7 +140,7 @@ daemon-tests: daemon-tests-main daemon-tests-jb daemon-tests-pubsub daemon-tests
 	daemon-tests-sdp-orig-replacements daemon-tests-moh daemon-tests-evs-dtx daemon-tests-transform \
 	daemon-tests-transcode-config daemon-tests-codec-prefs daemon-tests-http daemon-tests-heuristic \
 	daemon-tests-asymmetric daemon-tests-rtcp daemon-tests-redis-subscribe daemon-tests-rtp-ext daemon-tests-ssrc \
-	daemon-tests-bundle daemon-tests-dtls daemon-tests-rollback daemon-tests-rollback-redis \
+	daemon-tests-bundle daemon-tests-dtls daemon-tests-ice-force-answer daemon-tests-rollback daemon-tests-rollback-redis \
 	daemon-tests-redis-srtp-index \
 	daemon-tests-recording daemon-tests-create daemon-tests-created-subscriptions \
 	daemon-tests-dtx daemon-tests-dtx-cn daemon-tests-dtx-no-shift daemon-tests-dtx-dtmf \
@@ -320,6 +320,9 @@ daemon-tests-bundle:	daemon-test-deps
 daemon-tests-dtls:	daemon-test-deps
 	./auto-test-helper "$@" perl -I../perl auto-daemon-tests-dtls.pl
 
+daemon-tests-ice-force-answer:	daemon-test-deps
+	./auto-test-helper "$@" perl -I../perl auto-daemon-tests-ice-force-answer.pl
+
 daemon-tests-recording:	daemon-test-deps
 	./auto-test-helper "$@" perl -I../perl auto-daemon-tests-recording.pl
 

t/auto-daemon-tests-ice-force-answer.pl

@@ -0,0 +1,77 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+use NGCP::Rtpengine::Test;
+use NGCP::Rtpclient::SRTP;
+use NGCP::Rtpengine::AutoTest;
+use Test::More;
+
+# Regression coverage for issue #2161: forced ICE towards a plain-RTP peer
+# that explicitly rejects ICE in its answer. The answer towards the
+# originating plain-RTP peer must not invent ICE, even when the answer
+# control request specifies ICE=force.
+autotest_start(qw(--config-file=none -t -1 -i 203.0.113.1 -i 2001:db8:4321::1
+                -n 2223 -f -L 7 -E)) or die;
+
+my ($sock_a, $sock_b, $port_a, $port_b);
+($sock_a, $sock_b) = new_call(
+    [qw(198.51.100.1 7320)],
+    [qw(198.51.100.3 7322)],
+);
+
+($port_a) = offer('forced ICE offer, plain RTP peer', {
+    ICE => 'force',
+}, <<SDP);
+v=0
+o=- 1545997027 1 IN IP4 198.51.100.1
+s=tester
+t=0 0
+m=audio 7320 RTP/AVP 0
+c=IN IP4 198.51.100.1
+a=sendrecv
+----------------------------------
+v=0
+o=- 1545997027 1 IN IP4 198.51.100.1
+s=tester
+t=0 0
+m=audio PORT RTP/AVP 0
+c=IN IP4 203.0.113.1
+a=rtpmap:0 PCMU/8000
+a=sendrecv
+a=rtcp:PORT
+a=ice-ufrag:ICEUFRAG
+a=ice-pwd:ICEPWD
+a=candidate:ICEBASE 1 UDP 2130706431 203.0.113.1 PORT typ host
+a=candidate:ICEBASE 1 UDP 2130706175 2001:db8:4321::1 PORT typ host
+SDP
+
+($port_b) = answer('plain RTP answer rejects offered ICE', {
+    ICE => 'force',
+}, <<SDP);
+v=0
+o=- 1545997027 1 IN IP4 198.51.100.3
+s=tester
+t=0 0
+m=audio 7322 RTP/AVP 0
+c=IN IP4 198.51.100.3
+a=sendrecv
+----------------------------------
+v=0
+o=- 1545997027 1 IN IP4 198.51.100.3
+s=tester
+t=0 0
+m=audio PORT RTP/AVP 0
+c=IN IP4 203.0.113.1
+a=rtpmap:0 PCMU/8000
+a=sendrecv
+a=rtcp:PORT
+SDP
+
+snd($sock_a, $port_b, rtp(0, 1000, 3000, 0x1234, "\x00" x 160));
+rcv($sock_b, $port_a, rtpm(0, 1000, 3000, 0x1234, "\x00" x 160));
+snd($sock_b, $port_a, rtp(0, 2000, 6000, 0x5678, "\x00" x 160));
+rcv($sock_a, $port_b, rtpm(0, 2000, 6000, 0x5678, "\x00" x 160));
+
+rtpe_req('delete', 'delete');
+done_testing();