Check all remote ICE candidates in Rtc::accepts()

[OxleyS]

Sometimes, a WebRTC client will send e.g. DTLS traffic using a candidate pair that has been found successful, but is not the currently nominated pair. In my local tests, this only happens as the connection is being established. My guess is a race condition - if multiple pairs are nominated in sequence, it's possible for traffic to come in via an old nomination before the new one reaches the remote client and takes effect.

If this happens, Rtc::accepts() will reject the traffic due to not being from the current nominated pair. I noticed this while working with Firefox, but it may happen in other WebRTC clients as well. I also noticed that occasionally Firefox will get stuck using the same not-nominated pair to retry DTLS that failed in this manner (although I imagine such behavior is not intended).

To minimize the performance impact, we continue to eagerly check against the nominated pair, and only fallback to checking all candidates as a last resort.

[algesten]

A good start.

One small comment.

We also need to think through the security consequences here. Does this mean we accept traffic for remote candidates that have not been verified with a STUN request/response? Does that matter? The SCTP and RTP traffic will be protected by the DTLS fingerprint, so maybe it's not a big deal 🤔

Wonder how libWebRTC does this? Maybe they only accept unverified remote candidates for a short while?

[OxleyS]

That's true, it would be a good idea to sanity check candidates first. I can also try digging into libwebrtc source a bit and see if I find anything.

[OxleyS]

I digged into libwebrtc source a bit. It seems they have similar logic for deciding what to forward, complete with a fast path for the selected one. Here, Connection basically boils down to a candidate pair, with one created for every non-pruned local-remote combination.

The "non-pruned" bit may be important to us as well, in the spirit of the security consequences you mentioned. libwebrtc seems to prune candidates whose networks have disappeared, or when a re-gather is initiated. str0m's equivalent seems to be discarded?

[OxleyS]

We could then take one of a few approaches:

1. Check all non-discarded remote candidates for a matching source address.
2. Check all candidate pairs with CheckState::Succeeded for a remote candidate with a matching source address (this automatically excludes discarded candidates).
3. The second solution, but also check the local candidate against the destination address.

Which of these do you feel is most appropriate?

[algesten]

I think 2 is a good choice. If it succeeded, we have got a successful request-response. That might match libWebRTC logic here.

[algesten]

Like it!

Thanks!