stun: fix ice role conflict handling

This patch fixes the role conflict handling in stun ICE usage,
according to RFC 5245, by adding including missing cases in the
test. The role switch not only depends of the comparison of the
stun ice-controlling/controlled attrib with the agent tie breaker
value, but it also depends on the current role of the agent.

This patch also changes the value returned by
stun_usage_ice_conncheck_create_reply() when a role conflict exists
but doesn't change the role of the agent, causing an error stun
response. Previously, this case could not be differenciated by the
caller from a case with no role conflict. Now by examinating the
return value, and whether the control param changed, the caller
can check the four possibles situations. The stun test suite is
updated to match this change.

Differential Revision: https://phabricator.freedesktop.org/D873

2 files changed, 56 insertions, 7 deletions

diff --git a/stun/tests/test-conncheck.c b/stun/tests/test-conncheck.c
index 610d43a..92b947c 100644
--- a/stun/tests/test-conncheck.c
+++ b/stun/tests/test-conncheck.c
@@ -213,7 +213,7 @@ int main (void)
 
   addr.ip4.sin_family = AF_INET;
 
-  /* Lost role conflict */
+  /* Role conflict, controlling + ICE-CONTROLLING, switching controlled */
   assert (stun_agent_init_request (&agent, &req, req_buf, sizeof(req_buf), STUN_BINDING));
   val = stun_message_append64 (&req, STUN_ATTRIBUTE_ICE_CONTROLLING, tie + 1);
   assert (val == STUN_MESSAGE_RETURN_SUCCESS);
@@ -223,7 +223,6 @@ int main (void)
   rlen = stun_agent_finish_message (&agent, &req, pass, pass_len);
   assert (rlen > 0);
 
-
   len = sizeof (resp_buf);
   control = true;
   val2 = stun_usage_ice_conncheck_create_reply (&agent, &req,
@@ -236,7 +235,7 @@ int main (void)
           stun_agent_default_validater, validater_data) == STUN_VALIDATION_SUCCESS);
   assert (stun_message_get_class (&resp) == STUN_RESPONSE);
 
-  /* Won role conflict */
+  /* Role conflict, controlled + ICE-CONTROLLED, switching controlling */
   assert (stun_agent_init_request (&agent, &req, req_buf, sizeof(req_buf), STUN_BINDING));
   val = stun_message_append64 (&req, STUN_ATTRIBUTE_ICE_CONTROLLED, tie - 1);
   assert (val == STUN_MESSAGE_RETURN_SUCCESS);
@@ -251,15 +250,60 @@ int main (void)
   val2 = stun_usage_ice_conncheck_create_reply (&agent, &req,
       &resp, resp_buf, &len, &addr.storage,
       sizeof (addr.ip4), &control, tie, STUN_USAGE_ICE_COMPATIBILITY_RFC5245);
-  assert (val2 == STUN_USAGE_ICE_RETURN_SUCCESS);
+  assert (val2 == STUN_USAGE_ICE_RETURN_ROLE_CONFLICT);
   assert (len > 0);
-  assert (control == false);
+  assert (control == true);
+  assert (stun_agent_validate (&agent, &resp, resp_buf, len,
+          stun_agent_default_validater, validater_data) == STUN_VALIDATION_SUCCESS);
+  assert (stun_message_get_class (&resp) == STUN_RESPONSE);
+
+  /* Role conflict, controlling + ICE-CONTROLLING, staying controlling */
+  assert (stun_agent_init_request (&agent, &req, req_buf, sizeof(req_buf), STUN_BINDING));
+  val = stun_message_append64 (&req, STUN_ATTRIBUTE_ICE_CONTROLLING, tie - 1);
+  assert (val == STUN_MESSAGE_RETURN_SUCCESS);
+  val = stun_message_append_string (&req, STUN_ATTRIBUTE_USERNAME,
+     (char *) ufrag);
+  assert (val == STUN_MESSAGE_RETURN_SUCCESS);
+  rlen = stun_agent_finish_message (&agent, &req, pass, pass_len);
+  assert (rlen > 0);
+
+  len = sizeof (resp_buf);
+  control = true;
+  val2 = stun_usage_ice_conncheck_create_reply (&agent, &req,
+      &resp, resp_buf, &len, &addr.storage,
+      sizeof (addr.ip4), &control, tie, STUN_USAGE_ICE_COMPATIBILITY_RFC5245);
+  assert (val2 == STUN_USAGE_ICE_RETURN_ROLE_CONFLICT);
+  assert (len > 0);
+  assert (control == true);
   assert (stun_agent_validate (&agent, &resp, resp_buf, len,
           stun_agent_default_validater, validater_data) == STUN_VALIDATION_SUCCESS);
   assert (stun_message_get_class (&resp) == STUN_ERROR);
   stun_message_find_error (&resp, &code);
   assert (code == STUN_ERROR_ROLE_CONFLICT);
 
+  /* Role conflict, controlled + ICE-CONTROLLED, staying controlling */
+  assert (stun_agent_init_request (&agent, &req, req_buf, sizeof(req_buf), STUN_BINDING));
+  val = stun_message_append64 (&req, STUN_ATTRIBUTE_ICE_CONTROLLED, tie + 1);
+  assert (val == STUN_MESSAGE_RETURN_SUCCESS);
+  val = stun_message_append_string (&req, STUN_ATTRIBUTE_USERNAME,
+      (char *) ufrag);
+  assert (val == STUN_MESSAGE_RETURN_SUCCESS);
+  rlen = stun_agent_finish_message (&agent, &req, pass, pass_len);
+  assert (rlen > 0);
+
+  len = sizeof (resp_buf);
+  control = false;
+  val2 = stun_usage_ice_conncheck_create_reply (&agent, &req,
+      &resp, resp_buf, &len, &addr.storage,
+      sizeof (addr.ip4), &control, tie, STUN_USAGE_ICE_COMPATIBILITY_RFC5245);
+  assert (val2 == STUN_USAGE_ICE_RETURN_ROLE_CONFLICT);
+  assert (len > 0);
+  assert (control == false);
+  assert (stun_agent_validate (&agent, &resp, resp_buf, len,
+          stun_agent_default_validater, validater_data) == STUN_VALIDATION_SUCCESS);
+  assert (stun_message_get_class (&resp) == STUN_ERROR);
+  stun_message_find_error (&resp, &code);
+  assert (code == STUN_ERROR_ROLE_CONFLICT);
 
   return 0;
 }
diff --git a/stun/usages/ice.c b/stun/usages/ice.c
index b9dcc54..a7d0d19 100644
--- a/stun/usages/ice.c
+++ b/stun/usages/ice.c
@@ -270,7 +270,12 @@ stun_usage_ice_conncheck_create_reply (StunAgent *agent, StunMessage *req,
      */
     stun_debug ("STUN Role Conflict detected:");
 
-    if (tie < q)
+    /* According to ICE RFC 5245, section 7.2.1.1, we consider the four
+     * possible cases when a role conflict is detected: two cases are
+     * resolved by switching role locally, and the two other cases are
+     * handled by responding with a STUN error.
+     */
+    if ((tie < q && *control) || (tie >= q && !*control))
     {
       stun_debug (" switching role from \"controll%s\" to \"controll%s\"",
            *control ? "ing" : "ed", *control ? "ed" : "ing");
@@ -282,7 +287,7 @@ stun_usage_ice_conncheck_create_reply (StunAgent *agent, StunMessage *req,
       stun_debug (" staying \"controll%s\" (sending error)",
            *control ? "ing" : "ed");
       err (STUN_ERROR_ROLE_CONFLICT);
-      return STUN_USAGE_ICE_RETURN_SUCCESS;
+      return STUN_USAGE_ICE_RETURN_ROLE_CONFLICT;
     }
   } else {
     if (stun_message_find64 (req, *control ? STUN_ATTRIBUTE_ICE_CONTROLLED