Skip to content

ENGDESK-49495: Restrict ICE auto-change to negotiated candidates only #513

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
baloeng wants to merge 1 commit into
base: jira-tel-6622
Choose a base branch
from
Open

ENGDESK-49495: Restrict ICE auto-change to negotiated candidates only #513

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 24 additions & 17 deletions src/switch_rtp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1681,29 +1681,36 @@ void switch_rtp_pvt_handle_ice(switch_rtp_t *rtp_session, switch_rtp_ice_t *ice,
channel = switch_core_session_get_channel(rtp_session->session);
}

ice->missed_count = 0;
ice->rready = 1;

if (cur_idx > -1) {
/* ENGDESK-49495: Only allow auto-change if source IP matches a known ICE candidate.
* This prevents DTLS path mismatch when STUN arrives from an IP not in the negotiated SDP.
* The STUN response is still sent (required by ICE), but we don't change the RTP/DTLS destination. */
if (cur_idx < 0) {

@damirn damirn Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cur_idx is assigned only in the candidate-search loop inside the else if (!do_adj …) → elapsed_adj > 1000 block — i.e. only on HIT 3/4/5. When do_adj is set earlier by HIT 1 (adj_window) or HIT 2 (switch_cmp_addr same-IP/diff-port, or use_candidate && !is_relay nomination), that loop never runs and cur_idx stays -1, so this guard unconditionally skips auto-change on those paths regardless of whether the source is a known candidate. That's very likely the ENGDESK-48822 reconnection regression. Suggest hoisting the candidate search so cur_idx is computed before this check on every adjust path.

switch_log_printf(SWITCH_CHANNEL_SESSION_LOG(rtp_session->session), SWITCH_LOG_WARNING,
"Skipping auto-change for %s stun/%s/dtls - source %s:%u is not a negotiated ICE candidate (current: %s:%u)\n",

@damirn damirn Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOG_WARNING fires on every skipped binding request; STUN retransmits sub-second, so a sustained unknown-IP source will spam the logs. Suggest DEBUG, or log once per source.

rtp_type(rtp_session), is_rtcp ? "rtcp" : "rtp",
from_host, from_port, host2, port2);
} else {
ice->missed_count = 0;

@damirn damirn Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed_count, rready, and last_ok (further down) were previously set on every do_adj; moving them inside the cur_idx≥0 branch means a peer that keeps sending STUN from an unknown IP never refreshes liveness → can trip ICE timeout/teardown even though we're still answering its binding requests. Suggest keeping rready/last_ok outside this branch and gating only chosen[] + switch_rtp_change_ice_dest.

ice->rready = 1;
ice->ice_params->chosen[ice->proto] = cur_idx;
}

switch_log_printf(SWITCH_CHANNEL_SESSION_LOG(rtp_session->session), SWITCH_LOG_NOTICE,
"Auto Changing %s stun/%s/dtls port from %s:%u to %s:%u idx:%d\n", rtp_type(rtp_session), is_rtcp ? "rtcp" : "rtp",
host2, port2,
from_host, from_port, cur_idx);
switch_log_printf(SWITCH_CHANNEL_SESSION_LOG(rtp_session->session), SWITCH_LOG_NOTICE,
"Auto Changing %s stun/%s/dtls port from %s:%u to %s:%u idx:%d\n", rtp_type(rtp_session), is_rtcp ? "rtcp" : "rtp",
host2, port2,
from_host, from_port, cur_idx);

switch_channel_set_flag(channel, CF_VIDEO_REFRESH_REQ);
switch_core_media_gen_key_frame(rtp_session->session);
switch_channel_set_flag(channel, CF_VIDEO_REFRESH_REQ);
switch_core_media_gen_key_frame(rtp_session->session);

switch_rtp_change_ice_dest(rtp_session, ice, from_host, from_port);
switch_rtp_change_ice_dest(rtp_session, ice, from_host, from_port);

ice->cand_responsive = is_responsive;
if (ice->cand_responsive) {
ice->initializing = 0;
}
ice->cand_responsive = is_responsive;
if (ice->cand_responsive) {
ice->initializing = 0;
}

ice->last_ok = now;
ice->last_ok = now;
}
}

if (!switch_rtp_test_flag(rtp_session, SWITCH_RTP_FLAG_ICE_NO_RESPONSE_IGNORED) || cmp || do_adj) {
Expand Down
Loading