Completing the thought above:

Filesystem experts: what I'm curious about is, is there any normally accessible js or ppapi path that would allow cross-origin filesystem request to make it as far as ChildProcessSecurityPolicyImpl::Can(Read|Write|Delete)FileSystemFile in the browser process? From what I could tell from the spec, I thought this shouldn't be possible. E.g. if you just try to xhr a different-origin filesystem file, it should be blocked, right?

Perhaps the code in RenderViewHostImpl::OnStartDragging affords a way to do this?

Perhaps this is becomes possible in a mode like --disable-web-security?

I'm planning on adding a temporary DumpWithoutCrashing to get stack traces here.

To further clarify, by "it should be blocked", I mean: should all cross-origin accesses should should be suppressed inside of blink, before they generate browser IPC?

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

commit 7fd5f88a9dbe923676a8b97caf3aad08eed631fd
Author: nick <nick@chromium.org>
Date: Mon Oct 10 18:34:25 2016

Revert "[Merge to M54] Disallow file api access from processes that lack permissions for the scheme of the origin."

This reverts commit c9ac10a66c02609504c34849f8bb17e6f60d3a5f.

Reverting because of bug 654479: data from canary suggests that this fix may
be having unintended behavior.

BUG= 644966 ,654479
TBR=creis@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2405933002
Cr-Commit-Position: refs/branch-heads/2840@{#702}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/7fd5f88a9dbe923676a8b97caf3aad08eed631fd/content/browser/child_process_security_policy_impl.cc

Michael & Kinuko probably are probably most familiar with the history of this code... Although it could very well be the case that some unintentional use of the system slipped by. So any more info about what is calling this/ where this is happening would be helpful.

adding Josh & Ben as well, as they have good ecosystem knowledge

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

commit 0a66889a04ad4c87af24aeaa567184bd052227bd
Author: nick <nick@chromium.org>
Date: Mon Oct 10 22:14:19 2016

Add temporary DumpWithoutCrashing to understand why the filesystem URL origin checks are failing in practice.

BUG=654479

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

[modify] https://crrev.com/0a66889a04ad4c87af24aeaa567184bd052227bd/content/browser/child_process_security_policy_impl.cc

No data yet (as far as I can tell) from the DumpWithoutCrashing, though the UMA is still firing.

Still nothing. Will look again tomorrow.

+victor 

Still nothing in go/crash, which prompted us to take another look at the UMA. The failure seems only to be happening on cros, which won't pick up my DumpWithoutCrashing instrumentation for a while still.

It looks like the cros wallpaper manager is an app that uses the filesystem api for storage. Charlie and I are doing some manual testing to see if we can find any regression there.

Couldn't repro anything w/ the latest cros dev, but that makes sense because the latest cros dev doesn't have my fix. All the samples are actually from 55.0.2882.0, which is a pre-dev cros version. I bet these are coming from manual testing.

+bshe, +xdai because of the possibility that this could be a bad regression for the chromeos wallpaper manager, that hasn't made it to dev yet.

We have a report, and it's on Windows: go/crash/a5fcdccb00000000

hmmm... Maybe something to do with DropboxExt64.65536.dll?

Also, just to document on the bug:


0x00007ffb56b1ac82	(chrome.dll -child_process_security_policy_impl.cc:806 )	content::ChildProcessSecurityPolicyImpl::HasPermissionsForFileSystemFile(int,storage::FileSystemURL const &,int)
0x00007ffb56b87bd3	(chrome.dll -fileapi_message_filter.cc:359 )	content::FileAPIMessageFilter::OnReadDirectory(int,GURL const &)


So this is happening on a directory read.

The origin URL isn't preserved, but the offsets in |parsed| are preserved, where I see a scheme that's 15 chars long and a host that's 8 chars. That may indicate "chrome-devtools://devtools".

However, local experimentation with chrome-devtools suggests that chrome-devtools pages can, in fact, use the filesystem api and work properly with the CanCommitURL check. If you right-click->inspect, GrantScheme("chrome-devtools") occurs with the following call stack:

>	content::ChildProcessSecurityPolicyImpl::SecurityState::GrantScheme	C++
 	content::ChildProcessSecurityPolicyImpl::GrantRequestURL	C++
 	content::RenderFrameHostImpl::UpdatePermissionsForNavigation	C++
 	content::RenderFrameHostImpl::Navigate	C++
 	content::NavigatorImpl::NavigateToEntry	C++
 	content::NavigatorImpl::NavigateToPendingEntry	C++
 	content::NavigationControllerImpl::NavigateToPendingEntryInternal	C++
 	content::NavigationControllerImpl::NavigateToPendingEntry	C++
 	content::NavigationControllerImpl::LoadEntry	C++
 	content::NavigationControllerImpl::LoadURLWithParams	C++
 	content::NavigationControllerImpl::LoadURL	C++
 	DevToolsWindow::Create	C++

+lushnikov for possible insights on possible devtools/filesystem interactions. I see devtools_file_helper.cc but don't know what it's hooked up to. Any guidance on how devtools is using the filesystem apis?

The evidence we have so far suggests a chrome-devtools origin FileSystemURL being accessed from a process that lacks permission for the chrome-devtools: scheme; the error seems more prevalent on chromeos than elsewhere.

assigning to creis@ to peek at bug reports and continue driving this discussion tomorrow (I'll be back Monday).

dgozman@: Do you know if there's anything that would read a filesystem URL with a chrome-devtools://devtools origin from a non-DevTools process (per comment 19)?

We're hoping to account for these cases so that we can proceed with a security fix.

In the meantime, I noticed that the vast majority of UMA occurrences (1092) happened on a single day (Monday) *for a single user* on ChromeOS:
https://uma.googleplex.com/timeline_v2/?sid=40000cfe11aa0c44e8aa11c4cbada131

Most days have about 30-40 occurrences across about 10 users, with some on Windows and some on ChromeOS.  That suggests this is a real issue but rare, with one outlier on Monday.

Still only 1 crash report.

We use file system API, but only from devtools process. I'm not aware of any other usage. Maybe that's just a wrong url someone has?

Thanks for letting us know.  Is there a good way to check in code whether a process is a devtools process (ideally from content)?  We could try to add some crash keys to confirm or deny.

Sorry, not much progress.  Back to nick@.

re #c24: not really, we rely on isolation logic to put chrome-devtools:// into a separate renderer.

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

commit 7fd5f88a9dbe923676a8b97caf3aad08eed631fd
Author: nick <nick@chromium.org>
Date: Mon Oct 10 18:34:25 2016

Revert "[Merge to M54] Disallow file api access from processes that lack permissions for the scheme of the origin."

This reverts commit c9ac10a66c02609504c34849f8bb17e6f60d3a5f.

Reverting because of bug 654479: data from canary suggests that this fix may
be having unintended behavior.

BUG= 644966 ,654479
TBR=creis@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2405933002
Cr-Commit-Position: refs/branch-heads/2840@{#702}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/7fd5f88a9dbe923676a8b97caf3aad08eed631fd/content/browser/child_process_security_policy_impl.cc

The instrumentation added in #8 is a top 20 crash in chrome OS

https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_ChromeOS%27%20AND%20product.version%3D%2756.0.2920.0%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27content%3A%3AChildProcessSecurityPolicyImpl%3A%3AHasPermissionsForFileSystemFile%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D&stbtiq=&reportid=1248404500000000&index=0#0

Number 5 crash in M56 dev which goes beta next week. 

Albert do you know who else can remove the instrumentation causing the crash?

If the DumpWithoutCrashing in r424260 is the concern, I think that can be safely reverted from the M56 branch.  We'll do that today and I'll follow up with Nick about this bug on Monday.

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

commit e4085dea2218d9110c806d78ca0488db3abe0491
Author: Lucas Furukawa Gadani <lfg@chromium.org>
Date: Fri Dec 02 22:08:47 2016

Revert "Add temporary DumpWithoutCrashing to understand why the filesystem URL origin checks are failing in practice."

This reverts commit 0a66889a04ad4c87af24aeaa567184bd052227bd.

BUG=654479
NOTRY=true
NOPRESUBMIT=true
R=creis@chromium.org

Review URL: https://codereview.chromium.org/2551613002 .

Cr-Commit-Position: refs/branch-heads/2924@{#304}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/e4085dea2218d9110c806d78ca0488db3abe0491/content/browser/child_process_security_policy_impl.cc

I am seeing bunch of these now in M-57, see


https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_ChromeOS%27%20AND%20product.version%3D%2757.0.2987.6%27%20AND%20custom_data.ChromeCrashProto.ptype%3D%27browser%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27%5BDump%20without%20crash%5D%20content%3A%3AChildProcessSecurityPolicyImpl%3A%3AHasPermissionsForFileSystemFile%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D


Product name: Chrome_ChromeOS
Magic Signature: [Dump without crash] content::ChildProcessSecurityPolicyImpl::HasPermissionsForFileSystemFile

Current link:
https://crash.corp.google.com/browse?q=product.name%3D'Chrome_ChromeOS'%20AND%20product.version%3D'57.0.2987.6'%20AND%20custom_data.ChromeCrashProto.ptype%3D'browser'%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D'%5BDump%20without%20crash%5D%20content%3A%3AChildProcessSecurityPolicyImpl%3A%3AHasPermissionsForFileSystemFile'%20AND%20ReportID%3D'a79cf2c680000000'&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D#3


Search properties:
product.name: Chrome_ChromeOS
product.version: 57.0.2987.6
custom_data.chromecrashproto.ptype: browser
custom_data.chromecrashproto.magic_signature_1.name: [Dump without crash] content::ChildProcessSecurityPolicyImpl::HasPermissionsForFileSystemFile
reportid: a79cf2c680000000

Metadata :
Product Name: Chrome_ChromeOS
Product Version: 57.0.2987.6
Report ID: a79cf2c680000000
Report Time: Thu, 26 Jan 2017 18:54:14 GMT
Uptime: 246613 ms
Cumulative Uptime: 0 ms
User Email: 
OS Name: Linux
OS Version: 0.0.0 Linux 3.18.0-13733-g6a96b8f #1 SMP PREEMPT Mon Jan 23 19:56:38 PST 2017 x86_64
CPU Architecture: amd64
CPU Info: family 6 model 76 stepping 4

Thread 14 CRASHED [DUMP_REQUESTED @ 0x00006451f15136bd ] MAGIC SIGNATURE THREAD
Stack Quality100%Show frame trust levels
0x00006451f15136bd	(chrome -exception_handler.cc:654 )	google_breakpad::ExceptionHandler::WriteMinidump()
0x00006451ef8ae8f4	(chrome -dump_without_crashing.cc:23 )	base::debug::DumpWithoutCrashing()
0x00006451eec260ff	(chrome -child_process_security_policy_impl.cc:815 )	content::ChildProcessSecurityPolicyImpl::HasPermissionsForFileSystemFile(int, storage::FileSystemURL const&, int)
0x00006451eebdf5b9	(chrome -blob_dispatcher_host.cc:121 )	content::BlobDispatcherHost::OnRegisterBlob(std::string const&, std::string const&, std::string const&, std::vector<storage::DataElement, std::allocator<storage::DataElement> > const&)
0x00006451eebdf943	(chrome -tuple.h:91 )	content::BlobDispatcherHost::OnMessageReceived(IPC::Message const&)
0x00006451edf23f0c	(chrome -browser_message_filter.cc:87 )	content::BrowserMessageFilter::Internal::OnMessageReceived(IPC::Message const&)
0x00006451edfbffd9	(chrome -message_filter_router.cc:22 )	IPC::MessageFilterRouter::TryFilters(IPC::Message const&)
0x00006451edfbdfed	(chrome -ipc_channel_proxy.cc:98 )	IPC::ChannelProxy::Context::TryFilters(IPC::Message const&)
0x00006451f0286e61	(chrome -ipc_channel_proxy.cc:133 )	IPC::ChannelProxy::Context::OnMessageReceived(IPC::Message const&)
0x00006451edfbd5e4	(chrome -ipc_channel_mojo.cc:388 )	IPC::ChannelMojo::OnMessageReceived(IPC::Message const&)
0x00006451edfbe644	(chrome -ipc_message_pipe_reader.cc:110 )	IPC::internal::MessagePipeReader::Receive(std::vector<unsigned char, std::allocator<unsigned char> > const&, base::Optional<std::vector<mojo::StructPtr<IPC::mojom::SerializedHandle>, std::allocator<mojo::StructPtr<IPC::mojom::SerializedHandle> > > >)
0x00006451edd35ede	(chrome -ipc.mojom.cc:248 )	IPC::mojom::ChannelStubDispatch::Accept(IPC::mojom::Channel*, mojo::internal::SerializationContext*, mojo::Message*)
0x00006451edf590eb	(chrome -interface_endpoint_client.cc:341 )	mojo::InterfaceEndpointClient::HandleValidatedMessage(mojo::Message*)
0x00006451edfbf004	(chrome -ipc_mojo_bootstrap.cc:646 )	ChannelAssociatedGroupController::Accept
0x00006451edf59002	(chrome -connector.cc:247 )	mojo::Connector::ReadAllAvailableMessages()
0x00006451edf5a517	(chrome -callback.h:85 )	mojo::Watcher::OnHandleReady(unsigned int)
0x00006451edf5196c	(chrome -callback.h:68 )	base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*)
0x00006451edf3a311	(chrome -message_loop.cc:421 )	base::MessageLoop::DoWork()
0x00006451edf3a9f2	(chrome -message_pump_libevent.cc:218 )	base::MessagePumpLibevent::Run(base::MessagePump::Delegate*)
0x00006451ef8e0137	(chrome -run_loop.cc:37 )	base::RunLoop::Run()
0x00006451eec01d74	(chrome -browser_thread_impl.cc:276 )	content::BrowserThreadImpl::IOThreadRun(base::RunLoop*)
0x00006451eec01f22	(chrome -browser_thread_impl.cc:311 )	content::BrowserThreadImpl::Run(base::RunLoop*)
0x00006451ef8f7388	(chrome -thread.cc:328 )	base::Thread::ThreadMain()
0x00006451ef8f45d5	(chrome -platform_thread_posix.cc:71 )	ThreadFunc
0x00007377f9226577	(libpthread-2.23.so -pthread_create.c:333 )	start_thread
0x00007377f7f7e6dc	(libc-2.23.so + 0x000f66dc )	clone

I dug into this some more today. There are probably multiple issues occurring, one of which might be ChromeOS specific.

There are almost as many Windows crash reports as there are ChromeOS reports, but the stacks look different. The ChromeOS stack zel reports above seems to have something to with creating a blob from a FileSystemURL. Whereas the Windows reports are almost all in PPAPI IPCs.

On the Windows crash report I opened, I saw some memory referencing chrome-extension://pkedcjkdefgpdelpbcmbmeomcjbeemfm/mirroring_common.js, which is the media router extension. It looks like this extension does do some fileapi stuff, but I'm not sure how this intersects with PPAPI at all.

nick: The Media Router Extension (all JS code) shouldn't intersect with PPAPI at all. There's nothing Pepper or Flash specific in there.

I did a scan on a flattened dev build of the MR Extension, and there are some file references. However, they are all a part of the dynamic module-loading code in the goog.* (Closure) library. I don't think any file:// URLs are actually being read at runtime (the compiler probably just couldn't eliminate code that should never execute), other than through the chrome-extension:// protocol. Every thing else "file" related seems more about logging (to record the LOC that is logging an error), or has "file" in the API name, but is really about reading/writing in-memory Blobs (e.g., FileReader, or Blobs constructed from in-memory arrays).

The MR Extension also uses localStorage and chrome.storage.sync extensively, but would that contribute to this bug's issue?

Users experienced this crash on the following builds:

Win Canary 58.0.3021.0 -  0.25 CPM, 1 reports, 1 clients (signature [Dump without crash] content::ChildProcessSecurityPolicyImpl::HasPermissionsForFileSystemFile)

If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates.

- Go/Fracas

Sounds like we have repro steps for this in issue 705295, involving drag and drop of a Google Drive file from the Files App.  This uses URLs like filesystem:chrome-extension://hhaomjibdihmijegdhdafkllkbggdgoj/external/drive-...

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

commit c1ef2baa3dc05e2879e28bc22301e6bd713adad3
Author: lukasza <lukasza@chromium.org>
Date: Tue Apr 18 19:11:11 2017

Remove DumpWithoutCrashing for HasPermissionsForFileSystemFile/CanCommitURL.

This DumpWithoutCrashing turned out to be non-actionable (see
https://crbug.com/654479#c35).  Furthermore some forbidden accesses seem
unavoidable and in such cases DumpWithoutCrashing causes unnecessary
jank/latency during legitimate drag-and-drop scenarios (in case of
cross-site dragover/dragenter/dragleave that drags filesystem URIs - see
https://crbug.com/705295#c30).

BUG=654479, 705295

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

[modify] https://crrev.com/c1ef2baa3dc05e2879e28bc22301e6bd713adad3/content/browser/child_process_security_policy_impl.cc

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

commit daca1351774c52279fede4e295f2a29642bc331c
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Apr 28 15:43:09 2017

Remove DumpWithoutCrashing for HasPermissionsForFileSystemFile/CanCommitURL.

This DumpWithoutCrashing turned out to be non-actionable (see
https://crbug.com/654479#c35).  Furthermore some forbidden accesses seem
unavoidable and in such cases DumpWithoutCrashing causes unnecessary
jank/latency during legitimate drag-and-drop scenarios (in case of
cross-site dragover/dragenter/dragleave that drags filesystem URIs - see
https://crbug.com/705295#c30).

BUG=654479, 705295

Review-Url: https://codereview.chromium.org/2826693003
Cr-Commit-Position: refs/heads/master@{#465317}
(cherry picked from commit c1ef2baa3dc05e2879e28bc22301e6bd713adad3)

Review-Url: https://codereview.chromium.org/2853533002 .
Cr-Commit-Position: refs/branch-heads/3071@{#295}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/daca1351774c52279fede4e295f2a29642bc331c/content/browser/child_process_security_policy_impl.cc

Hrm, reading through issue 705295 .... is there anything left to do here? Did we determine this is expected behavior?