Lock down creation of "filesystem:chrome-extension://" URLs

Disallow file api access from processes that lack permissions for the scheme of the origin.

BUG=644966
Committed: https://crrev.com/e2e627390daf45140560b17c3e5f20fe73544a4f
Cr-Commit-Position: refs/heads/master@{#423348}

[ncarter (slow), 2016-10-04 17:58:52]

The CQ bit was checked by nick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 17:59:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 18:09:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 18:09:35]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ncarter (slow), 2016-10-04 18:25:26]

Description was changed from

==========
Restrict filesystem:chrome-extension:// URL creation to appropriate
processes.

BUG=
==========

to

==========
Disallow file api access from processes that lack permissions for the scheme of
the origin.

BUG=644966
==========

[ncarter (slow), 2016-10-04 18:30:49]

The CQ bit was checked by nick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 18:31:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ncarter (slow), 2016-10-04 18:51:41]

nick@chromium.org changed reviewers:
+ creis@chromium.org

[ncarter (slow), 2016-10-04 18:51:41]

Charlie: this is the fix I'm testing. I'm working on adding a
chrome_security_exploit_browsertest case for the IPCs now.

[commit-bot: I haz the power, 2016-10-04 19:55:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 19:56:00]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-10-04 22:00:43]

The CQ bit was checked by nick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 22:01:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2016-10-04 22:19:04]

LGTM.  Just publishing the comments that you and I discussed in person.

https://codereview.chromium.org/2387173005/diff/40001/chrome/browser/chrome_s...
File chrome/browser/chrome_security_exploit_browsertest.cc (right):

https://codereview.chromium.org/2387173005/diff/40001/chrome/browser/chrome_s...
chrome/browser/chrome_security_exploit_browsertest.cc:136: std::string blob_path
= "5881f76e-10d2-410d-8c61-ef210502acfd";
Some cleanup possible here.

https://codereview.chromium.org/2387173005/diff/40001/chrome/browser/chrome_s...
chrome/browser/chrome_security_exploit_browsertest.cc:145:
data_elements[0].SetToBytes(blob_contents.c_str(), blob_contents.size());
Move below.

https://codereview.chromium.org/2387173005/diff/40001/chrome/browser/chrome_s...
chrome/browser/chrome_security_exploit_browsertest.cc:200: EXPECT_EQ(
Gate this (or the test) on --isolate-extensions mode.

https://codereview.chromium.org/2387173005/diff/40001/content/browser/child_p...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/2387173005/diff/40001/content/browser/child_p...
content/browser/child_process_security_policy_impl.cc:803: return false;
Consider adding a UMA to detect if this is happening too often?

[ncarter (slow), 2016-10-04 23:03:56]

The CQ bit was checked by nick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 23:04:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ncarter (slow), 2016-10-04 23:09:35]

nick@chromium.org changed reviewers:
+ isherman@chromium.org

[ncarter (slow), 2016-10-04 23:09:36]

+isherman for histograms.xml (i might have screwed this up, never declared a
boolean histogram before)

https://codereview.chromium.org/2387173005/diff/40001/chrome/browser/chrome_s...
File chrome/browser/chrome_security_exploit_browsertest.cc (right):

https://codereview.chromium.org/2387173005/diff/40001/chrome/browser/chrome_s...
chrome/browser/chrome_security_exploit_browsertest.cc:136: std::string blob_path
= "5881f76e-10d2-410d-8c61-ef210502acfd";
On 2016/10/04 22:19:04, Charlie Reis wrote:
> Some cleanup possible here.

Done.

https://codereview.chromium.org/2387173005/diff/40001/chrome/browser/chrome_s...
chrome/browser/chrome_security_exploit_browsertest.cc:145:
data_elements[0].SetToBytes(blob_contents.c_str(), blob_contents.size());
On 2016/10/04 22:19:04, Charlie Reis wrote:
> Move below.

Done.

https://codereview.chromium.org/2387173005/diff/40001/chrome/browser/chrome_s...
chrome/browser/chrome_security_exploit_browsertest.cc:200: EXPECT_EQ(
On 2016/10/04 22:19:04, Charlie Reis wrote:
> Gate this (or the test) on --isolate-extensions mode.

Done.

https://codereview.chromium.org/2387173005/diff/40001/content/browser/child_p...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/2387173005/diff/40001/content/browser/child_p...
content/browser/child_process_security_policy_impl.cc:803: return false;
On 2016/10/04 22:19:04, Charlie Reis wrote:
> Consider adding a UMA to detect if this is happening too often?

Done.

[commit-bot: I haz the power, 2016-10-05 00:24:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 00:24:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ilya Sherman, 2016-10-05 00:50:40]

https://codereview.chromium.org/2387173005/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2387173005/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5537: +<histogram
name="ChildProcessSecurityPolicy.FileSystemCanCommitOrigin"
It looks like you're declaring a new top-level name,
"ChildProcessSecurityPolicy".  Could you please check whether there's any
existing top-level group names that would be appropriate to nest this under?

https://codereview.chromium.org/2387173005/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5538: +    units="boolean">
Please specify an enum here with semantically appropriate labels.

[jam, 2016-10-05 01:34:44]

jam@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2016-10-05 01:34:45]

https://codereview.chromium.org/2387173005/diff/80001/chrome/browser/chrome_s...
File chrome/browser/chrome_security_exploit_browsertest.cc (right):

https://codereview.chromium.org/2387173005/diff/80001/chrome/browser/chrome_s...
chrome/browser/chrome_security_exploit_browsertest.cc:16: #include
"content/common/fileapi/file_system_messages.h"
please don't add dependencies to content internals from chrome (I saw the
earlier patch that did this and just commented in
https://codereview.chromium.org/2364633004/)

you can do this through test functions in content/public/test

[ncarter (slow), 2016-10-05 19:03:06]

https://codereview.chromium.org/2387173005/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2387173005/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5537: +<histogram
name="ChildProcessSecurityPolicy.FileSystemCanCommitOrigin"
On 2016/10/05 00:50:40, Ilya Sherman wrote:
> It looks like you're declaring a new top-level name,
> "ChildProcessSecurityPolicy".  Could you please check whether there's any
> existing top-level group names that would be appropriate to nest this under?

Done. This particular case was a nice fit into the existing "FileSystem."
category. Though actually, it seems there are several
"ChildProcessSecurityPolicy." UMA histograms declared, just none appear to be in
histograms.xml?! I would fix it in this patch if I weren't targeting a merge. Do
we have anything server-side that will warn us about non-exported histograms
that are being uploaded in UMA reports?

https://codereview.chromium.org/2387173005/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5538: +    units="boolean">
On 2016/10/05 00:50:40, Ilya Sherman wrote:
> Please specify an enum here with semantically appropriate labels.

Thanks for suggesting this. Looking around I found BooleanHit which looks like a
good match -- is that the preferred pattern for a hit counter histogram, where
there's no value to sample, you just want to record that an event happened? I've
considered UMA_HISTOGRAM_COUNTS(..., 1) before, but it just looked so wrong --
there's no non-histogram macro like UMA_EVENT_COUNTER() right?

[ncarter (slow), 2016-10-05 20:49:31]

charlie: I've removed the controversial test, since we need this fix landed
ASAP. PTAL

[Charlie Reis, 2016-10-05 20:52:59]

LGTM.  We can land the test after resolving the questions around DEPS with jam@.

isherman@: Can you take another look at histograms.xml?  Thanks!

[ncarter (slow), 2016-10-05 21:11:19]

The CQ bit was checked by nick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 21:11:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2016-10-05 22:37:09]

Metrics lgtm, thanks.

https://codereview.chromium.org/2387173005/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2387173005/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5537: +<histogram
name="ChildProcessSecurityPolicy.FileSystemCanCommitOrigin"
On 2016/10/05 19:03:06, ncarter wrote:
> On 2016/10/05 00:50:40, Ilya Sherman wrote:
> > It looks like you're declaring a new top-level name,
> > "ChildProcessSecurityPolicy".  Could you please check whether there's any
> > existing top-level group names that would be appropriate to nest this under?
> 
> Done. This particular case was a nice fit into the existing "FileSystem."
> category. Though actually, it seems there are several
> "ChildProcessSecurityPolicy." UMA histograms declared, just none appear to be
in
> histograms.xml?! I would fix it in this patch if I weren't targeting a merge.
Do
> we have anything server-side that will warn us about non-exported histograms
> that are being uploaded in UMA reports?

Ah, interesting -- some histograms with this category do appear in the internal
histograms.xml file, but don't seem to have any data shown on the dashboard.  I
wonder if they're not generating any data, or what's going on...

(It's hard to have a server-side warning, because we only see the hashes, and we
quite intentionally can't unhash unmapped histograms.  We do have
find_unmapped_histograms.py though.)

https://codereview.chromium.org/2387173005/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5538: +    units="boolean">
On 2016/10/05 19:03:06, ncarter wrote:
> On 2016/10/05 00:50:40, Ilya Sherman wrote:
> > Please specify an enum here with semantically appropriate labels.
> 
> Thanks for suggesting this. Looking around I found BooleanHit which looks like
a
> good match -- is that the preferred pattern for a hit counter histogram, where
> there's no value to sample, you just want to record that an event happened?
I've
> considered UMA_HISTOGRAM_COUNTS(..., 1) before, but it just looked so wrong --
> there's no non-histogram macro like UMA_EVENT_COUNTER() right?

Yep, BooleanHit sounds like a good fit.  It's relatively rarely used, so there's
no macro defined for it; UMA_HISTOGRAM_BOOLEAN is a good choice for now.

[ncarter (slow), 2016-10-05 22:47:24]

The CQ bit was unchecked by nick@chromium.org

[ncarter (slow), 2016-10-05 22:47:24]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2016-10-05 22:47:25]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2387173005/#ps140001
(title: "rparen")

[commit-bot: I haz the power, 2016-10-05 22:47:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 00:09:52]

Description was changed from

==========
Disallow file api access from processes that lack permissions for the scheme of
the origin.

BUG=644966
==========

to

==========
Disallow file api access from processes that lack permissions for the scheme of
the origin.

BUG=644966
==========

[commit-bot: I haz the power, 2016-10-06 00:09:54]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-10-06 00:13:11]

Description was changed from

==========
Disallow file api access from processes that lack permissions for the scheme of
the origin.

BUG=644966
==========

to

==========
Disallow file api access from processes that lack permissions for the scheme of
the origin.

BUG=644966

Committed: https://crrev.com/e2e627390daf45140560b17c3e5f20fe73544a4f
Cr-Commit-Position: refs/heads/master@{#423348}
==========

[commit-bot: I haz the power, 2016-10-06 00:13:13]

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