Only accept HTTP and HTTPS urls for opensearch descriptor.

Only accept HTTP and HTTPS urls for opensearch descriptor.

This CL prevents http: or https: urls from referring to file: urls as search descriptor xmls.
Allowing urls to refer to other urls with the same scheme has been considered (e.g. a file: url
referring to an OSDD xml from a file: url), but this currently doesn't work for file: urls
because of http://b/863583, so is not implemented here.

BUG=429838
Committed: https://crrev.com/c4b8fd74e67dfb23ef150c6aa313cb506746f06f
Cr-Commit-Position: refs/heads/master@{#317723}

[meacer, 2015-02-18 21:48:58]

meacer@chromium.org changed reviewers:
+ pkasting@chromium.org

[meacer, 2015-02-18 21:48:58]

PTAL, thanks!

[Peter Kasting, 2015-02-18 21:58:56]

https://codereview.chromium.org/917313004/diff/20001/chrome/browser/ui/search...
File chrome/browser/ui/search_engines/search_engine_tab_helper.cc (right):

https://codereview.chromium.org/917313004/diff/20001/chrome/browser/ui/search...
chrome/browser/ui/search_engines/search_engine_tab_helper.cc:113: if
(!osdd_url.is_valid() || !osdd_url.SchemeIsHTTPOrHTTPS())
I feel like perhaps file: URLs should be allowed to specify OSDDs that are other
file: URLs.  For example, what about local testing of some website content that
uses a relative URL to specify the OSDD?  Seems like the OSDD should work
correctly there.

Perhaps the check here should be more like:

  if (!osdd_url.is_valid() ||
      ((osdd_url.scheme() != page_url.scheme()) &&
       !osdd_url.SchemeIsHTTPOrHTTPS()))
    ...

[meacer, 2015-02-18 22:25:11]

https://codereview.chromium.org/917313004/diff/20001/chrome/browser/ui/search...
File chrome/browser/ui/search_engines/search_engine_tab_helper.cc (right):

https://codereview.chromium.org/917313004/diff/20001/chrome/browser/ui/search...
chrome/browser/ui/search_engines/search_engine_tab_helper.cc:113: if
(!osdd_url.is_valid() || !osdd_url.SchemeIsHTTPOrHTTPS())
On 2015/02/18 21:58:56, Peter Kasting wrote:
> I feel like perhaps file: URLs should be allowed to specify OSDDs that are
other
> file: URLs.  For example, what about local testing of some website content
that
> uses a relative URL to specify the OSDD?  Seems like the OSDD should work
> correctly there.
> 
> Perhaps the check here should be more like:
> 
>   if (!osdd_url.is_valid() ||
>       ((osdd_url.scheme() != page_url.scheme()) &&
>        !osdd_url.SchemeIsHTTPOrHTTPS()))
>     ...

Hmm, that would still allow file:// urls to enumerate the filesystem, right?
Given a recent exploit using file:// urls (albeit in a different way), I have to
admit I don't feel too comfortable allowing that. WDYT?

[Peter Kasting, 2015-02-18 22:35:57]

On 2015/02/18 22:25:11, Mustafa Emre Acer wrote:
>
https://codereview.chromium.org/917313004/diff/20001/chrome/browser/ui/search...
> File chrome/browser/ui/search_engines/search_engine_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/917313004/diff/20001/chrome/browser/ui/search...
> chrome/browser/ui/search_engines/search_engine_tab_helper.cc:113: if
> (!osdd_url.is_valid() || !osdd_url.SchemeIsHTTPOrHTTPS())
> On 2015/02/18 21:58:56, Peter Kasting wrote:
> > I feel like perhaps file: URLs should be allowed to specify OSDDs that are
> other
> > file: URLs.  For example, what about local testing of some website content
> that
> > uses a relative URL to specify the OSDD?  Seems like the OSDD should work
> > correctly there.
> > 
> > Perhaps the check here should be more like:
> > 
> >   if (!osdd_url.is_valid() ||
> >       ((osdd_url.scheme() != page_url.scheme()) &&
> >        !osdd_url.SchemeIsHTTPOrHTTPS()))
> >     ...
> 
> Hmm, that would still allow file:// urls to enumerate the filesystem, right?
> Given a recent exploit using file:// urls (albeit in a different way), I have
to
> admit I don't feel too comfortable allowing that. WDYT?

To be clear: you're worried about an attack where an attacker gets you to
download a local file and then open it in your browser, and as a result can
attempt to (unreliably) probe for the presence of other files on your
filesystem?

I feel like if you're already downloading files from an attacker and then
opening them from your local filesystem you have potentially bigger problems.  I
would be surprised if there wasn't already a way to try and probe your
filesystem (and more reliably to boot) in such a scenario, although I don't know
for sure.  Maybe the other security guys would know.

[meacer, 2015-02-18 22:48:05]

On 2015/02/18 22:35:57, Peter Kasting wrote:
> On 2015/02/18 22:25:11, Mustafa Emre Acer wrote:
> >
>
https://codereview.chromium.org/917313004/diff/20001/chrome/browser/ui/search...
> > File chrome/browser/ui/search_engines/search_engine_tab_helper.cc (right):
> > 
> >
>
https://codereview.chromium.org/917313004/diff/20001/chrome/browser/ui/search...
> > chrome/browser/ui/search_engines/search_engine_tab_helper.cc:113: if
> > (!osdd_url.is_valid() || !osdd_url.SchemeIsHTTPOrHTTPS())
> > On 2015/02/18 21:58:56, Peter Kasting wrote:
> > > I feel like perhaps file: URLs should be allowed to specify OSDDs that are
> > other
> > > file: URLs.  For example, what about local testing of some website content
> > that
> > > uses a relative URL to specify the OSDD?  Seems like the OSDD should work
> > > correctly there.
> > > 
> > > Perhaps the check here should be more like:
> > > 
> > >   if (!osdd_url.is_valid() ||
> > >       ((osdd_url.scheme() != page_url.scheme()) &&
> > >        !osdd_url.SchemeIsHTTPOrHTTPS()))
> > >     ...
> > 
> > Hmm, that would still allow file:// urls to enumerate the filesystem, right?
> > Given a recent exploit using file:// urls (albeit in a different way), I
have
> to
> > admit I don't feel too comfortable allowing that. WDYT?
> 
> To be clear: you're worried about an attack where an attacker gets you to
> download a local file and then open it in your browser, and as a result can
> attempt to (unreliably) probe for the presence of other files on your
> filesystem?

Yes.

> 
> I feel like if you're already downloading files from an attacker and then
> opening them from your local filesystem you have potentially bigger problems. 
I
> would be surprised if there wasn't already a way to try and probe your
> filesystem (and more reliably to boot) in such a scenario, although I don't
know
> for sure.  Maybe the other security guys would know.

This was also the position of the security team until recently, but we are being
a bit more careful now. This is obviously speculative, but users may expect the
same level of security from file:// urls as they do from web urls.

Julien, any thoughts? The question is whether file:// urls should be able to
refer to other file:// urls as opensearch descriptors. You can find more details
in the bug.

[TheJH, 2015-02-21 01:15:02]

On 2015/02/18 22:25:11, Mustafa Emre Acer wrote:
> Hmm, that would still allow file:// urls to enumerate the filesystem, right?

That's possible anyway, using the well-known <script src={url_to_test}
onload={exists_handler} onerror={inaccessible_handler}></script> trick. Place
the following file in the local filesystem and you can use it to test for the
presence of files anywhere in the local filesystem.

===================================
<!DOCTYPE html>
<html>
 <head id="head">
  <script>
   var $ = document.getElementById.bind(document);

   function get_url(url) {
    var e = document.createElement('script');
    e.setAttribute('src', url);
    e.onload = alert.bind(null, 'exists');
    e.onerror = alert.bind(null, 'failed');
    $('head').appendChild(e);
   }

   function run() {
    var url = $('url').value;
    if (url[0] == '/') url = 'file://'+url;
    get_url(url);
   }
  </script>
 </head>
 <body>
  <h1>
   file presence tester
  </h1>
  <br>
  path: <input id="url" size="100">
  <br>
  <button onclick="run()">test</button>
 </body>
</html>
===================================

[meacer, 2015-02-21 01:31:29]

That's a good point. Perhaps we should block that too (if possible at all), but
that's a question for a longer discussion, so for this CL I'll go with Peter's
suggestion and allow file:// urls.

[meacer, 2015-02-23 20:37:37]

https://codereview.chromium.org/917313004/diff/20001/chrome/browser/ui/search...
File chrome/browser/ui/search_engines/search_engine_tab_helper.cc (right):

https://codereview.chromium.org/917313004/diff/20001/chrome/browser/ui/search...
chrome/browser/ui/search_engines/search_engine_tab_helper.cc:113: if
(!osdd_url.is_valid() || !osdd_url.SchemeIsHTTPOrHTTPS())
On 2015/02/18 22:25:11, Mustafa Emre Acer wrote:
> On 2015/02/18 21:58:56, Peter Kasting wrote:
> > I feel like perhaps file: URLs should be allowed to specify OSDDs that are
> other
> > file: URLs.  For example, what about local testing of some website content
> that
> > uses a relative URL to specify the OSDD?  Seems like the OSDD should work
> > correctly there.
> > 
> > Perhaps the check here should be more like:
> > 
> >   if (!osdd_url.is_valid() ||
> >       ((osdd_url.scheme() != page_url.scheme()) &&
> >        !osdd_url.SchemeIsHTTPOrHTTPS()))
> >     ...
> 
> Hmm, that would still allow file:// urls to enumerate the filesystem, right?
> Given a recent exploit using file:// urls (albeit in a different way), I have
to
> admit I don't feel too comfortable allowing that. WDYT?

Okay, I was going to allow file:// urls, but it turns out they already don't
work because of
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
(path for files is the actual file path, which is almost always longer than 1).

I'll document this and keep the current check.

[Peter Kasting, 2015-02-23 20:52:38]

OK.  LGTM then.

[meacer, 2015-02-23 21:07:53]

Thanks.

[meacer, 2015-02-23 21:08:10]

The CQ bit was checked by meacer@chromium.org

[meacer, 2015-02-23 21:08:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/917313004/#ps40001 (title:
"Document file:// url behavior, some style changes.")

[commit-bot: I haz the power, 2015-02-23 21:08:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/917313004/40001

[meacer, 2015-02-23 22:24:22]

The CQ bit was checked by meacer@chromium.org

[meacer, 2015-02-23 22:24:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/917313004/#ps60001 (title:
"Fix build")

[commit-bot: I haz the power, 2015-02-23 22:25:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/917313004/60001

[meacer, 2015-02-23 22:53:27]

The CQ bit was unchecked by meacer@chromium.org

[meacer, 2015-02-24 00:16:07]

The CQ bit was checked by meacer@chromium.org

[meacer, 2015-02-24 00:16:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/917313004/#ps80001 (title:
"Fix include")

[commit-bot: I haz the power, 2015-02-24 00:16:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/917313004/80001

[commit-bot: I haz the power, 2015-02-24 01:30:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-24 01:31:29]

Patchset 5 (id:??) landed as
https://crrev.com/c4b8fd74e67dfb23ef150c6aa313cb506746f06f
Cr-Commit-Position: refs/heads/master@{#317723}