Closed Bug 1436566 Opened 6 years ago Closed 6 years ago

[Mac] Land disabled-by-default sandboxing for the Flash NPAPI plugin process

Categories

(Core :: Security: Process Sandboxing, enhancement, P1)

Product:
Core
Component:
Security: Process Sandboxing
Version:
60 Branch
Platform:
Unspecified
macOS
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: haik, Assigned: haik)

References

Details

Attachments

(1 file)

Bug 1436566 - [Mac] Land disabled-by-default sandboxing for the Flash NPAPI plugin process.
59 bytes, text/x-review-board-request
Alex_Gaynor
: review+
jimm
: review+
Details 
As a first step towards enabling a sandbox by default for Flash NPAPI plugin process on Mac (bug 1433577), this bug is for landing a first version of the sandbox that is disabled by default. This will help with testing before the we are ready to enable the sandbox by default.
Assignee: nobody → haftandilian
Blocks: 1433577
Priority: -- → P1
The posted patch adds a disabled-by-default sandbox policy for the Flash plugin process. The sandbox can be enabled via pref security.sandbox.mac.flash.enabled or from the command line by setting MOZ_SANDBOX_MAC_FLASH_FORCE. When the sandbox is enabled, the parent passes an additional command line argument to the child plugin process. I don't expect this mechanism to be permanent. Assuming testing goes well and we choose to enable the sandbox by default, this code will probably have to be re-worked to support running Flash sandboxed/unsandboxed concurrently.
Comment on attachment 8949597 [details]
Bug 1436566 - [Mac] Land disabled-by-default sandboxing for the Flash NPAPI plugin process.

https://reviewboard.mozilla.org/r/218952/#review224794

::: dom/plugins/ipc/PluginModuleChild.h:317
(Diff revision 1)
>      NPError NPP_Destroy(PluginInstanceChild* instance) {
>          return mFunctions.destroy(instance->GetNPP(), 0);
>      }
>  
> +#if defined(OS_MACOSX)
> +    void EnableSafeMode(bool aShouldEnableLogging);

Is there a reason to call this "safe mode" instead of just "disable sandbox"?

::: dom/plugins/ipc/PluginModuleChild.cpp:57
(Diff revision 1)
>  
>  #ifdef MOZ_GECKO_PROFILER
>  #include "ChildProfilerController.h"
>  #endif
>  
> +#if defined(XP_MACOSX) && defined(MOZ_GMP_SANDBOX)

Is the GMP def the right one here?

::: dom/plugins/ipc/PluginModuleChild.cpp:305
(Diff revision 1)
>  #  error Please copy the initialization code from nsNPAPIPlugin.cpp
>  
>  #endif
>  
> +#if defined(OS_MACOSX)
> +    if ((aPluginFilename.find("Flash Player.plugin") != std::string::npos) &&

Eeep, is this really the right way to do this?

::: dom/plugins/ipc/PluginProcessChild.cpp:100
(Diff revision 1)
>      MOZ_ASSERT(values.size() >= 2, "not enough args");
>  
>      pluginFilename = UnmungePluginDsoPath(values[1]);
>  
> +#if defined(XP_MACOSX)
> +    if (values.size() >= 3 && values[2].compare("-safemode") == 0) {

Is there a reason to use `compare` instead of `operator==` here?

::: dom/plugins/ipc/PluginProcessParent.cpp:72
(Diff revision 1)
>  
>      vector<string> args;
>      args.push_back(MungePluginDsoPath(mPluginFilePath));
>  
> +#if defined(XP_MACOSX)
> +    if (aSandboxLevel > 0) {

Can you add a comment saying that on macOS sandboxLevel is really a boolean for "enabled"?

::: security/sandbox/mac/Sandbox.mm:132
(Diff revision 1)
>  bool StartMacSandbox(MacSandboxInfo const &aInfo, std::string &aErrorMessage)
>  {
>    std::vector<const char *> params;
>    std::string profile;
>    std::string macOSMinor = std::to_string(OSXVersion::OSXVersionMinor());
> +  std::string flashCacheDir, flashTempDir, flashPath;

Can we add a comment saying these need to be outside the block they're used in so the lifetime of the `c_str()` lasts as long as `params`?

::: security/sandbox/mac/SandboxPolicies.h:407
(Diff revision 1)
> +
> +  (if (string=? shouldLog "TRUE")
> +      (deny default)
> +      (deny default (with no-log)))
> +  (debug deny)
> +  (allow system-audit file-read-metadata)

What is `system-audit`?

::: security/sandbox/mac/SandboxPolicies.h:494
(Diff revision 1)
> +      (local udp))
> +
> +  (allow process-info-pidinfo)
> +  (allow process-info-setcontrol (target self))
> +
> +  (deny sysctl*)

Is this needed?

::: security/sandbox/mac/SandboxPolicies.h:515
(Diff revision 1)
> +          "kern.maxfilesperproc"
> +          "vm.footprint_suspend")))
> +
> +  ; Only supported on macOS 10.10+
> +  (if (defined? 'iokit-get-properties)
> +      (allow iokit-get-properties

Oh my. Given how many thigns are here, does it really make sense for us to try to block this?

::: security/sandbox/mac/SandboxPolicies.h:747
(Diff revision 1)
> +  (allow file-read* file-write* (subpath cacheDir))
> +  (allow file-read* file-write* (subpath tempDir))
> +
> +  (allow ipc-posix-sem
> +      (ipc-posix-name "MacromediaSemaphoreDig")
> +      (ipc-posix-name "59918130"))

Oh gosh, what is this?
Comment on attachment 8949597 [details]
Bug 1436566 - [Mac] Land disabled-by-default sandboxing for the Flash NPAPI plugin process.

https://reviewboard.mozilla.org/r/218952/#review224794

Thanks for reviewing. I considered simplifying this to rely on environment variables and limit changes to the child process, but one of the motivations for getting this landed in a disabled state is to allow SoftVision to test it and I think using a pref is going to be easier to manage for testing. I also added a temporary printf() that prints a line when the sandbox is enabled so testers can be sure the sandbox is being enabled.

> Is there a reason to call this "safe mode" instead of just "disable sandbox"?

I wanted to use the same Safe Mode naming as Safari because the concept here is the same, but admittedly we don't need that in the source code. Changed this to use sandbox naming.

> Is the GMP def the right one here?

No. I fixed this and several other places to use MOZ_SANDBOX.

> Eeep, is this really the right way to do this?

I should have fixed this earlier. I couldn't find a better way to do that so early in startup originally. I changed the code so that the child doesn't have to determine if it's the Flash plugin using the filename. If the filename changed this would break and we wouldn't know. Instead, the child uses the new command line parameters to know when to enable the sandbox.

> Is there a reason to use `compare` instead of `operator==` here?

No, fixed.

> Can you add a comment saying that on macOS sandboxLevel is really a boolean for "enabled"?

Done. Refactored the code to not use sandbox level.

> What is `system-audit`?

I think it allows use of the audit() system call. Without this, the Flash plugin crashes when attempting to print. This may be a side effect of it attempting to write a symlink in the tempdir which fails. I'll try to confirm what this is.

> Is this needed?

It should be covered by the default deny rule. Removing.

> Oh my. Given how many thigns are here, does it really make sense for us to try to block this?

We could still be blocking a lot more than we allow, but I've removed it. It's derived from WebKit and I don't know if any of it is specific to Flash.

> Oh gosh, what is this?

I don't know what's going on with this. Our Adobe contacts specifically mentioned semaphores needing to be in the sandbox profile so I left it in. There's a public WebKit changeset for this addition, but the comments don't provide any details: http://trac.webkit.org/changeset/152075/webkit
With the feature disabled:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dbe23f39a97d7262f016876060a3ce364a647a4

With a change to make the feature enabled by default:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a9f87c610c31e883eb2650d79aface4496fc1d8

I don't know what caused all the debug build failures, but I suspect it was an infra issue and I've resubmitted a debug run.
Just updated the code to make some minor changes. The main change is that the pref checking is now done in nsPluginTags.cpp like it is for Windows.
Comment on attachment 8949597 [details]
Bug 1436566 - [Mac] Land disabled-by-default sandboxing for the Flash NPAPI plugin process.

https://reviewboard.mozilla.org/r/218952/#review225656

LGTM; I think someone more familiar with the plugins codebase ought to take a quick look though.

::: security/sandbox/mac/SandboxPolicies.h:658
(Diff revision 3)
> +  (allow file-read* file-write* (subpath cacheDir))
> +  (allow file-read* file-write* (subpath tempDir))
> +
> +  (allow ipc-posix-sem
> +      (ipc-posix-name "MacromediaSemaphoreDig")
> +      (ipc-posix-name "59918130"))

If you know what this is, please add a comment for it :-)

::: security/sandbox/mac/SandboxPolicies.h:660
(Diff revision 3)
> +
> +  (allow ipc-posix-sem
> +      (ipc-posix-name "MacromediaSemaphoreDig")
> +      (ipc-posix-name "59918130"))
> +
> +  (allow file-read*

Please add a comment for this.

::: security/sandbox/mac/SandboxPolicies.h:688
(Diff revision 3)
> +  (allow file-read* file-write*
> +      (mount-relative-regex #"^/\.TemporaryItems/"))
> +
> +  (allow network-bind (local ip))
> +
> +  (if (defined? 'vnode-type)

This should always be defined for versions we support.
Attachment #8949597 - Flags: review?(agaynor) → review+
Comment on attachment 8949597 [details]
Bug 1436566 - [Mac] Land disabled-by-default sandboxing for the Flash NPAPI plugin process.

Jim, could you take a look at the changes to the generic plugin code here?
Attachment #8949597 - Flags: feedback?(jmathies)
Comment on attachment 8949597 [details]
Bug 1436566 - [Mac] Land disabled-by-default sandboxing for the Flash NPAPI plugin process.

https://reviewboard.mozilla.org/r/218952/#review225738

plugin code changes lgtm.

::: dom/plugins/base/nsPluginTags.cpp:445
(Diff revision 3)
> +
> +      // Enable sandbox logging in the plugin process if it's
> +      // been turned on via prefs or environment variables.
> +      if (Preferences::GetBool("security.sandbox.logging.enabled") ||
> +          PR_GetEnv("MOZ_SANDBOX_LOGGING") ||
> +          PR_GetEnv("MOZ_SANDBOX_MAC_FLASH_LOGGING")) {

please document these on the wiki once they land.

::: dom/plugins/ipc/PluginModuleChild.cpp:318
(Diff revision 3)
> +      if (!mozilla::StartMacSandbox(flashSandboxInfo, sbError)) {
> +          fprintf(stderr, "Failed to start sandbox:\n%s\n", sbError.c_str());
> +          return false;
> +      }
> +
> +      printf("Flash sandbox enabled.\n");

nit - debug printfs
Attachment #8949597 - Flags: review+
Comment on attachment 8949597 [details]
Bug 1436566 - [Mac] Land disabled-by-default sandboxing for the Flash NPAPI plugin process.

https://reviewboard.mozilla.org/r/218952/#review225656

> If you know what this is, please add a comment for it :-)

I did some testing with the semaphore rules removed and didn't run into any issues so I've removed them for now. I've asked our Adobe contact for more information about it. Better to remove them now before we start testing and then add them back if we find them necessary.

I've also removed the file-write rule for TemporaryItems.
Per Adobe, the MacromediaSemaphoreDig semaphore is used to implement Flash LocalConnection[1]. Given that, I'm adding it back because I suspect it's used for functionality we don't want to break--it can be used within a single .swf for synchronization or between multiple .swf's. There's no known reason to allow the "59918130" semaphore so I'm leaving that out.

It's a little concerning because in the future we expect to support Flash running sandboxed concurrently with the unsanboxed version if the user opts out of enabling the sandbox for a site and this might allow the sandboxed instance to meddle with the unsandboxed instance in some way. However, the semaphore API is limited quite limited.

1. https://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/net/LocalConnection.html
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88bcff60aab1
[Mac] Land disabled-by-default sandboxing for the Flash NPAPI plugin process. r=Alex_Gaynor,jimm
https://hg.mozilla.org/mozilla-central/rev/88bcff60aab1
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: