1484051 - [Mac] Flash sandbox allows file-read/write due to policy bug

Status: RESOLVED FIXED

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

Description

[Haik Aftandilian [:haik]]

The file access rules added for bug 1471977 is overly permissive. See below. Specifically the "require-all" should have been written to require the vnode-type REGULAR-FILE AND any of the cache-literal rules.

  ; Tests revealed file-write-{data,create,flags} required for some encrypted
  ; video playback. Allowing file-write* to match system profiles.
  (allow file-read* file-write*
      (cache-literal "/mds/mds.lock")
      (cache-literal "/mds/mdsDirectory.db_")
      (cache-literal "/mds/mdsDirectory.db_")
      (cache-literal "/mds/mdsObject.db")
      (cache-literal "/mds/mdsObject.db_")
      (require-all (vnode-type REGULAR-FILE)))

The Flash sandbox is currently only in Beta/Nightly. This bug defeats some of the file access restrictions provided by the Flash sandbox, but doesn't introduce a new security problem affecting users. Marking as a security bug for now.

Comment 1

[Haik Aftandilian [:haik]]

Attachment: Limit file-read/write to the intended mds files

The patch fixes the regression introduced in bug 1471977 where file-read/write access is mistakenly granted to all files readable/writeable by the user. The patch also fixes a bug masked by the regression, specifically having duplicate rules for "/mds/mdsDirectory.db_" and missing a rule for "/mds/mdsDirectory.db".

Comment 2

[Haik Aftandilian [:haik]]

Attachment: WIP test case. Not for landing.

A WIP patch I used to help demonstrate the bug and validate the fix. The patch changes the Mac test plugins to do some sandbox validation (specifically validates that writing to a file in $HOME fails) when they are initialized and MOZ_CRASH's on error causing several test plugin failures.

Comment 3

[Haik Aftandilian [:haik]]

I noticed this problem when testing on https://permadi.com/2011/02/flash-as3-saving-image-to-disk/ which is a Flash demo for saving a file to disk. Flash itself doesn't allow writing to disk without first displaying a file dialog, but the current version of the Flash sandbox should prevent that regardless.

I still have some additional tests to run to verify the fix on different OS versions. So far I've only tested on 10.13 and 10.14 Beta. Testing should include re-validating bug 1471977. I'm no longer able to stream the World Cup videos, but have tested some other encrypted videos on foxsports.com that I confirmed don't work without the fix for bug 1471977.

I will file a new bug to add some automated tests for the Mac Flash sandbox.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 9001879 [details] [diff] [review]
Limit file-read/write to the intended mds files

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

Nice catch -- sorry I missed this in the original review.

Since this is nightly/beta only, is it acceptable to have a more clear commit message?

Comment 5

[Haik Aftandilian [:haik]]

[Tracking Requested - why for this release]:
This bug affects the effectiveness of the Mac Flash sandbox which is a new feature shipping in build 62. The feature doesn't cause user-visible problems.

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Per Slack discussions with Haik, this should go into trunk without sec-approval since it is a moderate. Putting it on Beta will allow us to avoid shipping the problem.

Comment 7

[Haik Aftandilian [:haik]]

With the fix, I'm running into some issues with file dialogs. Mac Flash sandbox level 2 doesn't grant full read-access to the filesystem, but relies on sandbox extensions triggered by file dialog activity to make file dialogs work for things like file uploaders. On 10.12 and earlier, file dialogs are very buggy. As a result, I've filed bug 1484380 to switch to using level 1 by default. Level 2 provided a slight security improvement over level 1, but given the unreliability of the file dialog approach, I think the best course of action is to default to level 1.

Comment 8

[Haik Aftandilian [:haik]]

Attachment: Limit file-read/write to the intended mds files

Comment 9

[Haik Aftandilian [:haik]]

Comment on attachment 9002145 [details] [diff] [review]
Limit file-read/write to the intended mds files

Updated commit message.

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 9002145 [details] [diff] [review]
Limit file-read/write to the intended mds files

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

Patch looks correct.

I think, but I'm not positive, that it'd also work if you just removed the |(require-all)| wrapper around the |(vnode-type)| rule, but I think the explicit handling is easier to read.

Comment 11

[Haik Aftandilian [:haik]]

This needs to land on central after bug 1484380 which is on autoland at the moment.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/da16d500fe4e4ad3ca61a2fd3f51b4217bd834b0

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Please request Beta approval on this when you're comfortable doing so.

Comment 14

[Haik Aftandilian [:haik]]

Comment on attachment 9002145 [details] [diff] [review]
Limit file-read/write to the intended mds files

Approval Request Comment
[Feature/Bug causing the regression]:
Regression introduced by bug 1471977.

[User impact if declined]:
The Mac Flash sandbox filesystem restrictions won't work resulting in a compromised Flash process being able to write to the filesystem to any location the user has access to. (That is possible with Release/61, but the Mac Flash sandbox shipping in 62 is intended to prevent that.)

[Is this code covered by automated tests?]:
No

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

[Needs manual test from QE? If yes, steps to reproduce]:
We need to verify Flash file upload dialogs such as
[http://www.tinywebgallery.com/en/tfu/web_demo1.php] still allow users to select files. This should be tested on the different Mac OS versions we support.

We need to ensure bug 1484380 is still fixed. This can be verified by streaming live TV encrypted video on FoxSports.com which requires a TV login.

[List of other uplifts needed for the feature/fix]:
Bug 1484380 "[Mac] Default the Mac Flash sandbox to level 1". Bug 1484380 should be landed before this bug.

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
The change just affects the Mac Flash Plugin sandbox filesystem rules which means it only affects Flash and only some Flash applets. This is likely just Flash applets that use file uploaders or encrypted video streaming like foxsports.com and hbonow.com.

[String changes made/needed]:
None

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9002145 [details] [diff] [review]
Limit file-read/write to the intended mds files

Fixes an issue with the Mac Flash sandbox shipping enabled in 62 for the first time. Approved for 62.0b20.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7c4cd4c25782

Comment 17

[Bogdan Maris, Desktop Test Engineering]

Unfortunately we will not be able to verify this bug due to the fact that we don't have access to accounts on foxsports.com and hbonow.com. 
I'll go ahead and remove qe-verify+ from this bug for now, leaving the verification for someone who has access to those websites.