1322716 - (CVE-2017-5425) Overly inclusive regex used in Mac GMP sandbox profile exposes users' data to GMP processes

Status: VERIFIED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Haik Aftandilian [:haik]]

The GMP sandbox profile includes the following regex to allow access to files in /private/var.

From security/sandbox/mac/Sandbox.mm,

   135   "(allow file-read*\n"
   ...
   138   "    (regex #\"^/(private/)?var($|/)\")\n"

It's unlikely we need to allow read access to the entire /private/var directory tree which contains sensitive state from programs that use /private/var to store data. One example of private information being stored here is images received by text message through the OS X Messages app:

  /private/var/folders/.../com.apple.iChat/Messages/Images/<UUID>.jpg

The GMP process shouldn't be able to read personal pictures.

On OS X, the value of $TMPDIR is /var/folders/... where /var is a symlink to /private/var so the regex includes $TMPDIR which may be widely used. We should try to limit the regex to only include subdirectories we need.

Impact on the current _content_ sandbox is TBD. The content sandbox does allow access to some files in /private/var using regex that are much more constrained. I'd like to do another audit focusing on /private/var to confirm how much we allow.  Here's a list of files paths that I previously generated.

https://wiki.mozilla.org/Sandbox/OS_X_Rule_Set#How_security.sandbox.content.level_Affects_File_Access

Comment 1

[Haik Aftandilian [:haik]]

I couldn't find any indication that the GMP child process needs to be allowed to read from /private/var, but tests with the regex removed hit try failures when the profile was stored in /var/folders because the plugin library couldn't be loaded. (/var is a symlink to /private/var on OS X.)

When the GMP child process is launched, the first argument on the command line is a directory within the profile that contains the plugin dylib file (such as libwidevinecdm.dylib). The child process needs to be able to read the plugin binary from that directory in order to load the dylib. The loading of the dylib happens after the sandbox is enabled. This happens in GMPLoaderImpl::Load() where first we call mSandboxStarter->Start(aUTF8LibPath) and then later PR_LoadLibraryWithFlags(...).

The GMP child sandbox definition is modified at run time (before being applied) to include read permission of the full path to the dylib, which is derived from the directory passed to the GMP child process. On the failing try tests, the profile was being stored in /var/folders. An example dylib path is

  /var/folders/tg/r664rfr17rsdq6gszpzv8rzm00000w/T/
  tmpAxetwF.mozrunner/plugins/gmp-fakeopenh264/1.0/libfakeopenh264.dylib
  
As a result, the sandbox definition would include

  (allow file-read*
  ...
      (regex #"^/(private/)?var($|/)")
  ...
      (literal "/var/folders/tg/r664rfr17rsdq6gszpzv8rzm00000w/T/
                tmpAxetwF.mozrunner/plugins/gmp-fakeopenh264/1.0/libfakeopenh264.dylib")
  ...
  
For a profile in /private/var/, the "literal" rule doesn't grant the necessary permission to read the dylib. The dylib can be loaded because the regex permits it. To allow the path, the OS sandbox implementation needs to be able to read /var to determine it is a symlink to /private/var. And then it needs permission to read the file at /private/var/.../.dylib. The regex allows reading of /var and it allows reading of any path starting with /private/var/. As a result, removing the regex breaks the dylib loading for profiles in /var/folders.

We need the literal rule to be the path to the dylib with all symlinks resolved like what realpath(3) provides. In GMPChild.cpp:GetPluginPaths(), we have the following

  // Mac sandbox rules expect paths to actual files and directories -- not
  // soft links.
  aPluginDirectoryPath = GetNativeTarget(libDirectory);
  aPluginFilePath = GetNativeTarget(libFile);
  
but GetNativeTarget() only checks if the full path refers to a symlink, not if any directories earlier in the path are symlinks. nsIFile includes a Normalize() method that calls realpath on OS X and we can use that to solve this problem. With the additional Normalize() calls and the /private/var regex removed, try results were clean and manual browsing using the GMP process appeared to work. With the fix, the GMP sandbox definition ends up with

  (allow file-read*
  ...
      (literal "/private/var/folders/tg/r664rfr17rsdq6gszpzv8rzm00000w/T/
                tmpAxetwF.mozrunner/plugins/gmp-fakeopenh264/1.0/libfakeopenh264.dylib")
  ...

Try results:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=119f96c45ae49fbf11dd43d17770aa13b6cbd5db
  
So far testing included runs on OS X 10.10 on try and OS X 10.12 manually. I plan to test on 10.9 and 10.11 as well.

Comment 2

[Haik Aftandilian [:haik]]

Attachment: Rmoves /private/var regex from the GMP Sandbox

Removes the /private/var regex and adds Normalize() calls as described in comment 1.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8820580 [details] [diff] [review]
Rmoves /private/var regex from the GMP Sandbox

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

I'll r+, but I have little real comment to make.  r? cpearce, though I suspect he's the same.  gcp or whomever he nominates as mac sandbox expert would be the primary reviewer

Comment 4

[Haik Aftandilian [:haik]]

(In reply to Randell Jesup [:jesup] from comment #3)
> Comment on attachment 8820580 [details] [diff] [review]
> Rmoves /private/var regex from the GMP Sandbox
> 
> Review of attachment 8820580 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'll r+, but I have little real comment to make.  r? cpearce, though I
> suspect he's the same.  gcp or whomever he nominates as mac sandbox expert
> would be the primary reviewer

Thanks. I'm the Mac sandbox module owner. I thought someone from media should OK this.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8820580 [details] [diff] [review]
Rmoves /private/var regex from the GMP Sandbox

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

Provided Netflix still works, sure...

Comment 6

[Haik Aftandilian [:haik]]

Tested Amazon and Netflix streaming on OS X 10.12, 10.11, and 10.9.

Comment 7

[Haik Aftandilian [:haik]]

Attachment: Removes /private/var regex from the GMP Sandbox

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1cf1a98659d1578bc4cd8f0b0e687274e99c674

Comment 9

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

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

Comment 10

[Haik Aftandilian [:haik]]

Comment on attachment 8822698 [details] [diff] [review]
Removes /private/var regex from the GMP Sandbox

Approval Request Comment
[Feature/Bug causing the regression]:
Long standing GMP sandbox bug.

[User impact if declined]:
If a GMP plugin process was compromised, the sandbox would allow the process to read some private data from /private/var. This is where some temp files are stored, including pictures from picture messages. Requires a GMP exploit or malicious plugin before this bug can have an impact.

[Is this code covered by automated tests?]:
Partially. Issues with media playback aren't always caught by our automated tests.

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]:
Requesting QE run manual tests to validate that Netflix and Amazon video streaming still works for each version of OS X that we support. I've manually tested on OS X 10.12 and 10.11.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
Medium risk. The GMP sandbox is used for Netflix and Amazon streaming.

[Why is the change risky/not risky?]:
We don't know exactly which paths the closed-source plugins (that are run in the GMP sandbox) need access to, but automated and manual tests haven't revealed any problems with this change on Nightly.

[String changes made/needed]:
None.

Comment 11

[Haik Aftandilian [:haik]]

Aurora try push:

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

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 8822698 [details] [diff] [review]
Removes /private/var regex from the GMP Sandbox

restrict osx gmp sandbox some more, beta52+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e579f6ee4c6d

Comment 14

[Andrei Vaida [:avaida]]

Flagging this for manual testing per Comment 10.

Comment 15

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I verified on netflix using the folowing Mac OS X:
 - 10.9.5
 - 10.10.5
 - 10.11.6
 - 10.12.3
On the following channels:
 - Fx 54.0a1
 - Fx 53.0a2
 - Fx 52.0b9
 - Fx 52.0esr
 - Fx 45.8.0esr

The streaming went smooth. 

Cheers!