1357846 - Test dom/html/test/test_filepicker_default_directory.html fails without home directory read access

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Haik Aftandilian [:haik]]

Testing with security.sandbox.content.level=3 on Mac, the test dom/html/test/test_filepicker_default_directory.html fails. This reproduces locally for me and on try. The error I receive is

  Passed: 0
  Failed: 1
  Todo: 0
  failed | uncaught exception - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get] at doApply@chrome://specialpowers/content/specialpowersAPI.js:146:10 apply@chrome://specialpowers/content/specialpowersAPI.js:211:30 @http://mochi.test:8888/tests/dom/html/test/test_filepicker_default_directory.html:24:30 

This will probably apply to all platforms.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Ok, so the offending code in the test is:

> var defaultUploadDirectory = Cc["@mozilla.org/file/directory_service;1"]
>                             .getService(Ci.nsIDirectoryService)
>                             .QueryInterface(Ci.nsIProperties)
>                             .get("Desk", Ci.nsIFile);

It's getting the Directory Service, and then trying to get the desktop as an IFile. Getting the IFile produces a stat (|file-read-metadata|) on the home directory (|/Users/agaynor/|) somewhere, still tracking down where exactly that happens.

There's two obvious solutions:

1) Allow |stat("/Users/agaynor")|, it's not clear me that there's any risk for this very specific case, but it probably should be avoided in the general principle of "Don't expand the content processes permissions" (on macOS at least, getcwd() require you to have file-read*, file-read-metadata is not sufficient)

2) Remote the stat to the parent process. What is the prior art here? Obviously there should not be a GetDesktopPath() RPC, since it exposes data from the parent that's not part of the web security model (specifically the desktop's path contains the current OS's user). How do tests usually get access to stuff they need from the parent?

Comment 2

[Haik Aftandilian [:haik]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #1)
> Ok, so the offending code in the test is:
> 
> > var defaultUploadDirectory = Cc["@mozilla.org/file/directory_service;1"]
> >                             .getService(Ci.nsIDirectoryService)
> >                             .QueryInterface(Ci.nsIProperties)
> >                             .get("Desk", Ci.nsIFile);
> 
> It's getting the Directory Service, and then trying to get the desktop as an
> IFile. Getting the IFile produces a stat (|file-read-metadata|) on the home
> directory (|/Users/agaynor/|) somewhere, still tracking down where exactly
> that happens.
> 
> There's two obvious solutions:
> 
> 1) Allow |stat("/Users/agaynor")|, it's not clear me that there's any risk
> for this very specific case, but it probably should be avoided in the
> general principle of "Don't expand the content processes permissions" (on
> macOS at least, getcwd() require you to have file-read*, file-read-metadata
> is not sufficient)

That's definitely OK for now. Let's go with this.

We will want to eliminate information leaks like this, but for now the priority is read access to sensitive files. And the OS username is definitely already exposed to the content process in many ways. Two examples: the content-writable temporary directory is under $HOME and the profile path is passed on the command line.

> 2) Remote the stat to the parent process. What is the prior art here?

If the stat is in code we control, we could remote it, but it wouldn't be worth it in this case. We'd add extra complexity to prevent an information leak to a compromised content process, but that information is available by other means.

> Obviously there should not be a GetDesktopPath() RPC, since it exposes data
> from the parent that's not part of the web security model (specifically the
> desktop's path contains the current OS's user). How do tests usually get
> access to stuff they need from the parent?

From Javascript, you can use the message manager to send messages between processes. You can install handlers temporarily for the test using nsIMessageListenerManager::addMessageListener. But I recommend checking with someone with more experience writing Firefox tests before going that route.

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

Ok, after some hacking, I now have remoted the calls from JS to Chrome (wasn't too messy), but we're in trouble because the real problem is here: https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#435

This is pretty deep in the DOM stack; I'd appreciate some advice on how to best not do file IO in the middle of the DOM :-)

Comment 4

[Haik Aftandilian [:haik]]

baku, is this something you could provide some advice on?

The context is that we're testing removing of filesystem read access from the content process.

And it sounds like calling NS_GetSpecialDirectory(NS_OS_DESKTOP_DIR, getter_AddRefs(localFile)) always results in trying to stat the directory. Is that what you found Alex? I know some uses of nsIFile don't result in the file being touched. For example, InitWithPath.

Assuming we can't call NS_GetSpecialDirectory(NS_OS_DESKTOP_DIR, ...), we might have to change the interface nsIFilePicker so that it can be used from the child without having to stat or read the default directory. We could pass a directory service key or path string to the file picker instead of an nsIFile object. Or perhaps we could change teh PFilePickerParent RecvOpen method work so that if no directory is set, the parent uses the same algorithm as UploadLastDir::ContentPrefCallback::HandleCompletion.

The PFilePicker::Open message is used by the child to tell the parent to open the file dialog and that message uses strings to encode the default directory.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: specialDir.patch

I would not set 'Desk' as default display directory. We cannot touch displayDirectory attribute, because it is used by addons.

Here I introduce an extra attribute called: nsIFilePicker.displaySpecialDirectory. When this is set, the picker, in the parent process, will call NS_GetSpecialDirectory().

Would be nice to force NS_GetSpecialDirectory() to be parent process only.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8860327 [details] [diff] [review]
specialDir.patch


> // Set the display directory
> NS_IMETHODIMP nsBaseFilePicker::SetDisplayDirectory(nsIFile *aDirectory)
> {
>+  if (!mDisplaySpecialDirectory.IsEmpty()) {
>+    return NS_OK;
>+  }
>+
So we don't set DisplayDirectory if special directory is already set.


>+// Set the display special directory
>+NS_IMETHODIMP nsBaseFilePicker::SetDisplaySpecialDirectory(const nsAString& aDirectory)
>+{
>+  if (mDisplayDirectory && mDisplaySpecialDirectory.IsEmpty()) {
>+    return NS_OK;
>+  }
but here the behavior is different.
Why this? Why mDisplaySpecialDirectory.IsEmpty is special

>+
>+  mDisplaySpecialDirectory = aDirectory;
>+  if (mDisplaySpecialDirectory.IsEmpty()) {
>+    mDisplayDirectory = nullptr;
>+    return NS_OK;
>+  }
>+
>+  return NS_GetSpecialDirectory(NS_ConvertUTF16toUTF8(mDisplaySpecialDirectory).get(),
>+                                getter_AddRefs(mDisplayDirectory));
>+}
This does not work the way the .idl says.


>+// Get the display special directory
>+NS_IMETHODIMP nsBaseFilePicker::GetDisplaySpecialDirectory(nsAString& aDirectory)
>
>+  aDirectory = mDisplaySpecialDirectory.IsEmpty();
>+  return NS_OK;
>+}
Does this even compile, or pass any tests?

Comment 7

[Alex Gaynor [:Alex_Gaynor]]

Attachment: 1357846-tests.diff

Change to the tests to make them not access $HOME.

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

This patch to the tests (and firefox.js -- that part should not be landed yet!) should make it starightforward to reproduce the issue.

Comment 9

[Andrea Marchesini [:baku]]

> So we don't set DisplayDirectory if special directory is already set.

> but here the behavior is different.
> Why this? Why mDisplaySpecialDirectory.IsEmpty is special

Not really. I use only 1 variable to set the directory: mDisplayDirectory.
If the setter of DisplayDirectory is used but we have mDisplaySpecialDirectory set, I don't continue.
If the setter of DisplaySpecialDirectory is used but we have mDisplayDirectory without having mDisplaySpecialDirectory set, I don't continue because it means that, before, the setter of DisplayDirectory has been called.

> This does not work the way the .idl says.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: specialDir.patch

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

Combining our two patches, it does appear to fix the issue that with sandbox level 3, you now default to the desktop correctly on file inputs again, however the tests fail for me:

1 INFO TEST-START | dom/html/test/test_filepicker_default_directory.html
2 INFO TEST-PASS | dom/html/test/test_filepicker_default_directory.html | DisplayDirectory is null
3 INFO TEST-PASS | dom/html/test/test_filepicker_default_directory.html | DisplaySpecialDirectory matches the path
GECKO(79210) | JavaScript error: chrome://specialpowers/content/SpecialPowersObserverAPI.js, line 357: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIPrefBranch.setCharPref]

Comment 12

[Andrea Marchesini [:baku]]

Alex_Gaynor, I'm landing my patch because it fixes the issue for HTMLInputElement. Can you investigate what else blocks this bug?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/272037dc7f57
Introducing nsIFilePicker.displaySpecialDirectory, r=smaug

Comment 14

[Alex Gaynor [:Alex_Gaynor]]

Yup, I'll dig into getting this to pass end to end. Thanks for jumping onto this so quickly, we appreciate it!

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/272037dc7f57

Comment 16

[Alex Gaynor [:Alex_Gaynor]]

Whoops, didn't mean to close it earlier... not sure how I did that.

Comment 17

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1357846 - Remove some dead code which triggers errors at sandbox level 3

This code only became dead very recently; in the first patch improving this
issue (which fixed an underlying issue with the code, but not the tests).

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

Comment 18

[Andrea Marchesini [:baku]]

Comment on attachment 8862436 [details]
Bug 1357846 - Remove some dead code which triggers errors at sandbox level 3

https://reviewboard.mozilla.org/r/134370/#review137326

Comment 19

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8862436 [details]
Bug 1357846 - Remove some dead code which triggers errors at sandbox level 3

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134370/diff/1-2/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd0632c4d21f
Remove some dead code which triggers errors at sandbox level 3 r=baku

Comment 21

[Haik Aftandilian [:haik]]

Comment on attachment 8862436 [details]
Bug 1357846 - Remove some dead code which triggers errors at sandbox level 3

https://reviewboard.mozilla.org/r/134370/#review137422

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd0632c4d21f