Skip to content
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.

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

fix: don't panic on STUN requests for unknown interfaces #493

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
69 changes: 48 additions & 21 deletions src/ice/agent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -842,14 +842,14 @@ impl IceAgent {
/// Handles an incoming STUN message.
///
/// Will not be used if [`IceAgent::accepts_message`] returns false.
pub fn handle_packet(&mut self, now: Instant, packet: StunPacket) {
pub fn handle_packet(&mut self, now: Instant, packet: StunPacket) -> bool {
trace!("Handle receive: {:?}", &packet.message);

// Regardless of whether we have remote_creds at this point, we can
// at least check the message integrity.
if !self.accepts_message(&packet.message) {
debug!("Message not accepted");
return;
return false;
}

if packet.message.is_binding_request() {
Expand All @@ -864,6 +864,8 @@ impl IceAgent {
});

// TODO handle unsuccessful responses.

true
}

/// Provide the current time to the [`IceAgent`].
Expand Down Expand Up @@ -1169,25 +1171,28 @@ impl IceAgent {
self.remote_candidates.len() - 1
};

let local_idx = self
.local_candidates
.iter()
.enumerate()
.find(|(_, v)| {
// The local candidate will be
// either a host candidate (for cases where the request was not received
// through a relay) or a relayed candidate (for cases where it is
// received through a relay). The local candidate can never be a
// server-reflexive candidate.
matches!(v.kind(), CandidateKind::Host | CandidateKind::Relayed)
&& v.addr() == req.destination && v.proto() == req.proto
})
// Receiving traffic for an IP address that neither is a HOST nor RELAY is a configuration
// fault. We need to be aware of the interfaces that the ice agent is connected to.
.expect(
"STUN request for socket that is neither a host nor a relay candidate. This is a config error.",
)
.0;
let local_idx = match self.local_candidates.iter().enumerate().find(|(_, v)| {
// The local candidate will be
// either a host candidate (for cases where the request was not received
// through a relay) or a relayed candidate (for cases where it is
// received through a relay). The local candidate can never be a
// server-reflexive candidate.
Comment on lines +1175 to +1179
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am wondering if this is still true with our changes in #455? Shouldn't we instead also check for server-reflexive candidates and compare the base instead?

Choose a reason for hiding this comment

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

The .expect here comes from the assumption you're going to configure all ways you are going to receive traffic. server-reflexive (and peer-reflexive) are secondary (not configured), they are discovered and more crucially always received via a primary (host or relayed).

The logic below would be very strange if the local_idx can be a secondary (reflexive). It would imply we can make a NAT - NAT like a server reflexive of a server reflexive.

matches!(v.kind(), CandidateKind::Host | CandidateKind::Relayed)
&& v.addr() == req.destination
&& v.proto() == req.proto
}) {
Some((i, _)) => i,
None => {
// Receiving traffic for an IP address that neither is a HOST nor RELAY is most likely a configuration fault where the user forgot to add a candidate for the local interface.
// We are network-connected application so we need to handle this gracefully: Log a message and discard the packet.

debug!(
"Discarding STUN request on unknown interface: {}",
req.destination
);
return;
}
};

let maybe_pair = self
.candidate_pairs
Expand Down Expand Up @@ -1893,6 +1898,28 @@ mod test {
assert!(stun_message.is_successful_binding_response());
}

#[test]
pub fn discards_packet_from_unknown_candidate() {
let mut agent = IceAgent::new();
let remote_creds = IceCreds::new();
agent.set_remote_credentials(remote_creds.clone());

let request =
make_serialized_binding_request(&agent.local_credentials, &remote_creds, false, 0);

agent.handle_packet(
Instant::now(),
StunPacket {
proto: Protocol::Udp,
source: ipv4_1(),
destination: ipv4_2(),
message: StunMessage::parse(&request).unwrap(),
},
);

assert!(agent.poll_transmit().is_none());
}

fn make_serialized_binding_request(
local_creds: &IceCreds,
remote_creds: &IceCreds,
Expand Down
10 changes: 5 additions & 5 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1618,15 +1618,15 @@ impl Rtc {
self.peer_bytes_rx += bytes_rx as u64;

match r.contents.inner {
Stun(stun) => self.ice.handle_packet(
now,
io::StunPacket {
Stun(stun) => {
let packet = io::StunPacket {
proto: r.proto,
source: r.source,
destination: r.destination,
message: stun,
},
),
};
self.ice.handle_packet(now, packet);
}
Dtls(dtls) => self.dtls.handle_receive(dtls)?,
Rtp(rtp) => self.session.handle_rtp_receive(now, rtp),
Rtcp(rtcp) => self.session.handle_rtcp_receive(now, rtcp),
Expand Down
Loading