New issue
Advanced search Search tips

Issue 669785 link

Starred by 5 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Disallow navigations to ccTLDs from ChromeOmniboxNavigationObserver

Project Member Reported by pkasting@chromium.org, Nov 30 2016

Issue description

Split from bug 479620.

ChromeOmniboxNavigationObserver should check whether the alternate match's destination is a ccTLD per the eTLD service.  If so, it should not send it a ping.

See comment 46 of the original bug.
 
Copying rsleevi's comment 50 from the original bug:

***

There's several possible approaches, so I was trying to cover them:

1) We can filter out ccTLDs (2 letter Alpha-Alpha combinations), which are part of the overall IANA reserved list (of 2 letter TLDs), without hitting the PSL. This provides a localized solution to address 'known' badness (those outside ICANN rules)

2) We can filter out using the registry controlled domains ICANN section, which includes the IANA ccTLDs and the ICANN gTLDs (plus the other root zone entries, like .com and .mil). That's a more heavy-handed solution, and will break the "intranet names are also on the PSL". This implicitly incorporate #1.

3) If we wanted to minimize the breakage from #2 for intranet names (which seemed your concern from earlier messages), we could try to do the more complex AI_CANONNAME thing, which is a variation on #2 that catches all of #1 still.


The pragmatic security/privacy risk only applies to #1 (IANA ccTLDs). Names from #2 (the ICANN gTLDs) 'shouldn't' have host records (contractually), and so shouldn't run into the same security/privacy issues. However, "shouldn't" isn't the same as "guaranteed cannot", hence why #2/#3 exist as options if security/privacy want a strong/hard guarantee.
Cc: rsleevi@chromium.org
Thanks for the summary.

I believe #2 may not be a strict superset of #1, if a TLD's rules are something like "foo.xx / bar.xx" (note lack of inclusion of "xx" itself).  Then "xx" isn't an eTLD and would be allowed by #2, but not by #1.  It depends on the implementation.  (Also, if this difference _does_ occur, I don't know if that's a good or a bad thing.)

I think given the potential downsides here, it would be nice to pick up the wins of #2/3 so bad or incompetent actors don't get user data even briefly.  It also seems like coding #2 would be easier than #1, as it's likely a single (existing) function call in a place that is not perf-critical, whereas to build #1 we'd actually need to enumerate the real country codes (just assuming any two-character name is a country code would break non-cc intranets).

The reason I made the comments about "you seem to be saying that anything that clashes with a TLD will be broken now or later" is that, if that's true, it reduces the motivation to do #3, sufficiently such that I'm not inclined to do it.

Accordingly, it seems like we should do #2, and potentially worry about the "strict superset" case.
.xx is supposed to be covered in the PSL as part of the PSL having all of the root zone data (which would include both IANA and ICANN delegations, but not reserved domains)

But I agree with #2 as both the easiest and most pragmatic case, though acknowledging it will cause some (intended) breakage.
rsleevi -- should this really be P1?  I'm trying to decide whether to punt it off my plate into the general pool.
Given the state of Issue 479620, it probably is a P2
Labels: -Pri-1 Pri-2
Owner: ----
Status: Available (was: Assigned)
If no one has touched this bug in five months, is it really a P2?  If it is, someone should say so now so the appropriate fixers (whoever they may be) know to allocate time to fix it this coming quarter.

Comment 8 by k...@chromium.org, Jan 23 2018

Owner: k...@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 9 by bugdroid1@chromium.org, Feb 3 2018

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

commit 0b180e48e0c416729c0913bc41574a1c9b874306
Author: Kevin Bailey <krb@chromium.org>
Date: Sat Feb 03 03:41:04 2018

[omnibox] Don't ping TLD for alternate nav query

We "ping" (with http) queries that might be host names. Some TLDs
respond as hosts. Filter these out from pinging. (Assume the user
would not want to navigate there.)

Bug: 669785
Change-Id: I11905edf854e52f346048f7f705acab913d02984
Reviewed-on: https://chromium-review.googlesource.com/886562
Commit-Queue: Kevin Bailey <krb@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534253}
[modify] https://crrev.com/0b180e48e0c416729c0913bc41574a1c9b874306/components/omnibox/browser/autocomplete_input.cc
[modify] https://crrev.com/0b180e48e0c416729c0913bc41574a1c9b874306/components/omnibox/browser/autocomplete_input_unittest.cc
[modify] https://crrev.com/0b180e48e0c416729c0913bc41574a1c9b874306/net/base/registry_controlled_domains/registry_controlled_domain.cc
[modify] https://crrev.com/0b180e48e0c416729c0913bc41574a1c9b874306/net/base/registry_controlled_domains/registry_controlled_domain.h
[modify] https://crrev.com/0b180e48e0c416729c0913bc41574a1c9b874306/net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Feb 8 2018

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

commit b0d920d494329b2f6f40704c3b0e5f8733da35dd
Author: Kevin Bailey <krb@chromium.org>
Date: Thu Feb 08 02:18:14 2018

Revert "[omnibox] Don't ping TLD for alternate nav query"

This reverts commit 0b180e48e0c416729c0913bc41574a1c9b874306.

Reason for revert: The change is causing DCHECKs, and blank Omnibox suggestions in production code. The fix is taking longer than expected.

Original change's description:
> [omnibox] Don't ping TLD for alternate nav query
> 
> We "ping" (with http) queries that might be host names. Some TLDs
> respond as hosts. Filter these out from pinging. (Assume the user
> would not want to navigate there.)
> 
> Bug: 669785
> Change-Id: I11905edf854e52f346048f7f705acab913d02984
> Reviewed-on: https://chromium-review.googlesource.com/886562
> Commit-Queue: Kevin Bailey <krb@chromium.org>
> Reviewed-by: Peter Kasting <pkasting@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#534253}

TBR=pkasting@chromium.org,krb@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 669785
Change-Id: If065e96368cd81d94c0de6ad98451b4e8b21c9db
Reviewed-on: https://chromium-review.googlesource.com/906973
Reviewed-by: Kevin Bailey <krb@chromium.org>
Commit-Queue: Kevin Bailey <krb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535259}
[modify] https://crrev.com/b0d920d494329b2f6f40704c3b0e5f8733da35dd/components/omnibox/browser/autocomplete_input.cc
[modify] https://crrev.com/b0d920d494329b2f6f40704c3b0e5f8733da35dd/components/omnibox/browser/autocomplete_input_unittest.cc
[modify] https://crrev.com/b0d920d494329b2f6f40704c3b0e5f8733da35dd/net/base/registry_controlled_domains/registry_controlled_domain.cc
[modify] https://crrev.com/b0d920d494329b2f6f40704c3b0e5f8733da35dd/net/base/registry_controlled_domains/registry_controlled_domain.h
[modify] https://crrev.com/b0d920d494329b2f6f40704c3b0e5f8733da35dd/net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc

Comment 11 by k...@chromium.org, Feb 13 2018

Cc: k...@chromium.org
Owner: ----
Status: Available (was: Assigned)

Sign in to add a comment