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

Reporter

Description

•

7 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

Haik Aftandilian [:haik]

Reporter

Updated

•

7 years ago

Blocks: 1332190

Whiteboard: sbmc2

Alex Gaynor [:Alex_Gaynor]

Assignee

Updated

•

7 years ago

Assignee: nobody → agaynor

Alex Gaynor [:Alex_Gaynor]

Assignee

Comment 1

•

7 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)

Haik Aftandilian [:haik]

Reporter

Comment 2

•

7 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)

Alex Gaynor [:Alex_Gaynor]

Assignee

Comment 3

•

7 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.

Alex Gaynor [:Alex_Gaynor]

Assignee

Comment 4

•

7 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.

Alex Gaynor [:Alex_Gaynor]

Assignee

Comment 5

•

7 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)

Haik Aftandilian [:haik]

Reporter

Comment 6

•

7 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)

Haik Aftandilian [:haik]

Reporter

Comment 7

•

7 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.

Alex Gaynor [:Alex_Gaynor]

Assignee

Comment 8

•

7 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.

Alex Gaynor [:Alex_Gaynor]

Assignee

Comment 9

•

7 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)

Benjamin Smedberg

Comment 10

•

7 years ago

Talked on IRC: it's fine to land the already-reviewed patch which removes this test.

Flags: needinfo?(benjamin)

Comment hidden (mozreview-request)

This is a re-land of 9900d421e24e, which was backed out.

r=qdot

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

Alex Gaynor [:Alex_Gaynor]

Assignee

Updated

•

7 years ago

Attachment #8862912 - Flags: review?(haftandilian)

Haik Aftandilian [:haik]

Reporter

Comment 12

•

7 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+

Alex Gaynor [:Alex_Gaynor]

Assignee

Comment 13

•

7 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)

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

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

Alex Gaynor [:Alex_Gaynor]

Assignee

Updated

•

7 years ago

Keywords: checkin-needed

Pulsebot

Comment 15

•

7 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

Haik Aftandilian [:haik]

Reporter

Comment 16

•

7 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

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

Comment 17

•

7 years ago

bugherder

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

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox55: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla55