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

[thomaseizinger]

Resolves: #473.

[thomaseizinger]

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?

[lolgesten]

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.

[lolgesten]

Why does this scenario not use .accepts_message() to test whether the ICE agent want the packet?

While I accept we don't have to panic, I still believe the user should configure the ICE agent correctly. Silently discarding random UDP packets means it's easy to introduce bugs and not notice a misconfiguration.

[thomaseizinger]

While I accept we don't have to panic, I still believe the user should configure the ICE agent correctly. Silently discarding random UDP packets means it's easy to introduce bugs and not notice a misconfiguration.

We do use accepts_message in our code and I can still trigger this panic. That being said, we don't yet have https://github.com/algesten/str0m/pull/488in our fork so that might change things?

[thomaseizinger]

accepts_message only checks the message and doesn't have access to the destination SocketAddr so it cannot check whether it has a candidate for that:

str0m/src/ice/agent.rs

Line 767 in 3c5edee

pub fn accepts_message(&self, message: &StunMessage<'_>) -> bool {

[algesten]

We do use accepts_message in our code and I can still trigger this panic. That being said, we don't yet have https://github.com/algesten/str0m/pull/488in our fork so that might change things?

Interesting! That would be good to know. It makes a big difference to me if accepts_message says "yes" and handle_message says "panic"!

Spontaneously it seems these should be in lockstep.

[thomaseizinger]

We do use accepts_message in our code and I can still trigger this panic. That being said, we don't yet have algesten/str0m/pull/488in our fork so that might change things?

Interesting! That would be good to know. It makes a big difference to me if accepts_message says "yes" and handle_message says "panic"!

Spontaneously it seems these should be in lockstep.

See the comment above. i think we'd have to change accepts_message to accept a StunPacket to make it say "no" to packets from unknown interfaces.

Note that the way this panic gets triggered for us is:

1. We invalidate all other candidates as soon as we have a working path
2. We want to delay sending of relay candidates by ~1s to first check if we can hole-punch

Combinding (1) and (2) leads to a race condition where we might have already invalidated a candidate but the remote wants to still send use STUN requests.

[algesten]

Right. Hm. Maybe we should just add the interface check to the accepts message.

Neither of "I accept it and discard it", or "I accept it and panic" seems entirely right.

[thomaseizinger]

My take on it is:

• IceAgent::handle_packet must not panic, regardless what data you feed it. IMO, one of the aspects of a SANS-IO design is that you can (directly) feed it unstructured input from the network. Thus, it must not panic or otherwise we have a DoS attack vector. I think the correct solution for this fn is to discard the message.
• IceAgent::accepts_message is useful when multiple agents are multiplexed on a single socket. IMO there isn't a lot of value in having this function (we've discussed this before but I'll mention it for completeness). Processing a message and checking whether we want to do so requires very similar code. Code that must not diverge. To me, it seems like we could just do whatever we do in accepts_message at the very top of handle_message and return true | false from it, indicating whether we've handled the message. It might not hold true for the entire Rtc instance but at least for the IceAgent, doing the actual processing should be very cheap.

Perhaps as a compromise, we could make accepts_message pub(crate) such that the Rtc instance can still call it but the public API of IceAgent doesn't expose it? Then it wouldn't be a silent discard but just returning false to indicate that we didn't handle it.

[thomaseizinger]

We can also (additionally) align the two APIs such that the entire packet can be passed to accepts_message and checked early.

[algesten]

I'm good merging this now, if you think it's ok?

[thomaseizinger]

If we return false now, should we also change stun_server_handle_message to return false in case we discard it based on an unknown candidate?

[algesten]

If we return false now, should we also change stun_server_handle_message to return false in case we discard it based on an unknown candidate?

I tried that but didn't like where it was going. Places like if candidate.discarded { return are not clear whether it's true or false.

I decided that rather than muddle the semantics, the bool is exactly that of accepts_message.

[thomaseizinger]

I decided that rather than muddle the semantics, the bool is exactly that of accepts_message.

Okay, in that case, isn't that commit unrelated to this change? I am not sure how atomic you want to be on your PRs but it could be done separately in that case.

Otherwise, I am happy with this going in :)

[lolgesten]

Let's merge it! I'll reference the PR twice in the changelog.