908740 - Reject obviously bogus STUN and TURN candidates?

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Eric Rescorla (:ekr)]

One of the TURN servers I am using is having a bad hair day and decided to return candidates of the form 0.0.0.0:0. We happily accept these and treat them as the default candidate (per RFC 5245 S 4.1.4 (http://tools.ietf.org/html/rfc5245#section-4.1.4). However, when we try to setremote, it treats that as their being no media on the m-line and you get a negotiation failure.

Question: how aggressive should we be about checking for bogus candidates? Just this one? 1918 addresses?

Comment 1

[Byron Campen [:bwc]]

Initially, I think it makes sense to check for either 0.0.0.0 or port 0 in isolation is invalid. Going to bake this into the stun code in nICEr, since stuff like this is invalid from a protocol perspective.

Comment 2

[Byron Campen [:bwc]]

Attachment: Check mapped addr, xor mapped addr, and turn relay addr for obvious garbage. (Right now, in_addr is 0 or loopback, and in_port is 0)

Comment 3

[Byron Campen [:bwc]]

Checking for loopback since we already had a function for checking this laying around. Can remove if unpalatable.

Comment 4

[Byron Campen [:bwc]]

Attachment: Return R_BAD_DATA instead of 1.

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 802491 [details] [diff] [review]
Return R_BAD_DATA instead of 1.

Review of attachment 802491 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm with one nit.

Please report on testing against Google.

::: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c
@@ +407,5 @@
> +int nr_stun_transport_addr_check(nr_transport_addr* addr)
> +  {
> +    int r;
> +    if((r=nr_transport_addr_is_wildcard(addr)))
> +      return R_BAD_DATA;

Don't assign against r, since we're not using it. Here and below.

Comment 6

[Byron Campen [:bwc]]

Attachment: A little more tweaking.

Comment 7

[Byron Campen [:bwc]]

Pushed to try earlier today:

https://tbpl.mozilla.org/?tree=Try&rev=6f5aa8224a7c

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b69c26b9dc2

Please make sure your patches have usable commit messages in the future that describe what the patch is actually doing (not just restating the bug summary). Also, checkin-needed is a bug keyword, not a whiteboard status :)

Comment 9

[Eric Rescorla (:ekr)]

Ryan,

This actually is a pretty fair summary of the change. Byron could have written "localhost and wildcard" instead of "obviously bogus", I guess....

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b69c26b9dc2

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Note: the r+ wasn't moved forward to the "a little more tweaking" patch (nit-fixed patch), so the sheriff checked in the patch with r+.  This isn't a problem for uplift monday; it will at most add a warning for an unused var.  Please throw up a patch to get rid of the unused-var warnings.  If this gets uplifted, propose the "a little more tweaking" patch for uplift.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 802501 [details] [diff] [review]
A little more tweaking.

Carry forward r+

Comment 13

[Byron Campen [:bwc]]

I'm not seeing the difference between the final patch (with nit fixes) and either of the patches applied to inbound and central...

Comment 14

[Byron Campen [:bwc]]

Re-closing; the code on mc looks like this landed fine.