I noticed some problems that were introduced by my fix for bug 1237847 "[e10s] Null deref crash when running test_pluginstream_newstream.html" which intended to allow the content process to write to a temporary directory on OS X under ~/Library. Previously, writes to the whole home directory outside of ~/Library were permitted. The fix allows writes to the entire ~ directory including ~/Library which was previously blocked. This affects Aurora/47 and Nightly/48. We have three problems. 1) The first problem with the fix is that the new rule to allow writes to the temporary directory is not built correctly due to an argument passing issue. In Sandbox.mm:StartMacSandbox(), aInfo.appTempDir is the empty string "" apparently due to the implicit struct copy of the MacSandboxInfo argument. I don't know exactly why this is yet. When appTempDir is "", the rules added in 1237847 become overly permissive. They allow writes to the whole home directory. Existing rule allowing writes to the home directory outside of ~/Library: 333 "; the following rules should be removed when printing and \n" 334 "; opening a file from disk are brokered through the main process\n" 335 " (if\n" 336 " (< sandbox-level 2)\n" 337 " (allow file*\n" 338 " (require-not\n" 339 " (home-subpath \"/Library\")))\n" 340 ... Rules added for 1237847: 426 "; bug 1237847\n" 427 " (allow file-read*\n" 428 " (home-subpath appTempDir))\n" 429 " (allow file-write*\n" 430 " (home-subpath appTempDir))\n" 431 " )\n" 2) Even if the writes at lines 427-430 were built correctly, the require-not at line 338 prevents writes within ~/Library (per my testing.) So the ruleset would need to be more complicated to allow writes to the temporary directory, but not the entire ~/Library directory. 3) The temporary directory is not being created by the parent process. In the GetAndCleanTempDir function we have the following code. 608 static already_AddRefed<nsIFile> 609 GetAndCleanTempDir() 610 { ... 620 rv = tempDir->Remove(/* aRecursive */ true); 621 if (NS_FAILED(rv) && rv != NS_ERROR_FILE_NOT_FOUND) { 622 NS_WARNING("Failed to delete temp directory."); 623 return nullptr; 624 } ... This code is intended to ignore tempDir->Remove() failures when the failure is a result of the tempDir not existing. However, on OS X, in that scenario, tempDir->Remove() fails and a different nsresult is returned. Instead of NS_ERROR_FILE_NOT_FOUND, it is NS_ERROR_FILE_TARGET_DOES_NOT_EXIST. This is due to rmdir(2) (which is used by Remove()) using an errno of ENOENT which is converted to NS_ERROR_FILE_TARGET_DOES_NOT_EXIST in nsresultForErrno() from xpcom/io/nsLocalFile.h. This causes the tempDir to never be created by the parent process. It's just luck that the testcase being fixed by 1237847 was in fact fixed given these errors.

The argument passing issue in #1 above is due to a missing initializer for the appTempDir field in the _MacSandboxInfo(const struct _MacSandboxInfo& other) constructor. Above in #2, I was wrong when I said the require-not rule prevents the new rules from working. With the argument passing issue fixed, the new rules added don't work because they use (home-subpath appTempDir) which would only work if appTempDir is a relative path from the home directory. It's an absolute path so just "subpath" should be used to allow reads and writes inside appTempDir.

Comment on attachment 8739297 [details] [diff] [review] Patches addressing the tempdir creation/destroy and rule set issues Review of attachment 8739297 [details] [diff] [review]: ----------------------------------------------------------------- This looks OK, but is there a reason for the aInfo parameter to StartMacSandbox not being a const reference? In fact do we need that explicit copy constructor at all? (Although I might misunderstand how this stuff works.) I also noticed that GMPLoaderImpl::SetSandboxInfo and MacSandboxStarter::SetSandboxInfo (called from the other) take a MacSandboxInfo*, which is passed in from GMPChild::SetMacSandboxInfo as a pointer to a local variable! MacSandboxStarter::SetSandboxInfo dereferences it, which means it gets copied, but it still seems like a bad idea to me.

Comment on attachment 8739297 [details] [diff] [review] Patches addressing the tempdir creation/destroy and rule set issues Review of attachment 8739297 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/sandbox/mac/Sandbox.h @@ +39,5 @@ > : type(MacSandboxType_Default), level(0) {} > _MacSandboxInfo(const struct _MacSandboxInfo& other) > : type(other.type), level(other.level), pluginInfo(other.pluginInfo), > appPath(other.appPath), appBinaryPath(other.appBinaryPath), > + appDir(other.appDir), appTempDir(other.appTempDir) {} As bob says I don't see the reason why we don't use the default here. If you want add =default (C++11) for clarity. ::: toolkit/xre/nsAppRunner.cpp @@ +622,5 @@ > } > > + // Don't return an error if the directory doesn't exist. > + // Windows Remove() returns NS_ERROR_FILE_NOT_FOUND while > + // OS X returns NS_ERROR_FILE_TARGET_DOES_NOT_EXIST. Please file a bug for this. That's just bonkers. Also, does it make sense to sniff out the exact error codes here? What else could go wrong that'd want to make you continue>

(In reply to Bob Owen (:bobowen) from comment #3) > This looks OK, but is there a reason for the aInfo parameter to > StartMacSandbox not being a const reference? > > In fact do we need that explicit copy constructor at all? (Although I might > misunderstand how this stuff works.) I don't see a good reason to not use a const reference or to use that constructor either. There's definitely some cruft here. > I also noticed that GMPLoaderImpl::SetSandboxInfo and > MacSandboxStarter::SetSandboxInfo (called from the other) take a > MacSandboxInfo*, which is passed in from GMPChild::SetMacSandboxInfo as a > pointer to a local variable! > MacSandboxStarter::SetSandboxInfo dereferences it, which means it gets > copied, but it still seems like a bad idea to me. Would it make sense to clean this up in a separate bug so that this fix is limited to the TempDir issues? Thanks for reviewing.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #4) > Comment on attachment 8739297 [details] [diff] [review] > Patches addressing the tempdir creation/destroy and rule set issues > > Review of attachment 8739297 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: security/sandbox/mac/Sandbox.h > @@ +39,5 @@ > > : type(MacSandboxType_Default), level(0) {} > > _MacSandboxInfo(const struct _MacSandboxInfo& other) > > : type(other.type), level(other.level), pluginInfo(other.pluginInfo), > > appPath(other.appPath), appBinaryPath(other.appBinaryPath), > > + appDir(other.appDir), appTempDir(other.appTempDir) {} > > As bob says I don't see the reason why we don't use the default here. If you > want add =default (C++11) for clarity. OK. This could be lumped in with a new cleanup bug. > > ::: toolkit/xre/nsAppRunner.cpp > @@ +622,5 @@ > > } > > > > + // Don't return an error if the directory doesn't exist. > > + // Windows Remove() returns NS_ERROR_FILE_NOT_FOUND while > > + // OS X returns NS_ERROR_FILE_TARGET_DOES_NOT_EXIST. > > Please file a bug for this. That's just bonkers. Will do. > Also, does it make sense to sniff out the exact error codes here? What else > could go wrong that'd want to make you continue> I'll look into it. Thanks.

Comment on attachment 8739297 [details] [diff] [review] Patches addressing the tempdir creation/destroy and rule set issues Review of attachment 8739297 [details] [diff] [review]: ----------------------------------------------------------------- Yes clean-up in a separate bug.

(In reply to Haik Aftandilian [:haik] from comment #6) > (In reply to Gian-Carlo Pascutto [:gcp] from comment #4) > > Comment on attachment 8739297 [details] [diff] [review] > > Patches addressing the tempdir creation/destroy and rule set issues > > > > Review of attachment 8739297 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: security/sandbox/mac/Sandbox.h > > @@ +39,5 @@ > > > : type(MacSandboxType_Default), level(0) {} > > > _MacSandboxInfo(const struct _MacSandboxInfo& other) > > > : type(other.type), level(other.level), pluginInfo(other.pluginInfo), > > > appPath(other.appPath), appBinaryPath(other.appBinaryPath), > > > + appDir(other.appDir), appTempDir(other.appTempDir) {} > > > > As bob says I don't see the reason why we don't use the default here. If you > > want add =default (C++11) for clarity. > > OK. This could be lumped in with a new cleanup bug. > > > > > ::: toolkit/xre/nsAppRunner.cpp > > @@ +622,5 @@ > > > } > > > > > > + // Don't return an error if the directory doesn't exist. > > > + // Windows Remove() returns NS_ERROR_FILE_NOT_FOUND while > > > + // OS X returns NS_ERROR_FILE_TARGET_DOES_NOT_EXIST. > > > > Please file a bug for this. That's just bonkers. > > Will do. > > > Also, does it make sense to sniff out the exact error codes here? What else > > could go wrong that'd want to make you continue> > > I'll look into it. Thanks. Looked at the other OS X rmdir possible errors and none of them look like things we would want to silently ignore. They all indicate something has gone wrong that we should debug.

Hello Haik, GCP, this bug is marked as affecting Fx47. Do you think we should uplift to Beta47 or let it ride the trains?

I think we should. The fix has received good test coverage because the code changes are exercised every time Firefox starts up and exits.

If we do not plan to enable e10s and/or the sandbox on 47 release then uplifting makes little sense? Or does this potentially affect things like GMP? Haik, if you think this should go on beta, then request approval-mozilla-beta on the patch.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #15) > If we do not plan to enable e10s and/or the sandbox on 47 release then > uplifting makes little sense? Or does this potentially affect things like > GMP? Thanks for pointing that out gcp. You're right this doesn't need to be uplifted to 47 unless we were going to turn on content sandboxing for 47. The bug that introduced this regression (1237847) did get uplifted to 47, but all the code isn't executed unless we build with MOZ_CONTENT_SANDBOX and MOZ_CONTENT_SANDBOX is only set for mozilla-central nightly builds, not aurora or beta. It doesn't affect the GMP sandbox because that doesn't have the rules to allow read/write to this temporary directory. It wasn't changed by 1237847. > Haik, if you think this should go on beta, then request > approval-mozilla-beta on the patch. OK, now I don't think it needs to be uplifted. Technically the code in the beta repo suffers from this problem, but the code is compiled out because content sandboxing is not enabled in 47 and isn't planned to be before 47 ships. (And now I know I have to set the approval-mozilla-<release> flag to request uplift.)