Closed Bug 1322716 (CVE-2017-5425) Opened 3 years ago Closed 3 years ago
Overly inclusive regex used in Mac GMP sandbox profile exposes users' data to GMP processes
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
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.
Removes the /private/var regex and adds Normalize() calls as described in comment 1.
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
(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 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...
Attachment #8820580 - Flags: review?(cpearce) → review+
Tested Amazon and Netflix streaming on OS X 10.12, 10.11, and 10.9.
Attachment #8820580 - Flags: review?(gpascutto) → review+
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.
Attachment #8822698 - Flags: approval-mozilla-aurora?
Comment on attachment 8822698 [details] [diff] [review] Removes /private/var regex from the GMP Sandbox restrict osx gmp sandbox some more, beta52+
Attachment #8822698 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Flagging this for manual testing per Comment 10.
Whiteboard: [post-critsmash-triage] sb+ → [post-critsmash-triage][adv-main52+] sb+
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!
You need to log in before you can comment on or make changes to this bug.