From f0430580954ea398f6623568eb62c5942f4395f1 Mon Sep 17 00:00:00 2001 From: Repin Agent Date: Fri, 17 Jul 2026 03:43:48 -0600 Subject: [PATCH 1/2] feat(sip): interpret external_media_address FQDN-vs-literal in SDP, fail closed on DNS failure (M6 CP3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit external_media_address was emitted verbatim into c=/o= — an FQDN produced a malformed 'c=IN IP4 myhost.example.com'. CP3 interprets it: an IP literal is emitted as-is; an FQDN is resolved to an IP (preferring the peer's address family). On DNS failure it FAILS CLOSED — advertised_media_ip now returns Option and every caller rejects the call setup rather than advertise an unresolved FQDN or fall back to a leaky internal address: - outbound offer (channel_driver::request) -> Err, dial fails - inbound INVITE answer -> 488 Not Acceptable Here - UPDATE renegotiation answer -> 488 - re-INVITE answer -> 488 Receiver-side proof (e2e_media_fail_closed.rs): a peer sends an INVITE; an IP-literal external is ANSWERED 200 with that c= (positive control), an unresolvable FQDN external is REJECTED with a 4xx and NEVER 200. RED: fall open (emit the unresolved FQDN) -> the FQDN case is accepted/answered -> assertion RED. Plus unit tests: IP-literal passthrough, FQDN resolves, unresolvable fails-closed (None), and end-to-end advertised_media_ip_with fail-closed. --- .../tests/e2e_media_fail_closed.rs | 213 ++++++++++++++++++ crates/asterisk-sip/src/channel_driver.rs | 15 +- crates/asterisk-sip/src/event_handler.rs | 54 ++++- crates/asterisk-sip/src/sdp/mod.rs | 117 +++++++++- 4 files changed, 375 insertions(+), 24 deletions(-) create mode 100644 crates/asterisk-integration-tests/tests/e2e_media_fail_closed.rs diff --git a/crates/asterisk-integration-tests/tests/e2e_media_fail_closed.rs b/crates/asterisk-integration-tests/tests/e2e_media_fail_closed.rs new file mode 100644 index 0000000..5ec27d9 --- /dev/null +++ b/crates/asterisk-integration-tests/tests/e2e_media_fail_closed.rs @@ -0,0 +1,213 @@ +//! End-to-end acceptance for M6 CP3: `external_media_address` FQDN-vs-literal in +//! SDP, **fail-closed on DNS failure**. +//! +//! `external_media_address` was an uninterpreted string emitted verbatim into +//! `c=`/`o=`. CP3 interprets it: an IP literal is emitted; an FQDN is resolved; +//! and if the FQDN does NOT resolve, rustisk must FAIL CLOSED — reject the call +//! setup rather than advertise an unresolved FQDN or fall back to a leaky +//! internal address. +//! +//! Receiver-side proof: a SIP peer sends an INVITE and observes what rustisk +//! sends back. +//! * **positive control** — an IP-literal external resolves and the call is +//! ANSWERED 200 with that address in `c=`. +//! * **fail-closed** — an unresolvable FQDN external -> rustisk REJECTS with a +//! 4xx and NEVER sends a 200 (so no bogus/internal media address reaches the +//! peer). +//! +//! RED control (captured in the PR body): make the resolution fall open (emit +//! the internal address instead of failing closed) -> the peer receives a 200 +//! for the unresolvable-FQDN case -> the "no 200 / must be rejected" assertion +//! goes RED. + +use std::net::SocketAddr; +use std::sync::Arc; +use std::time::{Duration, Instant}; + +use asterisk_apps::adapter::register_all_apps; +use asterisk_codecs::codecs; +use asterisk_core::channel::tech_registry::TECH_REGISTRY; +use asterisk_core::pbx::{Context, Dialplan, Extension, Priority}; +use asterisk_sip::channel_driver::SipChannelDriver; +use asterisk_sip::event_handler::SipEventHandler; +use asterisk_sip::parser::SipMessage; +use asterisk_sip::pjsip_config::{ + set_global_pjsip_config, EndpointConfig, PjsipConfig, TransportConfig, +}; +use asterisk_sip::sdp::SessionDescription; +use asterisk_sip::session::SipSession; +use asterisk_sip::transport::UdpTransport; +use tokio::net::UdpSocket; + +const EXTEN: &str = "100"; + +/// Wait for a specific SIP status code, skipping provisional 1xx. +async fn recv_sip_status(sock: &UdpSocket, status: u16, budget: Duration) -> Option { + let deadline = Instant::now() + budget; + let mut buf = [0u8; 4096]; + while Instant::now() < deadline { + if let Ok(Ok((len, _))) = + tokio::time::timeout(Duration::from_millis(300), sock.recv_from(&mut buf)).await + { + if let Ok(msg) = SipMessage::parse(&buf[..len]) { + if msg.status_code() == Some(status) { + return Some(msg); + } + } + } + } + None +} + +/// Collect every distinct status code seen within the budget. +async fn collect_status_codes(sock: &UdpSocket, budget: Duration) -> Vec { + let deadline = Instant::now() + budget; + let mut buf = [0u8; 4096]; + let mut seen = Vec::new(); + while Instant::now() < deadline { + if let Ok(Ok((len, _))) = + tokio::time::timeout(Duration::from_millis(300), sock.recv_from(&mut buf)).await + { + if let Ok(msg) = SipMessage::parse(&buf[..len]) { + if let Some(code) = msg.status_code() { + if !seen.contains(&code) { + seen.push(code); + } + } + } + } + } + seen +} + +fn invite_request(call_id: &str, contact_port: u16, sdp: &str) -> SipMessage { + let raw = format!( + "INVITE sip:{EXTEN}@127.0.0.1 SIP/2.0\r\n\ + Via: SIP/2.0/UDP 127.0.0.1:{contact_port};branch=z9hG4bK{call_id}inv\r\n\ + From: \"Caller\" ;tag=caller{call_id}\r\n\ + To: \r\n\ + Call-ID: {call_id}\r\n\ + CSeq: 1 INVITE\r\n\ + Contact: \r\n\ + Content-Type: application/sdp\r\n\ + Content-Length: {len}\r\n\ + \r\n\ + {sdp}", + len = sdp.len() + ); + SipMessage::parse(raw.as_bytes()).unwrap() +} + +fn config_with_external_media(external: &str) -> PjsipConfig { + PjsipConfig { + endpoints: vec![EndpointConfig { + name: EXTEN.to_string(), + context: "default".to_string(), + auth: None, + ..Default::default() + }], + transports: vec![TransportConfig { + name: "transport-udp".to_string(), + protocol: "udp".to_string(), + bind: "127.0.0.1:5060".parse().unwrap(), + external_media_address: Some(external.to_string()), + external_signaling_address: None, + external_signaling_port: None, + cert_file: None, + priv_key_file: None, + // Empty local_net: the loopback caller is treated as EXTERNAL, so + // the external_media_address path (and CP3 resolution) is exercised. + local_net: vec![], + }], + ..Default::default() + } +} + +fn dialplan() -> Dialplan { + let mut dp = Dialplan::new(); + let mut ctx = Context::new("default"); + let mut ext = Extension::new(EXTEN); + ext.add_priority(Priority { + priority: 1, + app: "Answer".to_string(), + app_data: String::new(), + label: None, + }); + ctx.add_extension(ext); + dp.add_context(ctx); + dp +} + +async fn send_invite( + handler: &Arc, + sip_local: SocketAddr, + caller: &UdpSocket, + call_id: &str, +) -> Option { + let caller_addr = caller.local_addr().unwrap(); + let offer = SessionDescription::create_offer("127.0.0.1", 40000, &[codecs::pcmu()]); + let invite = invite_request(call_id, caller_addr.port(), &offer.to_string()); + let session = SipSession::new_inbound(&invite, sip_local, caller_addr).expect("session"); + handler + .handle_incoming_invite(&invite, caller_addr, session) + .await +} + +#[tokio::test] +async fn external_media_fqdn_fails_closed() { + register_all_apps(); + + let handler_transport: Arc = Arc::new( + UdpTransport::bind("127.0.0.1:0".parse().unwrap()) + .await + .unwrap(), + ); + let sip_local: SocketAddr = handler_transport.local_addr().unwrap(); + let driver = Arc::new(SipChannelDriver::new(sip_local)); + driver.set_transport(handler_transport.clone()); + TECH_REGISTRY.register(driver.clone()); + let handler = Arc::new(SipEventHandler::new(Arc::new(dialplan()), handler_transport)); + handler.set_channel_driver(driver.clone()); + + let caller = UdpSocket::bind("127.0.0.1:0").await.unwrap(); + + // ---- POSITIVE CONTROL: IP-literal external resolves -> 200 answered ---- + set_global_pjsip_config(config_with_external_media("203.0.113.99")); + let accepted = send_invite(&handler, sip_local, &caller, "media-fc-ok").await; + assert_eq!( + accepted.as_deref(), + Some("media-fc-ok"), + "positive control: an IP-literal external_media_address must be accepted" + ); + let ok = recv_sip_status(&caller, 200, Duration::from_secs(5)) + .await + .expect("positive control: IP-literal external must be answered 200"); + let answer = SessionDescription::parse(&ok.body).expect("200 must carry SDP"); + assert_eq!( + answer.connection.as_ref().unwrap().addr, + "203.0.113.99", + "positive control: the resolved IP literal is advertised in c=" + ); + println!("[E2E] positive control: IP-literal external_media_address -> 200 with c=203.0.113.99"); + + // ---- FAIL-CLOSED: unresolvable FQDN external -> rejected, NO 200 ------- + set_global_pjsip_config(config_with_external_media("no-such-host.invalid")); + let accepted = send_invite(&handler, sip_local, &caller, "media-fc-fail").await; + assert_eq!( + accepted, None, + "fail-closed: an unresolvable external_media_address FQDN must NOT be accepted" + ); + let codes = collect_status_codes(&caller, Duration::from_secs(3)).await; + assert!( + !codes.contains(&200), + "fail-closed: rustisk must NEVER send a 200 (with a bogus/internal c=) for an \ + unresolvable external_media_address FQDN; saw {codes:?}" + ); + assert!( + codes.iter().any(|c| (400..600).contains(c)), + "fail-closed: rustisk must reject the INVITE with a 4xx/5xx; saw {codes:?}" + ); + println!("[E2E] fail-closed: unresolvable external_media_address FQDN -> rejected {codes:?}, no 200"); + + set_global_pjsip_config(PjsipConfig::default()); +} diff --git a/crates/asterisk-sip/src/channel_driver.rs b/crates/asterisk-sip/src/channel_driver.rs index 78502bd..21221ee 100644 --- a/crates/asterisk-sip/src/channel_driver.rs +++ b/crates/asterisk-sip/src/channel_driver.rs @@ -587,12 +587,15 @@ impl ChannelDriver for SipChannelDriver { // Create SDP offer with a concrete, routable connection address // (external_media_address / routed interface — never 0.0.0.0, - // issue #56). - let sdp = SessionDescription::create_offer( - &crate::sdp::advertised_media_ip(self.local_addr, remote_addr), - rtp_port, - &channel_codecs, - ); + // issue #56). Fail closed (CP3): if a configured external_media_address + // FQDN does not resolve, do NOT offer a bogus/internal media address — + // fail the dial. + let Some(media_ip) = crate::sdp::advertised_media_ip(self.local_addr, remote_addr) else { + return Err(AsteriskError::Internal(format!( + "outbound {dest}: external_media_address did not resolve; refusing to offer a media address (fail-closed)" + ))); + }; + let sdp = SessionDescription::create_offer(&media_ip, rtp_port, &channel_codecs); sip_session.local_sdp = Some(sdp); let counter = next_channel_suffix(); diff --git a/crates/asterisk-sip/src/event_handler.rs b/crates/asterisk-sip/src/event_handler.rs index de28ec4..b4178ba 100644 --- a/crates/asterisk-sip/src/event_handler.rs +++ b/crates/asterisk-sip/src/event_handler.rs @@ -698,7 +698,22 @@ impl SipEventHandler { // offer address. let media_peer = crate::sdp_rtp::remote_rtp_endpoint(&remote_sdp) .unwrap_or(remote_addr); - let local_ip = crate::sdp::advertised_media_ip(session.local_addr, media_peer); + // Fail closed (CP3): if a configured external_media_address FQDN does + // not resolve, do NOT answer with a bogus/internal c=/o= address — + // reject the INVITE (488) rather than leak or blackhole the media. + let Some(local_ip) = crate::sdp::advertised_media_ip(session.local_addr, media_peer) + else { + let unique_id = channel.lock().unique_id.0.clone(); + store::deregister(&unique_id); + self.callid_map.write().remove(&call_id); + warn!(call_id = %call_id, "Fail-closed: external_media_address did not resolve; rejecting INVITE 488"); + if let Ok(resp) = request.create_response(488, "Not Acceptable Here") { + if self.may_send_invite_final(request, &resp) { + let _ = self.transport.send(&resp, remote_addr).await; + } + } + return None; + }; let rtp_result = match self.channel_driver.get() { Some(driver) => driver.allocate_rtp_session(session.local_addr.ip()).await, None => self @@ -1482,13 +1497,17 @@ impl SipEventHandler { /// advertises the channel's REAL bound RTP port and a routable connection /// address (never 0.0.0.0), with route/NAT selection targeting the offer's /// media endpoint and falling back to the signaling source. + /// Build the SDP answer for an in-dialog renegotiation (UPDATE/re-INVITE + /// with an offer). Returns `None` — fail closed (CP3) — if a configured + /// `external_media_address` FQDN does not resolve, so the caller rejects the + /// renegotiation rather than answering with a bogus/internal media address. async fn renegotiation_answer( &self, channel_name: &str, offer: &SessionDescription, local_addr: SocketAddr, remote_addr: SocketAddr, - ) -> SessionDescription { + ) -> Option { let channel_codecs = self .channel_driver .get() @@ -1502,8 +1521,8 @@ impl SipEventHandler { None => 10000, }; let media_peer = crate::sdp_rtp::remote_rtp_endpoint(offer).unwrap_or(remote_addr); - let local_ip = crate::sdp::advertised_media_ip(local_addr, media_peer); - SessionDescription::create_answer(offer, &local_ip, media_port, &channel_codecs) + let local_ip = crate::sdp::advertised_media_ip(local_addr, media_peer)?; + Some(SessionDescription::create_answer(offer, &local_ip, media_port, &channel_codecs)) } /// Handle an in-dialog **UPDATE** (RFC 3311). Two shapes: @@ -1617,9 +1636,19 @@ impl SipEventHandler { } } - let answer = self + // Fail closed (CP3): if external_media_address does not resolve, + // reject the UPDATE renegotiation (488) rather than answer with a + // bogus/internal media address. + let Some(answer) = self .renegotiation_answer(&channel_name, &offer, local_addr, remote_addr) - .await; + .await + else { + warn!(call_id = %call_id, "Fail-closed: external_media_address did not resolve; rejecting UPDATE 488"); + if let Ok(resp) = request.create_response(488, "Not Acceptable Here") { + let _ = self.send_server_response(&resp, remote_addr).await; + } + return; + }; { let mut cs = cs_arc.lock().await; cs.session.remote_sdp = Some(offer); @@ -2111,7 +2140,18 @@ impl SipEventHandler { let answer_sdp = if let Some(ref offer) = remote_sdp { let media_peer = crate::sdp_rtp::remote_rtp_endpoint(offer) .unwrap_or(remote_addr); - let local_ip = crate::sdp::advertised_media_ip(session.local_addr, media_peer); + // Fail closed (CP3): reject the re-INVITE (488) if a configured + // external_media_address FQDN does not resolve, rather than answer + // with a bogus/internal media address. + let Some(local_ip) = crate::sdp::advertised_media_ip(session.local_addr, media_peer) + else { + warn!(call_id = %call_id, "Fail-closed: external_media_address did not resolve; rejecting re-INVITE 488"); + let response = request.create_response(488, "Not Acceptable Here").ok()?; + if self.may_send_invite_final(request, &response) { + let _ = self.transport.send(&response, remote_addr).await; + } + return None; + }; let answer = SessionDescription::create_answer( offer, &local_ip, diff --git a/crates/asterisk-sip/src/sdp/mod.rs b/crates/asterisk-sip/src/sdp/mod.rs index a839db3..5ecff96 100644 --- a/crates/asterisk-sip/src/sdp/mod.rs +++ b/crates/asterisk-sip/src/sdp/mod.rs @@ -915,7 +915,16 @@ fn addr_type_for(addr: &str) -> &'static str { /// 2. the concrete local bind address, when there is one; /// 3. for an unspecified bind (`0.0.0.0`/`::`), the local interface the /// kernel routes toward the peer. -pub fn advertised_media_ip(local: std::net::SocketAddr, remote: std::net::SocketAddr) -> String { +/// +/// **Fail-closed (CP3):** returns `None` when a configured +/// `external_media_address` is an FQDN that does NOT resolve. In that case the +/// caller MUST reject the call setup rather than advertise an unresolved FQDN +/// or fall back to a leaky internal address in `c=`/`o=`. An IP-literal external +/// address, a resolvable FQDN, or any non-external path always returns `Some`. +pub fn advertised_media_ip( + local: std::net::SocketAddr, + remote: std::net::SocketAddr, +) -> Option { let (external, local_net) = match crate::pjsip_config::get_global_pjsip_config() { Some(cfg) => { // NAT config is transport-specific: use the transport whose bind @@ -1041,7 +1050,7 @@ fn advertised_media_ip_with( local_net: &[String], local: std::net::SocketAddr, remote: std::net::SocketAddr, -) -> String { +) -> Option { // 1. A configured NAT address wins for peers outside local_net. if let Some(ext) = external.filter(|e| !e.is_empty()) { let peer_is_local = local_net.iter().any(|cidr| { @@ -1050,13 +1059,18 @@ fn advertised_media_ip_with( .unwrap_or(false) }); if !peer_is_local { - return ext.to_string(); + // CP3: interpret the external address. An IP literal is emitted as + // is; an FQDN is resolved to an IP. On DNS failure we FAIL CLOSED + // (return None) — never emit an unresolved FQDN into c=/o= and never + // fall through to the internal/routed address below, which would + // leak the internal topology or advertise a bogus media address. + return resolve_external_media_addr(ext, remote.ip()); } } // 2. A concrete bound address is advertised as-is. if !local.ip().is_unspecified() { - return local.ip().to_string(); + return Some(local.ip().to_string()); } // 3. Bound to INADDR_ANY: let the kernel pick the interface it would @@ -1067,7 +1081,7 @@ fn advertised_media_ip_with( if sock.connect(remote).is_ok() { if let Ok(resolved) = sock.local_addr() { if !resolved.ip().is_unspecified() { - return resolved.ip().to_string(); + return Some(resolved.ip().to_string()); } } } @@ -1075,7 +1089,33 @@ fn advertised_media_ip_with( // 4. No route to the peer: keep the bind address (previous behaviour) // rather than inventing one. - local.ip().to_string() + Some(local.ip().to_string()) +} + +/// Interpret a configured `external_media_address` for SDP `c=`/`o=` (CP3). +/// +/// * An IP literal (v4 or v6) is emitted verbatim. +/// * An FQDN is resolved to an IP, preferring the peer's address family so an +/// IPv4 peer is not handed an IPv6 media address (and vice-versa), falling +/// back to any resolved address. +/// * **Fail closed:** returns `None` on DNS failure (or an empty resolution set) +/// so the caller rejects the offer/answer rather than advertising an +/// unresolved FQDN or a leaky internal address. +/// +/// The lookup is blocking, matching the existing blocking route-selection in +/// [`advertised_media_ip_with`]. Async resolution with TTL caching is a separate +/// hardening (M1/M-l) and out of this fail-closed scope. +fn resolve_external_media_addr(ext: &str, remote: std::net::IpAddr) -> Option { + if let Ok(ip) = ext.parse::() { + return Some(ip.to_string()); + } + use std::net::ToSocketAddrs; + let resolved: Vec = (ext, 0u16).to_socket_addrs().ok()?.collect(); + let pick = resolved + .iter() + .find(|sa| sa.is_ipv4() == remote.is_ipv4()) + .or_else(|| resolved.first())?; + Some(pick.ip().to_string()) } #[cfg(test)] @@ -1093,7 +1133,7 @@ mod tests { fn test_media_ip_concrete_bind_passes_through() { assert_eq!( advertised_media_ip_with(None, &[], sa("192.0.2.10:5060"), sa("198.51.100.7:5060")), - "192.0.2.10" + Some("192.0.2.10".to_string()) ); } @@ -1101,7 +1141,8 @@ mod tests { /// peer is used instead (loopback peer -> loopback source). #[test] fn test_media_ip_unspecified_resolves_routable_source() { - let ip = advertised_media_ip_with(None, &[], sa("0.0.0.0:5060"), sa("127.0.0.1:5062")); + let ip = advertised_media_ip_with(None, &[], sa("0.0.0.0:5060"), sa("127.0.0.1:5062")) + .expect("non-external path always resolves Some"); assert_ne!(ip, "0.0.0.0", "must never advertise INADDR_ANY (issue #56)"); assert_eq!(ip, "127.0.0.1", "loopback peer routes via loopback"); } @@ -1116,7 +1157,7 @@ mod tests { sa("0.0.0.0:5060"), sa("198.51.100.7:5060"), ), - "203.0.113.99" + Some("203.0.113.99".to_string()) ); } @@ -1131,7 +1172,7 @@ mod tests { sa("127.0.0.1:5060"), sa("127.0.0.1:5062"), ), - "127.0.0.1" + Some("127.0.0.1".to_string()) ); } @@ -1180,11 +1221,65 @@ mod tests { // Local bind 127.0.0.1 is NOT covered by the 192.0.2.50 transport: // its external address must not leak into this dialog's SDP. let ip = advertised_media_ip(sa("127.0.0.1:5060"), sa("127.0.0.1:5062")); - assert_eq!(ip, "127.0.0.1"); + assert_eq!(ip, Some("127.0.0.1".to_string())); // Reset the process-global config for other tests in this binary. set_global_pjsip_config(PjsipConfig::default()); } + // ---- external_media_address FQDN-vs-literal, fail-closed (CP3) ------- + + /// An IP-literal external_media_address is emitted verbatim. + #[test] + fn test_media_external_ip_literal_passes_through() { + assert_eq!( + resolve_external_media_addr("203.0.113.99", sa("198.51.100.7:5060").ip()), + Some("203.0.113.99".to_string()) + ); + assert_eq!( + resolve_external_media_addr("2001:db8::1", sa("[2001:db8::9]:5060").ip()), + Some("2001:db8::1".to_string()) + ); + } + + /// A resolvable FQDN external_media_address resolves to an IP literal + /// (localhost is guaranteed to resolve on any host). + #[test] + fn test_media_external_fqdn_resolves_to_ip() { + let resolved = resolve_external_media_addr("localhost", sa("127.0.0.1:5060").ip()) + .expect("localhost must resolve"); + let ip: std::net::IpAddr = resolved.parse().expect("resolved value must be an IP literal"); + assert!(ip.is_loopback(), "localhost must resolve to a loopback IP, got {ip}"); + } + + /// **Fail closed:** an unresolvable FQDN yields None so the caller rejects + /// the offer/answer instead of advertising a bogus/internal address. Uses + /// the RFC 6761 `.invalid` TLD, guaranteed never to resolve. + #[test] + fn test_media_external_fqdn_unresolvable_fails_closed() { + assert_eq!( + resolve_external_media_addr("no-such-host.invalid", sa("198.51.100.7:5060").ip()), + None, + "an unresolvable external_media_address FQDN must fail closed (None)" + ); + } + + /// End-to-end through advertised_media_ip_with: an external FQDN that will + /// not resolve, for a non-local peer, fails closed rather than falling + /// through to the internal bind address. + #[test] + fn test_media_ip_with_unresolvable_external_fails_closed() { + assert_eq!( + advertised_media_ip_with( + Some("no-such-host.invalid"), + &[], + sa("10.1.2.3:5060"), + sa("198.51.100.7:5060"), + ), + None, + "fail closed: must NOT fall back to the internal bind 10.1.2.3" + ); + } + // ---- advertised_signaling_hostport (New-3) -------------------------- /// No external signaling address: the bind host:port is advertised as-is. From 0b8715b5a0ff48f013f1912579289e0015cfc9f0 Mon Sep 17 00:00:00 2001 From: Repin Agent Date: Fri, 17 Jul 2026 03:57:37 -0600 Subject: [PATCH 2/2] =?UTF-8?q?fix(sip):=20address=20codex=20CP3=20review?= =?UTF-8?q?=20=E2=80=94=20reorder=20fail-closed,=20media=20transport=20sel?= =?UTF-8?q?ection,=20bounded=20DNS?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - F1 (MAJOR): resolve the SDP answer BEFORE mutating the media plane in the UPDATE and re-INVITE renegotiation paths, so a fail-closed rejection leaves the established call's media/hold/next-hop untouched instead of repointing the pump and then 488-ing the peer. - F2 (MAJOR): select the media transport that COVERS local_addr first, then read its external_media_address (same fix as CP2 F1 for signaling). Prevents a same-ip/wildcard transport's unresolvable external FQDN from rejecting a normal call on a transport that has no external address. Unit test added. - F3 (MAJOR): bound the blocking FQDN resolution with a 3s timeout on a scratch thread — a slow NSS/DNS lookup times out to None (fail closed) rather than stalling the serialized SIP control path. Full async+TTL DNS remains M1/M-l. - F4 (test): filter the fail-closed e2e response collection by Call-ID so an earlier call's retransmit cannot contaminate the assertion. --- .../tests/e2e_media_fail_closed.rs | 25 +++-- crates/asterisk-sip/src/event_handler.rs | 74 +++++++++------ crates/asterisk-sip/src/sdp/mod.rs | 94 +++++++++++++++---- 3 files changed, 137 insertions(+), 56 deletions(-) diff --git a/crates/asterisk-integration-tests/tests/e2e_media_fail_closed.rs b/crates/asterisk-integration-tests/tests/e2e_media_fail_closed.rs index 5ec27d9..0cce5e7 100644 --- a/crates/asterisk-integration-tests/tests/e2e_media_fail_closed.rs +++ b/crates/asterisk-integration-tests/tests/e2e_media_fail_closed.rs @@ -41,8 +41,14 @@ use tokio::net::UdpSocket; const EXTEN: &str = "100"; -/// Wait for a specific SIP status code, skipping provisional 1xx. -async fn recv_sip_status(sock: &UdpSocket, status: u16, budget: Duration) -> Option { +/// Wait for a specific SIP status code for `call_id`, skipping others (so a +/// retransmitted response from an earlier call cannot be mistaken for this one). +async fn recv_sip_status( + sock: &UdpSocket, + status: u16, + call_id: &str, + budget: Duration, +) -> Option { let deadline = Instant::now() + budget; let mut buf = [0u8; 4096]; while Instant::now() < deadline { @@ -50,7 +56,7 @@ async fn recv_sip_status(sock: &UdpSocket, status: u16, budget: Duration) -> Opt tokio::time::timeout(Duration::from_millis(300), sock.recv_from(&mut buf)).await { if let Ok(msg) = SipMessage::parse(&buf[..len]) { - if msg.status_code() == Some(status) { + if msg.status_code() == Some(status) && msg.call_id() == Some(call_id) { return Some(msg); } } @@ -59,8 +65,10 @@ async fn recv_sip_status(sock: &UdpSocket, status: u16, budget: Duration) -> Opt None } -/// Collect every distinct status code seen within the budget. -async fn collect_status_codes(sock: &UdpSocket, budget: Duration) -> Vec { +/// Collect every distinct status code seen for `call_id` within the budget +/// (responses for other Call-IDs are ignored, so an earlier call's retransmit +/// cannot contaminate this assertion). +async fn collect_status_codes(sock: &UdpSocket, call_id: &str, budget: Duration) -> Vec { let deadline = Instant::now() + budget; let mut buf = [0u8; 4096]; let mut seen = Vec::new(); @@ -69,6 +77,9 @@ async fn collect_status_codes(sock: &UdpSocket, budget: Duration) -> Vec { tokio::time::timeout(Duration::from_millis(300), sock.recv_from(&mut buf)).await { if let Ok(msg) = SipMessage::parse(&buf[..len]) { + if msg.call_id() != Some(call_id) { + continue; + } if let Some(code) = msg.status_code() { if !seen.contains(&code) { seen.push(code); @@ -179,7 +190,7 @@ async fn external_media_fqdn_fails_closed() { Some("media-fc-ok"), "positive control: an IP-literal external_media_address must be accepted" ); - let ok = recv_sip_status(&caller, 200, Duration::from_secs(5)) + let ok = recv_sip_status(&caller, 200, "media-fc-ok", Duration::from_secs(5)) .await .expect("positive control: IP-literal external must be answered 200"); let answer = SessionDescription::parse(&ok.body).expect("200 must carry SDP"); @@ -197,7 +208,7 @@ async fn external_media_fqdn_fails_closed() { accepted, None, "fail-closed: an unresolvable external_media_address FQDN must NOT be accepted" ); - let codes = collect_status_codes(&caller, Duration::from_secs(3)).await; + let codes = collect_status_codes(&caller, "media-fc-fail", Duration::from_secs(3)).await; assert!( !codes.contains(&200), "fail-closed: rustisk must NEVER send a 200 (with a bogus/internal c=) for an \ diff --git a/crates/asterisk-sip/src/event_handler.rs b/crates/asterisk-sip/src/event_handler.rs index b4178ba..567ee03 100644 --- a/crates/asterisk-sip/src/event_handler.rs +++ b/crates/asterisk-sip/src/event_handler.rs @@ -1624,31 +1624,33 @@ impl SipEventHandler { return; } - // Apply the offer to the media plane BEFORE answering, so the pump - // is renegotiated the instant our 200 goes out. - if let Some(driver) = self.channel_driver.get() { - if let Err(error) = driver.apply_inbound_offer(&channel_name, &offer).await { - warn!(call_id = %call_id, %error, "UPDATE media renegotiation failed"); - if let Ok(resp) = request.create_response(488, "Not Acceptable Here") { - let _ = self.send_server_response(&resp, remote_addr).await; - } - return; - } - } - - // Fail closed (CP3): if external_media_address does not resolve, - // reject the UPDATE renegotiation (488) rather than answer with a - // bogus/internal media address. + // Build the answer FIRST — before mutating the media plane — so a + // fail-closed rejection (CP3: external_media_address did not + // resolve) rejects the UPDATE with the ESTABLISHED call's media + // untouched, rather than leaving the pump repointed to a rejected + // address after the peer got a 488 (codex CP3 F1). let Some(answer) = self .renegotiation_answer(&channel_name, &offer, local_addr, remote_addr) .await else { - warn!(call_id = %call_id, "Fail-closed: external_media_address did not resolve; rejecting UPDATE 488"); + warn!(call_id = %call_id, "Fail-closed: external_media_address did not resolve; rejecting UPDATE 488 (media unchanged)"); if let Ok(resp) = request.create_response(488, "Not Acceptable Here") { let _ = self.send_server_response(&resp, remote_addr).await; } return; }; + + // Answer resolved: now apply the offer to the media plane, so the + // pump is renegotiated the instant our 200 goes out. + if let Some(driver) = self.channel_driver.get() { + if let Err(error) = driver.apply_inbound_offer(&channel_name, &offer).await { + warn!(call_id = %call_id, %error, "UPDATE media renegotiation failed"); + if let Ok(resp) = request.create_response(488, "Not Acceptable Here") { + let _ = self.send_server_response(&resp, remote_addr).await; + } + return; + } + } { let mut cs = cs_arc.lock().await; cs.session.remote_sdp = Some(offer); @@ -2101,6 +2103,28 @@ impl SipEventHandler { } } + // Fail closed (CP3) BEFORE touching the media plane: resolve the + // answer's media address first. If a configured external_media_address + // FQDN does not resolve, reject the re-INVITE (488) with the + // ESTABLISHED call's media untouched, rather than repointing/holding the + // pump and THEN 488-ing the peer (codex CP3 F1). + let resolved_local_ip = if let Some(ref offer) = remote_sdp { + let media_peer = crate::sdp_rtp::remote_rtp_endpoint(offer).unwrap_or(remote_addr); + match crate::sdp::advertised_media_ip(session.local_addr, media_peer) { + Some(ip) => Some(ip), + None => { + warn!(call_id = %call_id, "Fail-closed: external_media_address did not resolve; rejecting re-INVITE 488 (media unchanged)"); + let response = request.create_response(488, "Not Acceptable Here").ok()?; + if self.may_send_invite_final(request, &response) { + let _ = self.transport.send(&response, remote_addr).await; + } + return None; + } + } + } else { + None + }; + // ACTUALLY renegotiate the media plane (the load-bearing half of the // re-INVITE — previously this handler answered but never touched the // media session, so a codec/port change or hold was a sent-claim only): @@ -2138,20 +2162,10 @@ impl SipEventHandler { // issue #56). Route/NAT selection targets the re-INVITE's media // endpoint, falling back to the signaling source. let answer_sdp = if let Some(ref offer) = remote_sdp { - let media_peer = crate::sdp_rtp::remote_rtp_endpoint(offer) - .unwrap_or(remote_addr); - // Fail closed (CP3): reject the re-INVITE (488) if a configured - // external_media_address FQDN does not resolve, rather than answer - // with a bogus/internal media address. - let Some(local_ip) = crate::sdp::advertised_media_ip(session.local_addr, media_peer) - else { - warn!(call_id = %call_id, "Fail-closed: external_media_address did not resolve; rejecting re-INVITE 488"); - let response = request.create_response(488, "Not Acceptable Here").ok()?; - if self.may_send_invite_final(request, &response) { - let _ = self.transport.send(&response, remote_addr).await; - } - return None; - }; + // Media address already resolved fail-closed above (before any + // mutation); reuse it rather than resolving a second time. + let local_ip = resolved_local_ip + .expect("resolved_local_ip is Some whenever remote_sdp is Some"); let answer = SessionDescription::create_answer( offer, &local_ip, diff --git a/crates/asterisk-sip/src/sdp/mod.rs b/crates/asterisk-sip/src/sdp/mod.rs index 5ecff96..22e2459 100644 --- a/crates/asterisk-sip/src/sdp/mod.rs +++ b/crates/asterisk-sip/src/sdp/mod.rs @@ -927,20 +927,23 @@ pub fn advertised_media_ip( ) -> Option { let (external, local_net) = match crate::pjsip_config::get_global_pjsip_config() { Some(cfg) => { - // NAT config is transport-specific: use the transport whose bind - // covers `local` — exact ip+port, then exact ip, then a wildcard - // bind (which covers every interface). A transport bound to a - // DIFFERENT concrete address never donates its NAT config. - // (Dialogs don't yet carry their transport name; when they do, - // that binding should replace this bind-coverage lookup.) - let with_ext = |pred: &dyn Fn(&crate::pjsip_config::TransportConfig) -> bool| { - cfg.transports - .iter() - .find(|t| t.external_media_address.is_some() && pred(t)) - }; - let transport = with_ext(&|t| t.bind.ip() == local.ip() && t.bind.port() == local.port()) - .or_else(|| with_ext(&|t| t.bind.ip() == local.ip())) - .or_else(|| with_ext(&|t| t.bind.ip().is_unspecified())); + // Select the transport whose bind COVERS `local` first — exact + // ip+port, then exact ip, then a wildcard bind — and only THEN read + // its NAT config. Selecting by bind coverage (not by "has an + // external address") stops a same-ip/wildcard transport that + // happens to set an external address from donating it to a covering + // transport that set none — which, with fail-closed resolution, + // would otherwise REJECT a normal call on the transport that has no + // external address (codex CP3 F2). A transport bound to a DIFFERENT + // concrete address never covers `local`. (Dialogs don't yet carry + // their transport name; when they do, that binding should replace + // this bind-coverage lookup.) + let transport = cfg + .transports + .iter() + .find(|t| t.bind == local) + .or_else(|| cfg.transports.iter().find(|t| t.bind.ip() == local.ip())) + .or_else(|| cfg.transports.iter().find(|t| t.bind.ip().is_unspecified())); match transport { Some(t) => (t.external_media_address.clone(), t.local_net.clone()), None => (None, Vec::new()), @@ -1102,15 +1105,30 @@ fn advertised_media_ip_with( /// so the caller rejects the offer/answer rather than advertising an /// unresolved FQDN or a leaky internal address. /// -/// The lookup is blocking, matching the existing blocking route-selection in -/// [`advertised_media_ip_with`]. Async resolution with TTL caching is a separate -/// hardening (M1/M-l) and out of this fail-closed scope. +/// The FQDN lookup is bounded by a hard timeout so a slow NSS/DNS resolver +/// cannot stall the serialized SIP control path indefinitely — on timeout it +/// fails closed (`None`), the safe posture. Full async resolution with TTL +/// caching is the separate M1/M-l DNS hardening; this only bounds the blocking +/// call and preserves fail-closed. fn resolve_external_media_addr(ext: &str, remote: std::net::IpAddr) -> Option { if let Ok(ip) = ext.parse::() { return Some(ip.to_string()); } - use std::net::ToSocketAddrs; - let resolved: Vec = (ext, 0u16).to_socket_addrs().ok()?.collect(); + // Resolve on a scratch thread with a bounded receive: a slow lookup times + // out to None rather than blocking every other call on this stack. + let host = ext.to_string(); + let (tx, rx) = std::sync::mpsc::sync_channel::>(1); + std::thread::spawn(move || { + use std::net::ToSocketAddrs; + let resolved = (host.as_str(), 0u16) + .to_socket_addrs() + .map(|it| it.collect()) + .unwrap_or_default(); + let _ = tx.send(resolved); + }); + let resolved = rx + .recv_timeout(std::time::Duration::from_secs(3)) + .ok()?; let pick = resolved .iter() .find(|sa| sa.is_ipv4() == remote.is_ipv4()) @@ -1226,6 +1244,44 @@ mod tests { set_global_pjsip_config(PjsipConfig::default()); } + /// codex CP3 F2: a call on a transport with NO external_media_address must + /// get the normal (internal/routed) address — a same-ip transport whose + /// external FQDN is unresolvable must NOT be selected and fail the call + /// closed. Covering-transport-first selection prevents that cross-donation. + #[test] + fn test_media_ip_covering_transport_without_external_not_rejected() { + use crate::pjsip_config::{set_global_pjsip_config, PjsipConfig, TransportConfig}; + let base = |bind: &str, ext: Option<&str>| TransportConfig { + name: format!("t-{bind}"), + protocol: "udp".to_string(), + bind: bind.parse().unwrap(), + external_media_address: ext.map(|s| s.to_string()), + external_signaling_address: None, + external_signaling_port: None, + cert_file: None, + priv_key_file: None, + local_net: vec![], + }; + let cfg = PjsipConfig { + transports: vec![ + // The covering transport (exact ip 127.0.0.1) has NO external. + base("127.0.0.1:5060", None), + // A same-ip transport has an UNRESOLVABLE external FQDN. + base("127.0.0.1:5070", Some("no-such-host.invalid")), + ], + ..Default::default() + }; + set_global_pjsip_config(cfg); + // The call is on 127.0.0.1:5060 (no external) -> must resolve to the + // internal address, NOT fail closed by inheriting the :5070 FQDN. + assert_eq!( + advertised_media_ip(sa("127.0.0.1:5060"), sa("127.0.0.1:5062")), + Some("127.0.0.1".to_string()), + "a call on the no-external transport must not be rejected via a sibling's unresolvable FQDN" + ); + set_global_pjsip_config(PjsipConfig::default()); + } + // ---- external_media_address FQDN-vs-literal, fail-closed (CP3) ------- /// An IP-literal external_media_address is emitted verbatim.