Closed Bug 908740 Opened 6 years ago Closed 6 years ago

Reject obviously bogus STUN and TURN candidates?

Categories

(Core :: WebRTC: Networking, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: ekr, Assigned: bwc)

Details

Attachments

(1 file, 2 obsolete files)

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?
Assignee: nobody → docfaraday
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.
Checking for loopback since we already had a function for checking this laying around. Can remove if unpalatable.
Attached patch Return R_BAD_DATA instead of 1. (obsolete) — Splinter Review
Attachment #802480 - Attachment is obsolete: true
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.
Attachment #802491 - Flags: review+
Attachment #802491 - Attachment is obsolete: true
Pushed to try earlier today:

https://tbpl.mozilla.org/?tree=Try&rev=6f5aa8224a7c
Whiteboard: [checkin-needed]
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 :)
Whiteboard: [checkin-needed]
Ryan,

This actually is a pretty fair summary of the change. Byron could have written "localhost and wildcard" instead of "obviously bogus", I guess....
https://hg.mozilla.org/mozilla-central/rev/9b69c26b9dc2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 802501 [details] [diff] [review]
A little more tweaking.

Carry forward r+
Attachment #802501 - Flags: review+
I'm not seeing the difference between the final patch (with nit fixes) and either of the patches applied to inbound and central...
Re-closing; the code on mc looks like this landed fine.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.