Test dom/plugins/test/mochitest/test_bug406541.html fails without home directory read access

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: haik, Assigned: Alex_Gaynor)

Tracking

55 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: sbmc2)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
On try, on OS X, with security.sandbox.content.level=3, dom/plugins/test/mochitest/test_bug406541.html always fails.

01:37:45     INFO - TEST-UNEXPECTED-FAIL | dom/plugins/test/mochitest/test_bug406541.html | Plugin A should spawn 
01:37:45     INFO -     initialLoad/window.onmessage@dom/plugins/test/mochitest/test_bug406541.html:78:11
01:37:45     INFO -     EventHandlerNonNull*initialLoad@dom/plugins/test/mochitest/test_bug406541.html:77:9
01:37:45     INFO -     EventListener.handleEvent*runTest@dom/plugins/test/mochitest/test_bug406541.html:62:5

01:37:45     INFO - TEST-PASS | dom/plugins/test/mochitest/test_bug406541.html | Plugin B should NOT spawn 
01:37:45     INFO - Not taking screenshot here: see the one that was previously logged
01:37:45     INFO - TEST-UNEXPECTED-FAIL | dom/plugins/test/mochitest/test_bug406541.html | Plugin C should spawn 
01:37:45     INFO -     initialLoad/window.onmessage@dom/plugins/test/mochitest/test_bug406541.html:80:11
01:37:45     INFO -     EventHandlerNonNull*initialLoad@dom/plugins/test/mochitest/test_bug406541.html:77:9
01:37:45     INFO -     EventListener.handleEvent*runTest@dom/plugins/test/mochitest/test_bug406541.html:62:5

Example try push results (that also included dev code to remove access to /private/var):

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=150f4341d4db5e7fca69fc61b4c986ac9da4bc5d
(Reporter)

Updated

2 years ago
Blocks: 1332190
Whiteboard: sbmc2
(Assignee)

Updated

2 years ago
Assignee: nobody → agaynor
(Assignee)

Comment 1

2 years ago
This is passing for me locally:

0 INFO SimpleTest START
1 INFO TEST-START | dom/plugins/test/mochitest/test_bug406541.html
2 INFO TEST-PASS | dom/plugins/test/mochitest/test_bug406541.html | Plugin A should spawn
3 INFO TEST-PASS | dom/plugins/test/mochitest/test_bug406541.html | Plugin B should NOT spawn
4 INFO TEST-PASS | dom/plugins/test/mochitest/test_bug406541.html | Plugin C should spawn
GECKO(90026) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
GECKO(90026) | MEMORY STAT | vsize 3884MB | residentFast 144MB | heapAllocated 23MB
5 INFO TEST-OK | dom/plugins/test/mochitest/test_bug406541.html | took 1326ms
6 INFO TEST-START | Shutdown
7 INFO Passed:  3
8 INFO Failed:  0
9 INFO Todo:    0
10 INFO Mode:    e10s
11 INFO Slowest: 1326ms - /tests/dom/plugins/test/mochitest/test_bug406541.html
12 INFO SimpleTest FINISHED
13 INFO TEST-INFO | Ran 1 Loops
14 INFO SimpleTest FINISHED

This is running at commit abe5868346c7 with the following patch applied:

diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -1022,7 +1022,7 @@ pref("security.sandbox.gpu.level", 1);
 // process is killed when all windows are closed, so a change will take effect
 // when the 1st window is opened.
 #if defined(NIGHTLY_BUILD)
-pref("security.sandbox.content.level", 2);
+pref("security.sandbox.content.level", 3);
 #else
 pref("security.sandbox.content.level", 1);
 #endif



:haik, are you able to reproduce this issue locally?
Flags: needinfo?(haftandilian)
(Reporter)

Comment 2

2 years ago
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #1)
> :haik, are you able to reproduce this issue locally?

No, the test passes locally for me.

As a first step, I recommend modifying the test to print out the paths that are used and compare the try/local results.
Flags: needinfo?(haftandilian)
(Assignee)

Comment 3

2 years ago
Thanks for confirming; I suspect this has to do with access to the source directory then, since try runs on a packaged build. I'll keep investigating.
(Assignee)

Comment 4

2 years ago
Ok, I'm very confused why home directory permissions would affect this; here are the results of |plugin.getJavaCodebase|:

INFO A: file:///private/var/folders/21/3kp8v0nx0ds4xpp12th8vydh0000gn/T/TemporaryItems/Temp-%7B77333444-4682-bd42-ac5d-21f6a62630c3%7D/
INFO C: file:///private/var/folders/21/3kp8v0nx0ds4xpp12th8vydh0000gn/T/TemporaryItems/Temp-%7B77333444-4682-bd42-ac5d-21f6a62630c3%7D/subdir_bug406541/

These are nowhere near |$HOME| so level 2 vs level 3 shouldn't matter. I'll keep digging.
(Assignee)

Comment 5

2 years ago
Hah, wait, I missed it. Your try run also includes |/private/var|. Since |TmpD| is in |/private/var| which wins that fight? I think this doesn't have anything to do with |$HOME| restrictions.
Flags: needinfo?(haftandilian)
(Reporter)

Comment 6

2 years ago
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #5)
> Hah, wait, I missed it. Your try run also includes |/private/var|. Since
> |TmpD| is in |/private/var| which wins that fight? I think this doesn't have
> anything to do with |$HOME| restrictions.

I've also hit the problem on try without the /private/var changes. Here are some older try runs with level=3 and some other dev code. Sorry, I don't have a run that just changed the level to 3. The second link here just did level 3, but it's old and the logs are no longer viewable.

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=1743376b96d91d17630342347e4c95e2dff7df78
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d190a932a872ef236d3e7ec721d5df92f657a95e

That Temp-<UUID> directory is the ContentTempDir that we explicitly give content read/write access to with

    ; bug 1237847
    (allow file-read*
        (subpath appTempDir))
    (allow file-write*
        (subpath appTempDir))
Flags: needinfo?(haftandilian)
(Reporter)

Comment 7

2 years ago
I got this to reproduce locally on OS X 10.9 on an older laptop. (I tested with a mozilla-central artifact build.)

I'm not sure if it is relevant, but one thing I noticed was that the ContentTempDir is in ~/Library/Caches/TemporaryItems/Temp-<UUID>/ rather than in /private/var/... (i.e., $TMPDIR) as it is on Sierra.

I don't know if the OS changed or something in our code has led to the location of the content temp dir changing on newer OS versions, but when we introduced the content temp dir for Mac, it was in ~.

I tested setting browser.tabs.remote.separateFileProcess=false with level=3 and confirmed that the web content process could still read from the content temp dir.
(Assignee)

Comment 8

2 years ago
I admit I'm even a bit more confused now; we appear to have a blanket whitelist for _all_ TemporaryItems: https://dxr.mozilla.org/mozilla-central/source/security/sandbox/mac/SandboxPolicies.h?q=Library%2FCaches%2FTemp&redirect_type=single#320-322 (bug 1360556 :-)).

I'm continuing to investigate, I guess this means it's failing on something besides file-read or file-read-metadata.
(Assignee)

Comment 9

2 years ago
After far too long, I realized this whole test is for a feature that no longer exists, we don't support java applets anymore!

:bsmedberg tried to delete this test entirely in https://hg.mozilla.org/integration/autoland/rev/9900d421e24e, but it was backed out. It looks like the backout was for unrelated reasons though: https://bugzilla.mozilla.org/show_bug.cgi?id=1335475#c36

:bsmedberg, is there any additional context not captured on that bug, or is re-landing https://hg.mozilla.org/integration/autoland/rev/9900d421e24e by itself ok?
Flags: needinfo?(benjamin)
Talked on IRC: it's fine to land the already-reviewed patch which removes this test.
Flags: needinfo?(benjamin)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8862912 - Flags: review?(haftandilian)
(Reporter)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8862912 [details]
Bug 1360223 - Remove test about loading Java from file: origins because it's no longer relevant several times over.

https://reviewboard.mozilla.org/r/134788/#review137732
Attachment #8862912 - Flags: review?(haftandilian) → review+
(Assignee)

Comment 13

2 years ago
This does not appear to represent an underlying bug about sandbox permissions, but just a bug in a test (that ought to be deleted). At this point this test doesn't actually exercise any production code, since Java plugins aren't supported anymore, it just exercises some test-integration-helpers.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 15

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e31961900c19
Remove test about loading Java from file: origins because it's no longer relevant several times over. r=haik
Keywords: checkin-needed
(Reporter)

Comment 16

2 years ago
I used dtruss on the 10.9 system to look for failing syscalls and can see we're failing in a Java-specific firefox code path calling realpath(). See stack below.

The implementation of realpath() starts at the base of the path and for each path component triggers the getattrlist syscall. In this case, we are calling Normalize for the test files such as $HOME/Library/Caches/TemporaryItems/Temp-{UUID}/subdir_bug406541.html. getattrlist() fails and as a result Normalize() fails. (I took a quick look at the realpath/readlist/stat source and couldn't see exactly how we end up in getattrlist, but don't want to spend more time on this.)

To summarize, calling nsIFile::Normalize on a file the content process has read access to will fail if the process doesn't have read access to all the directories in the path. This is going to be true for (at least) the Content Temp Dir, and $PROFILE/{chrome,extensions}. 

And that means that on 10.12 where the ContentTempDir is in /private/var, we would have seen this failure after removing read access to /private/var, just not on try for now because the tests run on OS X 10.10.

 8153/0x82ceb:  getattrlist("/Users/haik\0", 0x7FFF93C09CB4, 0x7FFF5142DAA0)             = -1 Err#1 
              libsystem_kernel.dylib`__getattrlist+0xa
              XUL`nsLocalFile::Normalize()+0x42
              XUL`NS_RelaxStrictFileOriginPolicy(nsIURI*, nsIURI*, bool)+0x1f4
              XUL`nsObjectLoadingContent::CheckJavaCodebase()+0xf0
              XUL`nsObjectLoadingContent::LoadObject(bool, bool, nsIRequest*)+0x483
              XUL`mozilla::dom::HTMLSharedObjectElement::StartObjectLoad()+0x64
              XUL`mozilla::detail::RunnableMethodImpl<mozilla::dom::HTMLSharedObjectElement*, void (mozilla::dom::HTMLSharedObjectElement::*)(), true, false>::Run()+0x27
              XUL`nsContentUtils::RemoveScriptBlocker()+0x88
              XUL`nsDocument::EndUpdate(unsigned int)+0xda
              XUL`nsHTMLDocument::EndUpdate(unsigned int)+0x1d
              XUL`nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&)+0xeb8
              XUL`mozilla::dom::NodeBinding::appendChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&)+0x11d
              XUL`mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*)+0x171
              XUL`js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)+0x336
              XUL`Interpret(JSContext*, js::RunState&)+0x917e
              XUL`js::RunScript(JSContext*, js::RunState&)+0x1dc
              XUL`js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*)+0x1ae
              XUL`js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*)+0xf0
              XUL`Evaluate(JSContext*, js::ScopeKind, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>)+0x2eb
              XUL`JS::Evaluate(JSContext*, JS::AutoObjectVector&, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>)+0x8c

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e31961900c19
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.