Closed Bug 1190032 Opened 6 years ago Closed 6 years ago

[e10s] Sandbox failure in nsPluginHost::GetPluginTempDir (deny file-write-create /Users/mstange/Library/Caches/TemporaryItems/plugtmp)

Categories

(Core :: Security: Process Sandboxing, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
e10s + ---
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: mstange, Assigned: smichaud)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached file sandbox report
I noticed this error message in my system console on 10.11 but don't steps to reproduce:

deny file-write-create /Users/mstange/Library/Caches/TemporaryItems/plugtmp

> mkdir + 10
> nsLocalFile::Create(unsigned int, unsigned int) + 32 (nsLocalFileUnix.cpp:497)
> nsLocalFile::CreateUnique(unsigned int, unsigned int) + 331 (nsLocalFileCommon.cpp:78)
> nsPluginHost::GetPluginTempDir(nsIFile**) + 234 (nsPluginHost.cpp:812)
> nsPluginStreamListenerPeer::SetupPluginCacheFile(nsIChannel*) + 548 (nsPluginStreamListenerPeer.cpp:379)
> nsPluginStreamListenerPeer::OnStreamTypeSet(int) + 138 (nsCOMPtr.h:296)
(In reply to Markus Stange [:mstange] from comment #0)
> I noticed this error message in my system console on 10.11 but don't steps
> to reproduce

"but I don't have steps to reproduce for it" is what I wanted to say.
Was there a crash or other visible problem associated with this message?
Blocks: 1083344
I also see this sandbox error on OS X 10.9.5, and presumably would see it on both 10.10.X and 10.11.

It happens every time we call nsPluginHost::GetPluginTempDir(), which we only do when setting up a plugin stream listener on behalf of a plugin.  So these errors happen loading any plugin capable of displaying "movies" or other complex objects -- e.g. QuickTime, Silverlight and Flash.

As best I can tell the error has no ill effect -- the plugin seems to continue working normally.  But we should still fix this.

Examples that I tested with:

* The movie previews at http://apple.com/trailers
* http://demos.telerik.com/silverlight/
No longer blocks: el-capitan
Summary: [10.11] Sandbox failure in nsPluginHost::GetPluginTempDir (deny file-write-create /Users/mstange/Library/Caches/TemporaryItems/plugtmp) → Sandbox failure in nsPluginHost::GetPluginTempDir (deny file-write-create /Users/mstange/Library/Caches/TemporaryItems/plugtmp)
Summary: Sandbox failure in nsPluginHost::GetPluginTempDir (deny file-write-create /Users/mstange/Library/Caches/TemporaryItems/plugtmp) → [e10s] Sandbox failure in nsPluginHost::GetPluginTempDir (deny file-write-create /Users/mstange/Library/Caches/TemporaryItems/plugtmp)
Attached patch Fix (obsolete) — Splinter Review
This gets rid of the error in my tests.  I also confirmed that the plugtmp directory does get created.
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attachment #8655660 - Flags: review?(areinald.bug)
Comment on attachment 8655660 [details] [diff] [review]
Fix

Review of attachment 8655660 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/sandbox/mac/Sandbox.mm
@@ +397,5 @@
>    "    (allow iokit-open\n"
>    "        (iokit-user-client-class \"NVDVDContextTesla\")\n"
>    "        (iokit-user-client-class \"Gen6DVDContext\"))\n"
>    "\n"
> +  "; bug 1190032\n"

Even if we want every rule associated with a bug number, it would be prettier to group this rule (and the comment) together with other file* rules.
Same for the 2 rules above.
But r+ anyway.
Attachment #8655660 - Flags: review?(areinald.bug) → review+
I ended up moving both bug-specific rules to the bottom of the ruleset.  Future bug-specific rules can presumably be added there.

Both of these rules are minimal changes needed to fix particular bugs.  Yes, at some point someone needs to rationalize and clean up our ruleset ... but I think that's a ways off yet.
Attachment #8655660 - Attachment is obsolete: true
Attachment #8656906 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c48f8f05e6d9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Jed Davis points out (in bug 1202910) that my previous patch won't fix all cases of this bug.  If nsLocalFile::CreateUnique() finds an existing plugtmp directory, it creates another after the pattern plugtmp-n.

http://hg.mozilla.org/mozilla-central/annotate/dd9e40b46959/xpcom/io/nsLocalFileCommon.cpp#l59

This revision of my patch should be able to deal with that.
Attachment #8658867 - Flags: review?(areinald.bug)
Comment on attachment 8658867 [details] [diff] [review]
Make patch fix general case

Oops, never mind.

Bug 1201935 will deal with this.
Attachment #8658867 - Flags: review?(areinald.bug)
Attachment #8658867 - Attachment is obsolete: true
(In reply to Steven Michaud [:smichaud] from comment #10)
> Bug 1201935 will deal with this.

I'm going to change that patch to not grant write permissions to all of TemporaryItems, because we don't actually need that for that bug, which means attachment 8658867 [details] [diff] [review] should be revived.
Thanks.  I'll wait until you're done with bug 1201935.
Comment on attachment 8658867 [details] [diff] [review]
Make patch fix general case

Since this is longer taken care of by bug 1201935 ...
Attachment #8658867 - Attachment is obsolete: false
Attachment #8658867 - Flags: review?(areinald.bug)
Comment on attachment 8658867 [details] [diff] [review]
Make patch fix general case

Review of attachment 8658867 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/sandbox/mac/Sandbox.mm
@@ +415,5 @@
>    "        (iokit-user-client-class \"Gen6DVDContext\"))\n"
>    "\n"
>    "; bug 1190032\n"
>    "    (allow file*\n"
> +  "        (home-regex \"/Library/Caches/TemporaryItems/plugtmp.*\"))\n"

If the pattern is plugtmp-n maybe we could use a more restrictive regex like (I'm not sure of the syntax):
(home-regex \"/Library/Caches/TemporaryItems/plugtmp(-[0-9]*)?\"))\n"

Just a suggestion you can try if you want, but r+ anyway.
Attachment #8658867 - Flags: review?(areinald.bug) → review+
Comment on attachment 8658867 [details] [diff] [review]
Make patch fix general case

I had a similar thought.  But I couldn't find the right syntax, and ultimately just decided to keep things simple.
You need to log in before you can comment on or make changes to this bug.