Reduce buggy usage of the registry controlled domain service.

extensions/common/manifest_handlers/externally_connectable.cc

Index: extensions/common/manifest_handlers/externally_connectable.cc
diff --git a/extensions/common/manifest_handlers/externally_connectable.cc b/extensions/common/manifest_handlers/externally_connectable.cc
index 7dcdaffcb546269e0230463cbde36ba8b0b18f70..fc9b6a7e3e1508f5373336db94c0d2456b9e08e8 100644
--- a/extensions/common/manifest_handlers/externally_connectable.cc
+++ b/extensions/common/manifest_handlers/externally_connectable.cc
@@ -20,6 +20,7 @@
 #include "extensions/common/permissions/api_permission_set.h"
 #include "extensions/common/url_pattern.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
+#include "net/base/url_util.h"
 #include "url/gurl.h"
 
 namespace rcd = net::registry_controlled_domains;
@@ -139,9 +140,21 @@ std::unique_ptr<ExternallyConnectableInfo> ExternallyConnectableInfo::FromValue(
         continue;
       }
 
+      url::CanonHostInfo host_info;
+      std::string canonical_host =
+          net::CanonicalizeHost(pattern.host(), &host_info);
+      if (canonical_host.empty()) {
+        // CanonicalizeHost returns empty string on error. The URL parsing
+        // combined with host().empty() should have caught this above.
+        NOTREACHED() << *it;

[Peter Kasting, 2016/10/22 05:04:18]

This violates the style guide by handling NOTREACHED.

I think we should just delete this block entirely.  If you are
super-uncomfortable doing it in this CL I can write a separate CL to replace the
whole original block with a DCHECK_NE(std::string::npos, registry_length), but
then this CL would still want to rip out that DCHECK to make use of
HostHasRegistryControlledDomain() anyway, so that's just make-work that doesn't
buy anything.  So let's just remove it.

[brettw, 2016/10/24 21:45:23]

I was also unhappy with this function when I was writing it. I kind of wanted to
just check rcd::HostHasRegistryControlledDomain and delete this call altogether.

But I don't actually understand the calling code very well or how strong the
guarantees actually are above, and it seemed like it might be a potentially
security-sensitive check (these patterns are coming from extensions manifests
which makes me paranoid). I didn't want to fall through to doing something if it
actually means "malicious extension manifest" so I defaulted to exactly
preserving the previous behavior.

[Peter Kasting, 2016/10/24 23:04:32]

Pretty sure we need to handle malicious input here, but that this code is
literally saying control flow can't reach here due to the parsing above.  It
made more sense in the old world since the old code called a function that had a
return value that we were basically saying couldn't be returned.  But in the new
world you're now adding explicit code to go out of its way and check for this
case.  This raises my hackles because there's no way someone reading the new
version could think anything other than that this is, in fact, possible to hit,
and we need this code (and the NOTREACHED is the part that's broken).  In other
words, I think you're reinforcing the precise concerns you had :)

I suggest looping in an OWNER of this code to sanity-check that my
interpretation and proposed change are correct.  If possible I'd like to avoid
adding this code just to spin up a separate parallel review to rip it back out,
though I also don't want to block your fix if for some reason correct resolution
of this turns out to be hard.

[asargent_no_longer_on_chrome, 2016/10/25 18:20:49]

For context, this check is less about security and more about our philosophy of
the web platform and how web pages can rely on extensions to provide additional
capabilities. The externally_connectable feature lets an extension specify an
explicit set of url match patterns that are allowed to open a messaging pipe to
the extension, and when the feature was developed there was a strong desire from
the web platform folks to limit this to some finite set of websites (ideally
something like a website companion extension developed by the same team as the
website itself) to avoid having extensions that become de facto providers of
additional API surface to the entire web platform. 

The guarantee we want is that each url match pattern we put into |matches| is
roughly "a particular site", e.g. not matching all hosts or all urls under an
"eTLD"/"public suffix". It's ok (definitely not something we should crash on) if
an extension accidentally provides an invalid entry; we should just generate an
install warning and keep processing. 

As for the question of the NOTREACHED, it seems like we're guaranteed that
pattern.host() will be some non-empty string, but isn't it possible that (eg due
to bugs in URLPattern) that it will be some nonempty but nonsensical string that
will end up failing the canonicalization process in CanonicalizeHost? I didn't
trace beyond the body of CanonicalizeHost in net/base/url_util.cc, but it sure
looks like there is a possibility canon_host can end up empty. So my vote would
be to just remove the NOTREACHED and change the code to treat this as an install
warning and keep going like the other cases here (instead of setting *error and
returning a null ExternallyConnectableInfo).

[Peter Kasting, 2016/10/25 20:28:17]

Will it?  If so, then handling this makes sense, but can you give a sample input
which would cause this, so we can stick it in a motivating comment here?

I want to make sure we're only handling a real case that can actually get here,
and not handling something that could only ever arise if other code was not
obeying its contracts.  If, for example, URLPattern::Parse() should never return
PARSE_SUCCESS on a non-canonical host, we should definitely not handle failing
that here.  (If it shouldn't do this but currently does, then we should
separately fix that issue and then make sure we don't handle that here.)

Whereas if it's OK for Parse() to succeed on something that cannot be
canonicalized, then handling this is correct, and we should provide a sample
such input.

[asargent_no_longer_on_chrome, 2016/10/25 20:57:13]

Taking a quick look at the code for URLPattern::Parse, it doesn't seem to use
any of the GURL canonicalization utilities, in favor of just doing a bunch of
"manual" string parsing, so I would not be surprised if there was such a case
(or some future refactoring accidentally created one). It's definitely possible
for some inputs to URLPattern::Parse to end up with a non-canonical host, so
having a canonical host is definitely not part of the contract. 

This code doesn't execute very frequently, so I would err on the side of extra
checks for defense in depth even if the conditions they test for are unlikely. 

[Peter Kasting, 2016/10/25 21:05:19]

OK.  I'm fine with that outcome, as long as we have comments that make this
clear (as opposed to the old comments that implied the opposite, or no comments
at all).

Do you think it would make sense to use more of the GURL/url_util parsing and
canonicalization code in the future instead of roll-your-own string parsing?  If
so perhaps there should be a separate bug for that?

[asargent_no_longer_on_chrome, 2016/10/25 23:37:55]

Hmm, the tricky part about doing that in general is that a bunch of patterns
that are legal are not valid urls (a "*" can appear in various places), so I
wouldn't expect our canonicalizer to work on them:

https://developer.chrome.com/extensions/match_patterns

But it looks like at least for the host, we could enforce that it canonicalizes
properly after parsing, as long as it is non-empty. I've filed crbug.com/659375
for this. 

+        *error = ErrorUtils::FormatErrorMessageUTF16(
+            errors::kErrorInvalidMatchPattern, *it);
+        return std::unique_ptr<ExternallyConnectableInfo>();
+      }
+
       // Wildcards on subdomains of a TLD are not allowed.
-      size_t registry_length = rcd::GetRegistryLength(
-          pattern.host(),
+      bool has_registry = rcd::HostHasRegistryControlledDomain(
+          canonical_host,

[Peter Kasting, 2016/10/22 05:04:18]

Nit: Seems like using pattern.host() here is fine regardless of whether we rip
the block above out, since this function will try to re-canonicalize anyway.

[brettw, 2016/10/24 21:45:23]

If we're keeping the code above (which I think we should), it will tend to be
faster to canonicalize an already-canonicalized host.

[Peter Kasting, 2016/10/24 23:04:32]

Seems fair, I mostly said this because I'm convinced the above code must go away
and I wanted to be clear (to myself) we didn't need to canonicalize this.

           // This means that things that look like TLDs - the foobar in
           // http://google.foobar - count as TLDs.
           rcd::INCLUDE_UNKNOWN_REGISTRIES,
@@ -149,17 +162,9 @@ std::unique_ptr<ExternallyConnectableInfo> ExternallyConnectableInfo::FromValue(
           // codereview.appspot.com and evil.appspot.com are different.
           rcd::INCLUDE_PRIVATE_REGISTRIES);
 
-      if (registry_length == std::string::npos) {
-        // The URL parsing combined with host().empty() should have caught this.
-        NOTREACHED() << *it;
-        *error = ErrorUtils::FormatErrorMessageUTF16(
-            errors::kErrorInvalidMatchPattern, *it);
-        return std::unique_ptr<ExternallyConnectableInfo>();
-      }
-
       // Broad match patterns like "*.com", "*.co.uk", and even "*.appspot.com"
       // are not allowed. However just "appspot.com" is ok.
-      if (registry_length == 0 && pattern.match_subdomains()) {
+      if (!has_registry && pattern.match_subdomains()) {
         // Warning not error for forwards compatibility.
         install_warnings->push_back(
             InstallWarning(ErrorUtils::FormatErrorMessage(