New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 657199 link

Starred by 1 user
Status: Fixed
Owner:
█ brettw@chromium.org
Last visit 26 days ago
Closed: Oct 2016
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Callers wrongly pass uncanonicalized host names to GetRegistryLength

Project Member Reported by brettw@chromium.org, Oct 18 2016 
net::registry_controlled_domains::GetRegistryLength returns the number of characters in the canonicalized URL (it internally canonicalizes).

But in the presence of IDN and escaped characters, canonicalization changes the length so the returned value can not be used as indices into the original string. Some callers do this.

For example, there is a Chinese TLD "中国". If you pass "foo.中国" as UTF-8 to GetRegistryLength it will return 10 because the Punycode version is "xn--fiqs8s". This will also be wrong for escaping like "foo.%53o," (0x53 = 'c').

template_url_service.cc even does GetRegistryLength(UTF16ToUTF8(host)) and then does operations based on the result. In this case, it expects an answer of 2, passes a TLD of length 6 (UTF-8), and gets a result of 10!
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d

commit 060f6a0de7706cc43f9d773ae9ce2cb36bc9964d
Author: brettw <brettw@chromium.org>
Date: Wed Oct 26 00:23:11 2016

Reduce buggy usage of the registry controlled domain service.

GetRegistryLength for host names canonicalizes the input for the caller, but
then returns the length in the canonicalized input, which is not necessarily
the same as the length in the original string. As a result, computations
performed by the caller based on this value can be wrong (see the bug for
more).

All callers of this function were audited and changed to use on of the
following:

- Many callers don't need the offsets. A new function
  HostHasRegistryControlledDomain is added to check for the presence of
  a R.C.D. without the risk of returning incorrect string lengths.

- Many callers already have guaranteed-canonical strings (they came out of
  a GURL or KURL object soon before the call) These were changed to use a
  new GetCanonicalHostRegistryLength function. A further advantage is that
  these calls will be faster.

- A new Permissive function is added that handles cases where the input
  is necessarily non-canonical.

Adds an IDN test case to the unit tests.

Removes checking for IP addresses in the already-known-canonical cases.
This requires a separate full canonicalization and IP addresses should
never match the R.C.D. list.

BUG= 657199 

Review-Url: https://codereview.chromium.org/2433583002
Cr-Commit-Position: refs/heads/master@{#427545}

[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/chrome/browser/android/history_report/delta_file_commons.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/chrome/browser/supervised_user/supervised_user_url_filter.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/chrome/browser/supervised_user/supervised_user_url_filter.h
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/chrome/renderer/safe_browsing/phishing_url_feature_extractor.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/components/google/core/browser/google_util.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/components/omnibox/browser/autocomplete_input.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/components/omnibox/browser/history_quick_provider.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/components/omnibox/browser/history_url_provider.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/components/search_engines/template_url_service.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/components/ssl_errors/error_classification.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/components/ssl_errors/error_classification.h
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/components/ssl_errors/error_classification_unittest.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/components/url_formatter/url_fixer.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/content/renderer/webpublicsuffixlist_impl.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/content/renderer/webpublicsuffixlist_impl.h
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/extensions/common/csp_validator.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/extensions/common/manifest_handlers/externally_connectable.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/extensions/common/permissions/permission_message_util.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/extensions/common/url_pattern.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/net/base/registry_controlled_domains/effective_tld_names_unittest1.gperf
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/net/base/registry_controlled_domains/registry_controlled_domain.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/net/base/registry_controlled_domains/registry_controlled_domain.h
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/net/base/url_util.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/net/cert/cert_verify_proc.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/net/cert/x509_certificate.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/third_party/WebKit/public/platform/WebPublicSuffixList.h
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/url/url_canon.h
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/url/url_canon_host.cc
[modify] https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d/url/url_canon_unittest.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a

commit 45f62bf28f51df7f78a5e03c2cb8a5573ea1512a
Author: wychen <wychen@chromium.org>
Date: Wed Oct 26 01:21:05 2016

Revert of Reduce buggy usage of the registry controlled domain service. (patchset #9 id:160001 of https://codereview.chromium.org/2433583002/ )

Reason for revert:
net_unittests failure on Cronet builders

Original issue's description:
> Reduce buggy usage of the registry controlled domain service.
>
> GetRegistryLength for host names canonicalizes the input for the caller, but
> then returns the length in the canonicalized input, which is not necessarily
> the same as the length in the original string. As a result, computations
> performed by the caller based on this value can be wrong (see the bug for
> more).
>
> All callers of this function were audited and changed to use on of the
> following:
>
> - Many callers don't need the offsets. A new function
>   HostHasRegistryControlledDomain is added to check for the presence of
>   a R.C.D. without the risk of returning incorrect string lengths.
>
> - Many callers already have guaranteed-canonical strings (they came out of
>   a GURL or KURL object soon before the call) These were changed to use a
>   new GetCanonicalHostRegistryLength function. A further advantage is that
>   these calls will be faster.
>
> - A new Permissive function is added that handles cases where the input
>   is necessarily non-canonical.
>
> Adds an IDN test case to the unit tests.
>
> Removes checking for IP addresses in the already-known-canonical cases.
> This requires a separate full canonicalization and IP addresses should
> never match the R.C.D. list.
>
> BUG= 657199 
>
> Committed: https://crrev.com/060f6a0de7706cc43f9d773ae9ce2cb36bc9964d
> Cr-Commit-Position: refs/heads/master@{#427545}

TBR=pkasting@chromium.org,asargent@chromium.org,brettw@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 657199 

Review-Url: https://codereview.chromium.org/2454553002
Cr-Commit-Position: refs/heads/master@{#427561}

[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/chrome/browser/android/history_report/delta_file_commons.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/chrome/browser/supervised_user/supervised_user_url_filter.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/chrome/browser/supervised_user/supervised_user_url_filter.h
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/chrome/renderer/safe_browsing/phishing_url_feature_extractor.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/components/google/core/browser/google_util.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/components/omnibox/browser/autocomplete_input.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/components/omnibox/browser/history_quick_provider.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/components/omnibox/browser/history_url_provider.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/components/search_engines/template_url_service.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/components/ssl_errors/error_classification.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/components/ssl_errors/error_classification.h
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/components/ssl_errors/error_classification_unittest.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/components/url_formatter/url_fixer.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/content/renderer/webpublicsuffixlist_impl.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/content/renderer/webpublicsuffixlist_impl.h
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/extensions/common/csp_validator.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/extensions/common/manifest_handlers/externally_connectable.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/extensions/common/permissions/permission_message_util.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/extensions/common/url_pattern.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/net/base/registry_controlled_domains/effective_tld_names_unittest1.gperf
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/net/base/registry_controlled_domains/registry_controlled_domain.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/net/base/registry_controlled_domains/registry_controlled_domain.h
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/net/base/url_util.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/net/cert/cert_verify_proc.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/net/cert/x509_certificate.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/third_party/WebKit/public/platform/WebPublicSuffixList.h
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/url/url_canon.h
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/url/url_canon_host.cc
[modify] https://crrev.com/45f62bf28f51df7f78a5e03c2cb8a5573ea1512a/url/url_canon_unittest.cc
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c

commit 0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c
Author: brettw <brettw@chromium.org>
Date: Wed Oct 26 19:47:32 2016

Reduce buggy usage of the registry controlled domain service.

GetRegistryLength for host names canonicalizes the input for the caller, but
then returns the length in the canonicalized input, which is not necessarily
the same as the length in the original string. As a result, computations
performed by the caller based on this value can be wrong (see the bug for
more).

All callers of this function were audited and changed to use on of the
following:

- Many callers don't need the offsets. A new function
  HostHasRegistryControlledDomain is added to check for the presence of
  a R.C.D. without the risk of returning incorrect string lengths.

- Many callers already have guaranteed-canonical strings (they came out of
  a GURL or KURL object soon before the call) These were changed to use a
  new GetCanonicalHostRegistryLength function. A further advantage is that
  these calls will be faster.

- A new Permissive function is added that handles cases where the input
  is necessarily non-canonical.

Adds an IDN test case to the unit tests.

Removes checking for IP addresses in the already-known-canonical cases.
This requires a separate full canonicalization and IP addresses should
never match the R.C.D. list.

Reland of https://codereview.chromium.org/2433583002/ with fix.

R=pkasting
BUG= 657199 

Review-Url: https://codereview.chromium.org/2446273004
Cr-Commit-Position: refs/heads/master@{#427779}

[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/chrome/browser/android/history_report/delta_file_commons.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/chrome/browser/supervised_user/supervised_user_url_filter.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/chrome/browser/supervised_user/supervised_user_url_filter.h
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/chrome/renderer/safe_browsing/phishing_url_feature_extractor.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/components/google/core/browser/google_util.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/components/omnibox/browser/autocomplete_input.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/components/omnibox/browser/history_quick_provider.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/components/omnibox/browser/history_url_provider.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/components/search_engines/template_url_service.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/components/ssl_errors/error_classification.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/components/ssl_errors/error_classification.h
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/components/ssl_errors/error_classification_unittest.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/components/url_formatter/url_fixer.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/content/renderer/webpublicsuffixlist_impl.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/content/renderer/webpublicsuffixlist_impl.h
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/extensions/common/csp_validator.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/extensions/common/manifest_handlers/externally_connectable.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/extensions/common/permissions/permission_message_util.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/extensions/common/url_pattern.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/net/base/registry_controlled_domains/effective_tld_names_unittest1.gperf
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/net/base/registry_controlled_domains/registry_controlled_domain.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/net/base/registry_controlled_domains/registry_controlled_domain.h
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/net/base/url_util.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/net/cert/cert_verify_proc.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/net/cert/x509_certificate.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/third_party/WebKit/public/platform/WebPublicSuffixList.h
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/url/url_canon.h
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/url/url_canon_host.cc
[modify] https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c/url/url_canon_unittest.cc
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5eedf6dbcfed2ef3d35f338a437800655dc0ef57

commit 5eedf6dbcfed2ef3d35f338a437800655dc0ef57
Author: wychen <wychen@chromium.org>
Date: Wed Oct 26 20:09:12 2016

Revert of Reduce buggy usage of the registry controlled domain service. (patchset #3 id:40001 of https://codereview.chromium.org/2446273004/ )

Reason for revert:
compile failure on Cronet Builders

Original issue's description:
> Reduce buggy usage of the registry controlled domain service.
>
> GetRegistryLength for host names canonicalizes the input for the caller, but
> then returns the length in the canonicalized input, which is not necessarily
> the same as the length in the original string. As a result, computations
> performed by the caller based on this value can be wrong (see the bug for
> more).
>
> All callers of this function were audited and changed to use on of the
> following:
>
> - Many callers don't need the offsets. A new function
>   HostHasRegistryControlledDomain is added to check for the presence of
>   a R.C.D. without the risk of returning incorrect string lengths.
>
> - Many callers already have guaranteed-canonical strings (they came out of
>   a GURL or KURL object soon before the call) These were changed to use a
>   new GetCanonicalHostRegistryLength function. A further advantage is that
>   these calls will be faster.
>
> - A new Permissive function is added that handles cases where the input
>   is necessarily non-canonical.
>
> Adds an IDN test case to the unit tests.
>
> Removes checking for IP addresses in the already-known-canonical cases.
> This requires a separate full canonicalization and IP addresses should
> never match the R.C.D. list.
>
> Reland of https://codereview.chromium.org/2433583002/ with fix.
>
> R=pkasting
> BUG= 657199 
>
> Committed: https://crrev.com/0a8baeebb5bb84e4dc2449c6860f32928eaa6d4c
> Cr-Commit-Position: refs/heads/master@{#427779}

TBR=pkasting@chromium.org,brettw@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 657199 

Review-Url: https://codereview.chromium.org/2454813002
Cr-Commit-Position: refs/heads/master@{#427783}

[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/chrome/browser/android/history_report/delta_file_commons.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/chrome/browser/supervised_user/supervised_user_url_filter.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/chrome/browser/supervised_user/supervised_user_url_filter.h
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/chrome/renderer/safe_browsing/phishing_url_feature_extractor.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/components/google/core/browser/google_util.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/components/omnibox/browser/autocomplete_input.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/components/omnibox/browser/history_quick_provider.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/components/omnibox/browser/history_url_provider.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/components/search_engines/template_url_service.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/components/ssl_errors/error_classification.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/components/ssl_errors/error_classification.h
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/components/ssl_errors/error_classification_unittest.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/components/url_formatter/url_fixer.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/content/renderer/webpublicsuffixlist_impl.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/content/renderer/webpublicsuffixlist_impl.h
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/extensions/common/csp_validator.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/extensions/common/manifest_handlers/externally_connectable.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/extensions/common/permissions/permission_message_util.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/extensions/common/url_pattern.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/net/base/registry_controlled_domains/effective_tld_names_unittest1.gperf
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/net/base/registry_controlled_domains/registry_controlled_domain.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/net/base/registry_controlled_domains/registry_controlled_domain.h
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/net/base/url_util.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/net/cert/cert_verify_proc.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/net/cert/x509_certificate.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/third_party/WebKit/public/platform/WebPublicSuffixList.h
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/url/url_canon.h
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/url/url_canon_host.cc
[modify] https://crrev.com/5eedf6dbcfed2ef3d35f338a437800655dc0ef57/url/url_canon_unittest.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 27 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1b0d95acde33777b9b9ab136c35eeefc11a64718

commit 1b0d95acde33777b9b9ab136c35eeefc11a64718
Author: brettw <brettw@chromium.org>
Date: Thu Oct 27 01:43:50 2016

Reduce buggy usage of the registry controlled domain service.

GetRegistryLength for host names canonicalizes the input for the caller, but
then returns the length in the canonicalized input, which is not necessarily
the same as the length in the original string. As a result, computations
performed by the caller based on this value can be wrong (see the bug for
more).

All callers of this function were audited and changed to use on of the
following:

- Many callers don't need the offsets. A new function
  HostHasRegistryControlledDomain is added to check for the presence of
  a R.C.D. without the risk of returning incorrect string lengths.

- Many callers already have guaranteed-canonical strings (they came out of
  a GURL or KURL object soon before the call) These were changed to use a
  new GetCanonicalHostRegistryLength function. A further advantage is that
  these calls will be faster.

- A new Permissive function is added that handles cases where the input
  is necessarily non-canonical.

Adds an IDN test case to the unit tests.

Removes checking for IP addresses in the already-known-canonical cases.
This requires a separate full canonicalization and IP addresses should
never match the R.C.D. list.

Reland of https://codereview.chromium.org/2446273004/ with fix which was a
reland of https://codereview.chromium.org/2433583002/ with fix.

TBR=pkasting@chromium.org
BUG= 657199 

Review-Url: https://codereview.chromium.org/2451353002
Cr-Commit-Position: refs/heads/master@{#427908}

[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/chrome/browser/android/history_report/delta_file_commons.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/chrome/browser/supervised_user/supervised_user_url_filter.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/chrome/browser/supervised_user/supervised_user_url_filter.h
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/chrome/renderer/safe_browsing/phishing_url_feature_extractor.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/components/google/core/browser/google_util.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/components/omnibox/browser/autocomplete_input.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/components/omnibox/browser/history_quick_provider.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/components/omnibox/browser/history_url_provider.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/components/search_engines/template_url_service.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/components/ssl_errors/error_classification.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/components/ssl_errors/error_classification.h
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/components/ssl_errors/error_classification_unittest.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/components/url_formatter/url_fixer.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/content/renderer/webpublicsuffixlist_impl.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/content/renderer/webpublicsuffixlist_impl.h
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/extensions/common/csp_validator.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/extensions/common/manifest_handlers/externally_connectable.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/extensions/common/permissions/permission_message_util.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/extensions/common/url_pattern.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/net/base/registry_controlled_domains/effective_tld_names_unittest1.gperf
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/net/base/registry_controlled_domains/registry_controlled_domain.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/net/base/registry_controlled_domains/registry_controlled_domain.h
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/net/base/url_util.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/net/cert/cert_verify_proc.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/net/cert/x509_certificate.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/third_party/WebKit/public/platform/WebPublicSuffixList.h
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/url/url_canon.h
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/url/url_canon_host.cc
[modify] https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718/url/url_canon_unittest.cc

Comment 6 by brettw@chromium.org, Oct 27 2016

Status: Fixed (was: Started)
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 27 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f2903f44f5ed9d756fb182a0fdd88fe85d172aab

commit f2903f44f5ed9d756fb182a0fdd88fe85d172aab
Author: foolip <foolip@chromium.org>
Date: Thu Oct 27 10:00:02 2016

Revert of Reduce buggy usage of the registry controlled domain service. (patchset #2 id:20001 of https://codereview.chromium.org/2451353002/ )

Reason for revert:
Failing DCHECK_EQ in GetCanonicalHostRegistryLength on WebKit Mac10.11 (dbg) running http/tests/xmlhttprequest/origin-whitelisting-ip-addresses-with-subdomains.html

BUG= 659949 

Original issue's description:
> Reduce buggy usage of the registry controlled domain service.
>
> GetRegistryLength for host names canonicalizes the input for the caller, but
> then returns the length in the canonicalized input, which is not necessarily
> the same as the length in the original string. As a result, computations
> performed by the caller based on this value can be wrong (see the bug for
> more).
>
> All callers of this function were audited and changed to use on of the
> following:
>
> - Many callers don't need the offsets. A new function
>   HostHasRegistryControlledDomain is added to check for the presence of
>   a R.C.D. without the risk of returning incorrect string lengths.
>
> - Many callers already have guaranteed-canonical strings (they came out of
>   a GURL or KURL object soon before the call) These were changed to use a
>   new GetCanonicalHostRegistryLength function. A further advantage is that
>   these calls will be faster.
>
> - A new Permissive function is added that handles cases where the input
>   is necessarily non-canonical.
>
> Adds an IDN test case to the unit tests.
>
> Removes checking for IP addresses in the already-known-canonical cases.
> This requires a separate full canonicalization and IP addresses should
> never match the R.C.D. list.
>
> Reland of https://codereview.chromium.org/2446273004/ with fix which was a
> reland of https://codereview.chromium.org/2433583002/ with fix.
>
> TBR=pkasting@chromium.org
> BUG= 657199 
>
> Committed: https://crrev.com/1b0d95acde33777b9b9ab136c35eeefc11a64718
> Cr-Commit-Position: refs/heads/master@{#427908}

TBR=pkasting@chromium.org,brettw@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 657199 

Review-Url: https://codereview.chromium.org/2459493002
Cr-Commit-Position: refs/heads/master@{#427984}

[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/chrome/browser/android/history_report/delta_file_commons.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/chrome/browser/supervised_user/supervised_user_url_filter.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/chrome/browser/supervised_user/supervised_user_url_filter.h
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/chrome/renderer/safe_browsing/phishing_url_feature_extractor.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/components/google/core/browser/google_util.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/components/omnibox/browser/autocomplete_input.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/components/omnibox/browser/history_quick_provider.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/components/omnibox/browser/history_url_provider.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/components/search_engines/template_url_service.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/components/ssl_errors/error_classification.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/components/ssl_errors/error_classification.h
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/components/ssl_errors/error_classification_unittest.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/components/url_formatter/url_fixer.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/content/renderer/webpublicsuffixlist_impl.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/content/renderer/webpublicsuffixlist_impl.h
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/extensions/common/csp_validator.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/extensions/common/manifest_handlers/externally_connectable.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/extensions/common/permissions/permission_message_util.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/extensions/common/url_pattern.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/net/base/registry_controlled_domains/effective_tld_names_unittest1.gperf
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/net/base/registry_controlled_domains/registry_controlled_domain.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/net/base/registry_controlled_domains/registry_controlled_domain.h
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/net/base/url_util.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/net/cert/cert_verify_proc.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/net/cert/x509_certificate.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/third_party/WebKit/public/platform/WebPublicSuffixList.h
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/url/url_canon.h
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/url/url_canon_host.cc
[modify] https://crrev.com/f2903f44f5ed9d756fb182a0fdd88fe85d172aab/url/url_canon_unittest.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 27 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5a36380ef1e202c3c6be710a4bdcbc9425014c5d

commit 5a36380ef1e202c3c6be710a4bdcbc9425014c5d
Author: brettw <brettw@chromium.org>
Date: Thu Oct 27 19:51:56 2016

Reduce buggy usage of the registry controlled domain service.

GetRegistryLength for host names canonicalizes the input for the caller, but
then returns the length in the canonicalized input, which is not necessarily
the same as the length in the original string. As a result, computations
performed by the caller based on this value can be wrong (see the bug for
more).

All callers of this function were audited and changed to use on of the
following:

- Many callers don't need the offsets. A new function
  HostHasRegistryControlledDomain is added to check for the presence of
  a R.C.D. without the risk of returning incorrect string lengths.

- Many callers already have guaranteed-canonical strings (they came out of
  a GURL or KURL object soon before the call) These were changed to use a
  new GetCanonicalHostRegistryLength function. A further advantage is that
  these calls will be faster.

- A new Permissive function is added that handles cases where the input
  is necessarily non-canonical.

Adds an IDN test case to the unit tests.

Removes checking for IP addresses in the already-known-canonical cases.
This requires a separate full canonicalization and IP addresses should
never match the R.C.D. list.

Reland of https://codereview.chromium.org/2451353002/ with fix which was a
reland of https://codereview.chromium.org/2446273004/ with fix which was a
reland of https://codereview.chromium.org/2433583002/ with fix.

TBR=pkasting@chromium.org
BUG= 657199 

Review-Url: https://codereview.chromium.org/2456643005
Cr-Commit-Position: refs/heads/master@{#428116}

[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/chrome/browser/android/history_report/delta_file_commons.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/chrome/browser/supervised_user/supervised_user_url_filter.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/chrome/browser/supervised_user/supervised_user_url_filter.h
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/chrome/renderer/safe_browsing/phishing_url_feature_extractor.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/components/google/core/browser/google_util.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/components/omnibox/browser/autocomplete_input.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/components/omnibox/browser/history_quick_provider.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/components/omnibox/browser/history_url_provider.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/components/search_engines/template_url_service.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/components/ssl_errors/error_classification.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/components/ssl_errors/error_classification.h
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/components/ssl_errors/error_classification_unittest.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/components/url_formatter/url_fixer.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/content/renderer/webpublicsuffixlist_impl.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/content/renderer/webpublicsuffixlist_impl.h
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/extensions/common/csp_validator.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/extensions/common/manifest_handlers/externally_connectable.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/extensions/common/permissions/permission_message_util.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/extensions/common/url_pattern.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/net/base/registry_controlled_domains/effective_tld_names_unittest1.gperf
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/net/base/registry_controlled_domains/registry_controlled_domain.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/net/base/registry_controlled_domains/registry_controlled_domain.h
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/net/base/url_util.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/net/cert/cert_verify_proc.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/net/cert/x509_certificate.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/url/url_canon.h
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/url/url_canon_host.cc
[modify] https://crrev.com/5a36380ef1e202c3c6be710a4bdcbc9425014c5d/url/url_canon_unittest.cc
►

Sign in to add a comment