Fix panic on STUN binding failure while multiplexing #488

OxleyS · 2024-03-21T14:17:04Z

An unrelated bug I had in my application was causing STUN to fail, and a Class::Failure message to come back. Because like in the chat example, I'm still using a single multiplexed UDP socket, this message first went to the Rtc::accepts() of a separate client that had not negotiated, which caused it to panic with "Remote ICE Credentials".

This is the same underlying panic as in #461, but this time the logic was allowed to fall through to the stun_credentials() call because we were only doing the preceding candidate-pair test for Class::Success responses, not Class::Failure.

I wanted to add a test for this, but with the current field visibility on StunMessage, I couldn't create a failure message in a test. I wasn't sure if adding a helper to impl StunMessage was appropriate.

algesten · 2024-03-21T14:18:54Z

Which panic are you hitting?

OxleyS · 2024-03-21T14:22:59Z

This one, because it is a STUN response we hit the else branch here, which expects remote credentials.

algesten · 2024-03-21T14:32:24Z

I think the correct fix would be:

--- a/src/ice/agent.rs
+++ b/src/ice/agent.rs
@@ -799,6 +799,9 @@ impl IceAgent {
                 trace!("Message rejected, transaction ID does not belong to any of our candidate pairs");
                 return false;
             }
+        } else {
+            trace!("Message reject, it is not a succesful binding response");
+            return false;
         }
 
         let (_, password) = self.stun_credentials(!message.is_response());

OxleyS · 2024-03-21T16:20:20Z

That patch would end up rejecting everything that's not a successful binding response, since all other message types, including things like binding requests, would fall through to that else case as well.

I believe the effect you intended was to just reject all non-success binding responses? That would also fix the panic, although arguably IceAgent should be accepting all traffic intended for it, even if it doesn't react to failed responses in any special way.

(Aside: Class::Indication and Class::Unknown wouldn't trigger panics with either of our solutions).

algesten · 2024-03-21T16:22:32Z

Taking a step back.

accept should, as you say, accept all traffic targeted at this Rtc instance.

OxleyS · 2024-03-25T12:34:51Z

I think we have our bases covered here then - STUN requests get handled as before, and now both STUN success and failure gets accepted if it corresponds to a STUN request from us (or rejected before the credentials check if not).

algesten · 2024-03-26T08:40:22Z

I still don't like this change.

What kind of responses are we talking about that we suddenly accept? What about middleboxes? We should be very specific with what we accept. Enumerate the cases.

OxleyS · 2024-03-26T13:52:18Z

I believe this enumeration is exhaustive:

Method::Binding, Class::Request: Unchanged, rejects on a local/remote ufrag mismatch and then checks integrity. Cannot panic because it does not require remote credentials to integrity-check.
Method::Binding, Class::Success: Unchanged, rejects if the transaction ID matches no binding request from us, then checks integrity. If the transaction ID matches, the integrity-check would panic without remote credentials, but we do not generate binding requests without remote credentials, so panic is effectively impossible.
Any method, Class::Failure: Changed, previous behavior would only check integrity, panicking if remote credentials have not been set. New behavior matches the Class::Success case.
Method::Unknown, Class::Success: Changed, both previous and new behavior match the Class::Failure case.
Method::Unknown, Class::Request: Unchanged, only checks integrity. Cannot panic because it does not require remote credentials to integrity-check.
Any method, Class::Indication or Class::Unknown: Unchanged, only checks integrity. Cannot panic because it does not require remote credentials to integrity-check.

This PR was aimed at fixing the (Method::Binding, Class::Failure) case specifically, but it seems both (Method::Unknown, Class::Success) and (Method::Unknown, Class::Failure) had the same panic bug. This PR ~~unintentionally~~ incidentally prevents these two from panicking as well, by subjecting them to the same transaction-ID-match test as a binding success would be. It's hard to have a good answer for what to do with Method::Unknown, but this seems like a reasonable-enough way to handle them.

Considering the confusion that seems to exist around the exact behavior of accepts() (and I made several mistakes in the process of putting the above enumeration together!), perhaps we should consider rewriting this method to use an exhaustive match, even if it means we can't use is_response() and similar utilities.

algesten · 2024-03-26T14:04:49Z

Considering the confusion that seems to exist around the exact behavior of accepts() (and I made several mistakes in the process of putting the above enumeration together!), perhaps we should consider rewriting this method to use an exhaustive match, even if it means we can't use is_response() and similar utilities.

Agree. I also think this function should default to false at the end and explicitly enumerate the cases where we do accept the message. So it's the opposite of today really.

OxleyS · 2024-03-26T15:20:49Z

Okay, I'll take a stab at doing that tomorrow.

…r than potentially panicking

src/ice/agent.rs

OxleyS · 2024-03-27T05:41:44Z

src/ice/agent.rs

+        let method = message.method();
+        let class = message.class();
+        match (method, class) {
+            (StunMethod::Binding, StunClass::Request | StunClass::Indication) => {


RFC 5389 indicates that requests and indications should be validated in a similar manner.

OxleyS · 2024-03-27T05:42:45Z

src/ice/agent.rs

+            (StunMethod::Binding, StunClass::Unknown) => {
+                // Without a known class, it's impossible to know how to validate the message
+                trace!("Message rejected, unknown STUN class");
+                false
+            }
+            (StunMethod::Unknown, _) => {
+                // Without a known method, it's impossible to know how to validate the message
+                trace!("Message rejected, unknown STUN method");
+                false
+            }


These are behavior changes from my previous enumeration. Since processing is so method and class-dependent, not knowing either means we can't really know how to even verify that this message is valid, or for this instance.

src/io/mod.rs

src/io/stun.rs

algesten

Looks great!

Thank you for doing this!

Small question

src/ice/agent.rs

algesten · 2024-03-27T07:58:26Z

src/ice/agent.rs

            }
-        }
+            (StunMethod::Binding, StunClass::Success | StunClass::Failure) => {
+                let belongs_to_a_candidate_pair = self


Maybe we should check nominated pair here first, to make a less compute intensive happy path?

Ah, yes, you're right! We even talked about that in the other PR, ~~I guess it got lost amongst the other discussion happening there.~~ Taking a second look, the discussion in the other PR was for Rtc::accepts(), not IceAgent::accepts_message(), and that fast path is indeed being used there.

I'll do that.

I've looked deeper into the code, and while adding this fast path is possible, it's surprisingly risky and would be better left to a separate PR, if we deem it necessary at all.

The issue is that our nominated pair IceAgent.nominated_send is not an index into the pairs array, but rather a PairId. Resolving that to an index would require a linear scan of the pairs array anyway, negating any benefits of checking it first. Storing an index instead would be very difficult because we sort pairs by priority and prune failed pairs often, making the bookkeeping very error-prone.

Alright! Thanks for checking!

OxleyS force-pushed the fix-stun-failure-panic branch 2 times, most recently from 15efbc4 to cfd034b Compare March 25, 2024 12:31

OxleyS added 2 commits March 27, 2024 14:37

IceAgent now properly rejects unknown binding failure messages, rathe…

63be7c0

…r than potentially panicking

Refactored IceAgent.accepts_message() to use an exhaustive match

008d9e8

OxleyS force-pushed the fix-stun-failure-panic branch from cfd034b to 008d9e8 Compare March 27, 2024 05:38

OxleyS commented Mar 27, 2024

View reviewed changes

algesten requested changes Mar 27, 2024

View reviewed changes

lolgesten approved these changes Mar 27, 2024

View reviewed changes

algesten approved these changes Mar 27, 2024

View reviewed changes

algesten merged commit ddbbe2f into algesten:main Mar 27, 2024
22 checks passed

OxleyS deleted the fix-stun-failure-panic branch March 27, 2024 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix panic on STUN binding failure while multiplexing #488

Fix panic on STUN binding failure while multiplexing #488

OxleyS commented Mar 21, 2024

algesten commented Mar 21, 2024

OxleyS commented Mar 21, 2024

algesten commented Mar 21, 2024

OxleyS commented Mar 21, 2024

algesten commented Mar 21, 2024

OxleyS commented Mar 25, 2024

algesten commented Mar 26, 2024

OxleyS commented Mar 26, 2024

algesten commented Mar 26, 2024

OxleyS commented Mar 26, 2024

OxleyS Mar 27, 2024

OxleyS Mar 27, 2024

algesten left a comment

algesten Mar 27, 2024

OxleyS Mar 27, 2024 •

edited

Loading

OxleyS Mar 27, 2024

lolgesten Mar 27, 2024

Fix panic on STUN binding failure while multiplexing #488

Fix panic on STUN binding failure while multiplexing #488

Conversation

OxleyS commented Mar 21, 2024

algesten commented Mar 21, 2024

OxleyS commented Mar 21, 2024

algesten commented Mar 21, 2024

OxleyS commented Mar 21, 2024

algesten commented Mar 21, 2024

OxleyS commented Mar 25, 2024

algesten commented Mar 26, 2024

OxleyS commented Mar 26, 2024

algesten commented Mar 26, 2024

OxleyS commented Mar 26, 2024

OxleyS Mar 27, 2024

Choose a reason for hiding this comment

OxleyS Mar 27, 2024

Choose a reason for hiding this comment

algesten left a comment

Choose a reason for hiding this comment

algesten Mar 27, 2024

Choose a reason for hiding this comment

OxleyS Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

OxleyS Mar 27, 2024

Choose a reason for hiding this comment

lolgesten Mar 27, 2024

Choose a reason for hiding this comment

OxleyS Mar 27, 2024 •

edited

Loading