1367560 - dom/filesystem/tests/test_basic.html ASSERT failure on OS X with level 3 due to /Volumes restriction

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Haik Aftandilian [:haik]]

On OS X, with a debug build, with security.sandbox.content.level=3, the test dom/filesystem/tests/test_basic.html triggers an ASSERT failure in the content process.

This appears to be triggered by the /Volumes restriction added in bug 1363179.

I tried running the test with '(allow file-read* (literal "/Volumes"))' added to the rules and that let the test pass. Whether or not that's the right fix needs more thought.

Process 44710 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0 XUL`mozilla::dom::Directory::Create() at Directory.cpp:92
   89  	#ifdef DEBUG
   90  	  bool isDir;
   91  	  nsresult rv = aFile->IsDirectory(&isDir);
-> 92  	  MOZ_ASSERT(NS_SUCCEEDED(rv) && isDir);
   93  	#endif
   94  	
   95  	  RefPtr<Directory> directory = new Directory(aParent, aFile, aFileSystem);
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0 XUL`mozilla::dom::Directory::Create() at Directory.cpp:92
    frame #1 XUL`mozilla::dom::GetDirectoryListingTaskChild::SetSuccessRequestResult() at GetDirectoryListingTask.cpp:152
    frame #2 XUL`mozilla::dom::FileSystemTaskChildBase::SetRequestResult() at FileSystemTaskBase.cpp:209
    frame #3 XUL`mozilla::dom::FileSystemTaskChildBase::Recv__delete__() at FileSystemTaskBase.cpp:219
    frame #4 XUL`mozilla::dom::PFileSystemRequestChild::OnMessageReceived() at PFileSystemRequestChild.cpp:90
    frame #5 XUL`mozilla::ipc::PBackgroundChild::OnMessageReceived() at PBackgroundChild.cpp:1630
    frame #6 XUL`mozilla::ipc::MessageChannel::DispatchAsyncMessage() at MessageChannel.cpp:2040
    frame #7 XUL`mozilla::ipc::MessageChannel::DispatchMessage() at MessageChannel.cpp:1975
    frame #8 XUL`mozilla::ipc::MessageChannel::RunMessage() at MessageChannel.cpp:1844
    frame #9 XUL`mozilla::ipc::MessageChannel::MessageTask::Run() at MessageChannel.cpp:1877
    frame #10 XUL`mozilla::SchedulerGroup::Runnable::Run() at SchedulerGroup.cpp:370
    frame #11 XUL`nsThread::ProcessNextEvent() at nsThread.cpp:1273
    frame #12 XUL`NS_ProcessPendingEvents() at nsThreadUtils.cpp:335
    frame #13 XUL`nsBaseAppShell::NativeEventCallback() at nsBaseAppShell.cpp:97
    frame #14 XUL`nsAppShell::ProcessGeckoEvents() at nsAppShell.mm:399
    frame #15 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__() 
    frame #16 CoreFoundation`__CFRunLoopDoSources0() 
    frame #17 CoreFoundation`__CFRunLoopRun() 
    frame #18 CoreFoundation`CFRunLoopRunSpecific() 
    frame #19 HIToolbox`RunCurrentEventLoopInMode() 
    frame #20 HIToolbox`ReceiveNextEventCommon() 
    frame #21 HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter() 
    frame #22 AppKit`_DPSNextEvent() 
    frame #23 AppKit`-[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]() 
    frame #24 XUL`::-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](NSEventMask, NSDate *, NSString *, BOOL)() at nsAppShell.mm:130
    frame #25 AppKit`-[NSApplication run]() 
    frame #26 XUL`nsAppShell::Run() at nsAppShell.mm:673
    frame #27 XUL`XRE_RunAppShell() at nsEmbedFunctions.cpp:893
    frame #28 XUL`mozilla::ipc::MessagePumpForChildProcess::Run() at MessagePump.cpp:269
    frame #29 XUL`MessageLoop::RunInternal() at message_loop.cc:238
    frame #30 XUL`MessageLoop::RunHandler() at message_loop.cc:231
    frame #31 XUL`MessageLoop::Run() at message_loop.cc:211
    frame #32 XUL`XRE_InitChildProcess() at nsEmbedFunctions.cpp:709
    frame #33 XUL`mozilla::BootstrapImpl::XRE_InitChildProcess() at Bootstrap.cpp:65
    frame #34 plugin-container`content_process_main() at plugin-container.cpp:64
    frame #35 plugin-container`main() at MozillaRuntimeMain.cpp:25
    frame #36 plugin-container`start()

Comment 1

[Haik Aftandilian [:haik]]

The problem is that Directory::Create() can't be called in DEBUG mode to create a directory object for a directory the content process doesn't have access to. This is due to the following code extra DEBUG check.

http://searchfox.org/mozilla-central/rev/35b37316149108d53a02fb8498e250ea8a22ab5d/dom/filesystem/Directory.cpp#89
  
  Directory::Create(nsISupports* aParent, nsIFile* aFile,
                    FileSystemBase* aFileSystem)
  ...
    #ifdef DEBUG
      bool isDir;
      nsresult rv = aFile->IsDirectory(&isDir);
      MOZ_ASSERT(NS_SUCCEEDED(rv) && isDir);
    #endif
  ...

With sandboxing, we're not going to be able to rely on being able to access the directory so this code should go.

That's a better fix than adding a sandbox '(allow file-read* (literal "/Volumes"))' rule which would allow the test to pass, but wouldn't fix the problem for directory listing operations on other directories.

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Ok, I took a look at the rest of the |dom/| directory, and I couldn't find anywhere else that triggers this assert, so I think, even though that DEBUG block is clearly wrong in a sandboxed process, I'm going to see how hard it would be to move the Directory object to the parent, since it seems unfortunate to remove a debugging assertion for one callsite. If I've missed any tests and really a ton more our broken we should revisit.

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

I spent some more time thinking about this, and the potential for tests (or application code) that break as we tighten up the sandbox only increases.

More importantly, if this code is really going to be accessible from content, in an e10s sandboxed world, it doesn't make sense for Directory methods to be touching the filesystem, it just can't work as a general rule.

Given this, I think the originally proposed solution of just deleting that |#if DEBUG| branch makes the most sense. Before I send a patch for that, I'd like some feedback from the DOM folks who own this code. :baku, do you have any objections to this change?

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1367560 - Remove an assertion from the Directory constructor that the directory exists on disk from DEBUG builds;

As long as Directory objects can be constructed in the content process, trying
to access the file system conceptually does not make sense because of
sandboxing. The canary in the coal mine for this was tests which started
crashing on Directory construction as we further locked down read access in the
sandbox.

Review commit: https://reviewboard.mozilla.org/r/146234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146234/

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8874843 [details]
Bug 1367560 - Remove an assertion from the Directory constructor that the directory exists on disk from DEBUG builds;

https://reviewboard.mozilla.org/r/146234/#review150206

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a8333a5941e
Remove an assertion from the Directory constructor that the directory exists on disk from DEBUG builds; r=baku

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a8333a5941e