Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reject ICE servers with an authority component or / #2998

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dontcallmedom
Copy link
Member

@dontcallmedom dontcallmedom commented Sep 4, 2024

Complete integration of URL parser from #2853
see also #2997 (comment) This aligns with the constraints set in the respective RFC (and thus with the current WebRTC Rec)


Preview | Diff

Complete integration of URL parser from #2853
see also #2997 (comment)
This aligns with the constraints set in the respective RFC (and thus with the current WebRTC Rec)
@alvestrand
Copy link
Contributor

The writing of tests has shown that there are a number of URIs that are "legal" according to the URL parser algorithm, but do not conform to the ABNF for:

  • Stun: RFC 7046
  • Turn: RFC 7065

We should make a decision on whether these URIs should be rejected or not; authority components are just one example of such nonconformance.

@dontcallmedom
Copy link
Member Author

dontcallmedom commented Sep 5, 2024

Validating the test stun URLs against the RFC7064 ABNF, it shows the following RFC-invalid values:

  "stun:example\t.\norg",
  "stun:[2001::1]",
  "stun:example(&!$)*+-.;_=~☺org",
  "stun:example.net:\n",
  "stun:GOO​⁠goo.com",
  "\u0000\u001b\u0004\u0012 stun:example.com\u001f \r ",
  "stun:www.foo。bar.com",
  "stun:Go.com",
  "stun:你好你好",
  "stun:faß.ExAmPlE",
  "stun:0Xc0.0250.01",
  "stun:ho\tst",
  "stun:!\"$&'()*+,-.;=_`{}~",
  "stun:\u0001\u0002\u0003\u0004\u0005\u0006\u0007\b\u000b\f\u000e\u000f\u0010\u0011\u0012\u0013\u0014\u0015\u0016\u0017\u0018\u0019\u001a\u001b\u001c\u001d\u001e\u001f^?!\"$%&'()*+,-.;=_`{}~",
  "stun:h\to\ns\rt:9\t0\n0\r0",
  "stun:1.2.3.4.",
  "stun:example.net/foo",
  "stun:/example.net",
  "stun:[email protected]",
  "stun:@example.net"

The last 4 are addressed by this pull request.

@dontcallmedom
Copy link
Member Author

dontcallmedom commented Sep 5, 2024

Among the list above:

  • the following are failing because they use non-ascii hostnames, which I see no reason to consider invalid (RFC3987 would make them valid, but isn't referenced by RFC7064 or 7065, so we would have to bless them explicitly in the API)
  "stun:example(&!$)*+-.;_=~☺org",
  "stun:Go.com",
  "stun:你好你好",
  "stun:faß.ExAmPlE",
  • I'm not sure why the RFC doesn't recognize this as a valid IPv6 URL:
  "stun:[2001::1]",
  • The following are failing because the URL spec cleans up URLs before parsing (control characters, tabs, line returns, ", normalization of dot across encodings, trailing dots); it probably makes sense to benefit from that clean up (given the many things that can go wrong when writing and distributing JS code), but only if that parsing is also used to feed the ICE agent (which the spec isn't explicit about at the moment):
  "stun:example\t.\norg",
  "stun:example.net:\n",
  "stun:GOO​⁠\u200b\u2060\ufeffgoo.com",
  "\u0000\u001b\u0004\u0012 stun:example.com\u001f \r ",
  "stun:www.foo。bar.com",
  "stun:0Xc0.0250.01",
  "stun:ho\tst",
  "stun:!\"$&'()*+,-.;=_`{}~",
  "stun:\u0001\u0002\u0003\u0004\u0005\u0006\u0007\b\u000b\f\u000e\u000f\u0010\u0011\u0012\u0013\u0014\u0015\u0016\u0017\u0018\u0019\u001a\u001b\u001c\u001d\u001e\u001f^?!\"$%&'()*+,-.;=_`{}~",
  "stun:h\to\ns\rt:9\t0\n0\r0",
  "stun:1.2.3.4.",

@dontcallmedom
Copy link
Member Author

dontcallmedom commented Sep 5, 2024

Conversely, the following are detected as invalid by the URL spec but not by the RFC ABNF:

  "stun:example.net:65536",
  "stun:%ef%b7%90zyx.com",
  "stun:%EF%BF%BD",
  "stun:a.b.c.xn--pokxncvks",
  "stun:a.b.c.XN--pokxncvks",
  "stun:a.b.c.Xn--pokxncvks",
  "stun:10.0.0.XN--pokxncvks",
  "stun:10.0.0.xN--pokxncvks",
  "stun:%ef%bc%85%ef%bc%90%ef%bc%90.com",
  "stun:%25",
  "stun:hello%00",
  "stun:192.168.0.257",
  "stun:ho%00st",
  "stun:example.com%80",
  "stun:10000000000",
  "stun:0xffffffff1",
  "stun:256.256.256.256"

@dontcallmedom
Copy link
Member Author

dontcallmedom commented Sep 5, 2024

I have just verified (on the set of URLs in the test case) that we can use the URL parser to normalize the "TWUS-valid but not RFC-valid" URLs into RFC-valid URLs.

We would need to complete the algorithm so that it doesn't just validate the urls, but replace them with the normalized values out of the parsing, by adding a final step to replace url with parsedUrl.scheme + hostAndPortURL.host (+ '?'parsedUrl.query if not null) . This would mean that e.g. stun:Go.com would be turned into stun:go.com, stun:faß.ExAmPlE into stun:xn--fa-hia.example). The advantage of doing so is that it would also mean the ICE Agent would get address information that would be much harder to get wrong (e.g. in terms of dealing with IDNA 2003 vs 2008).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants