Closed Bug 1471977 Opened 6 years ago Closed 6 years ago

Mac Flash sandbox causing World Cup playback issues on foxsports.com

Categories

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

Product:
Core
Component:
Security: Process Sandboxing
Version:
62 Branch
Platform:
Unspecified
macOS
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- disabled
firefox62 + verified
firefox63 --- verified

People

(Reporter: haik, Assigned: haik)

References

Details

Attachments

(2 files)

Example screenshot
916.65 KB, image/png
Details
Bug 1471977 - Mac Flash sandbox causing World Cup playback issues on foxsports.com
59 bytes, text/x-review-board-request
Alex_Gaynor
: review+
ritu
: approval-mozilla-beta+
Details
Attached image Example screenshot — 
On the FoxSports website, trying to watch a World Cup stream fails on Nightly. After clicking to enable Flash, the user is shown options in the Flash applet to either "Watch Live" video or "Restart". After clicking to watch live video, the applet appears to almost start playing video and then reverts back to the same screen. See attached screenshot.

For now, the problem is reproducible at this URL.
https://www.foxsports.com/soccer/fifa-world-cup/group-g-live-stream-june-28-2018

Workaround:
This appears to be caused by the Flash sandbox which is currently enabled on Nightly 63 and Beta 62. To temporarily disable the Flash sandbox, set the pref dom.ipc.plugins.sandbox-level.flash to 0 in about:config.

If the pref dom.ipc.plugins.sandbox-level.flash is not present in about:config, then the Mac Flash sandbox is not enabled. It is not enabled on Firefox 61 or earlier.
Assignee: nobody → haftandilian
Priority: -- → P1
I looked at this today and, from the Console.app/console errors, the problem could be related to cert checking for an HTTPS connection. I added access to various services to the Flash sandbox that may be used for cert checks, but continue to see "ATS failed system trust" errors and 

  plugin-container[2013:3925162] NSURLSession/NSURLConnection HTTP load failed (kCFStreamErrorDomainSSL, -9802)

Tested with adding these services

  + (global-name "com.apple.trustd")
  + (global-name "com.apple.securityd")
  + (global-name "com.apple.trustd.agent")
  + (global-name "com.apple.identityservicesd")
  + (global-name "com.apple.keychaind")
  + (global-name "com.apple.TrustEvaluationAgent")
  + (global-name "com.apple.secd")

More debugging needed.
After testing more on 10.13 and 10.11, it appears TLS needs read access to the /private/var/db/mds directory, the /Library/Preferences/com.apple.security.plist file, and read-write access to some files in the cache dir in the mds directory such as /private/var/folders/.../C/mds/mds.lock. I'll test on other OS versions and then post for review.

I couldn't get this video streaming to work with Safari, but Safari's plugin sandbox rules grant read access to /private/var/db/mds and read/write access to the entire cache dir. Looking at profiles shipping with OS X, application.sb and other sandboxes also grant access to the mds files.

Here's one stack for the mds.lock file collected with opensnoop(1m) where the Flash plugin is calling SecItemCopyMatching which is a documented Apple API for getting keychain items.

/var/folders/95/r4mg_qn91b75dn960d8xcppw0000gn/C//mds/mds.lock
  libsystem_kernel.dylib`__open+0xa
  Security`Security::MDSSession::updateDataBases()+0x2af
  Security`Security::MDSSession::DbOpen(char const*, cssm_net_address const*, unsigned int, Security::AccessCredentials const*, void const*, long&)+0x91
  Security`mds_DbOpen(long, char const*, cssm_net_address const*, unsigned int, cssm_access_credentials const*, void const*, long*)+0xd8
  Security`Security::MDSClient::Directory::cdsa() const+0x5e
  Security`Security::MDSClient::Directory::dlGetFirst(cssm_query const&, cssm_db_record_attribute_data&, cssm_data*, cssm_db_unique_record*&)+0x26
  Security`Security::CssmClient::Table<Security::MDSClient::Common>::startQuery(Security::CssmQuery const&, bool)+0xff
  Security`Security::CssmClient::Table<Security::MDSClient::Common>::fetch(Security::CssmClient::Query const&, int)+0x90
  Security`MdsComponent::MdsComponent(Security::Guid const&)+0xd1
  Security`CssmManager::loadModule(Security::Guid const&, unsigned int, Security::ModuleCallback const&)+0x8c
  Security`CSSM_ModuleLoad+0x81
  Security`Security::CssmClient::ModuleImpl::activate()+0x9d
  Security`Security::CssmClient::AttachmentImpl::activate()+0x5c
  Security`Security::CssmClient::DbImpl::open()+0x64
  Security`Security::KeychainCore::KCCursorImpl::next(Security::KeychainCore::Item&)+0x13d
  Security`SecKeychainSearchCopyNext+0x70
  Security`SecItemCopyMatching_osx(__CFDictionary const*, void const**)+0x237
  Security`SecItemCopyMatching+0x194
  FlashPlayer-10.6`0x000000011bb032b0+0x1ac
  FlashPlayer-10.6`0x000000011bb03260+0x1d

Stack from a TLS callback:

/var/folders/95/r4mg_qn91b75dn960d8xcppw0000gn/C//mds/mds.lock
  libsystem_kernel.dylib`__open+0xa
  Security`Security::MDSSession::updateDataBases()+0x2af
  Security`Security::MDSSession::DbOpen(char const*, cssm_net_address const*, unsigned int, Security::AccessCredentials const*, void const*, long&)+0x91
  Security`mds_DbOpen(long, char const*, cssm_net_address const*, unsigned int, cssm_access_credentials const*, void const*, long*)+0xd8
  Security`Security::MDSClient::Directory::cdsa() const+0x5e
  Security`Security::MDSClient::Directory::dlGetFirst(cssm_query const&, cssm_db_record_attribute_data&, cssm_data*, cssm_db_unique_record*&)+0x26
  Security`Security::CssmClient::Table<Security::MDSClient::Common>::startQuery(Security::CssmQuery const&, bool)+0xff
  Security`Security::CssmClient::Table<Security::MDSClient::Common>::fetch(Security::CssmClient::Query const&, int)+0x90
  Security`MdsComponent::MdsComponent(Security::Guid const&)+0xd1
  Security`CssmManager::loadModule(Security::Guid const&, unsigned int, Security::ModuleCallback const&)+0x8c
  Security`CSSM_ModuleLoad+0x81
  Security`Security::CssmClient::ModuleImpl::activate()+0x9d
  Security`Security::CssmClient::AttachmentImpl::activate()+0x5c
  Security`Security::KeychainCore::Certificate::clHandle()+0x98
  Security`allowedEVRootsForLeafCertificate+0x6d
  Security`Security::KeychainCore::Trust::evaluate(bool)+0x52
  Security`SecTrustCopyPublicKey+0x65
  Security`tls_set_peer_pubkey+0x108
  Security`tls_handshake_message_callback+0x134
  libsystem_coretls.dylib`SSLProcessHandshakeRecordInner+0xea
See Also: → 1474375
[Tracking Requested - why for this release]:

We want to ship the Mac Flash sandbox in 62 (bug 1474375), but we need to fix this foxsports.com video bug (and an HBO Now video bug: https://github.com/webcompat/web-bugs/issues/17274) in Beta 62.

Here is a webcompat bug dupe of this foxsports.com video bug: https://github.com/webcompat/web-bugs/issues/17310
tracking-firefox62: --- → ?
See Also: 1474375 → 
I've tested my fix with Nightly on macOS 10.13 with HBO Now and confirmed that without the fix, the video fails and with the fix, I'm able to stream.
Tested on policy changes on 10.9, 10.10, 10.11, 10.13, and 10.14 Beta. Not 10.12.
Tracking for 62 since we need this fix on 62 in order to let the work in bug 1474375 go to beta/release in 62.
tracking-firefox62: ? → +
Comment on attachment 8990472 [details]
Bug 1471977 - Mac Flash sandbox causing World Cup playback issues on foxsports.com

https://reviewboard.mozilla.org/r/255542/#review263392

::: security/sandbox/mac/Sandbox.mm:168
(Diff revision 2)
> +    params.push_back("DARWIN_USER_CACHE_DIR");
> +    char confStrBuf[PATH_MAX];
> +    if (!confstr(_CS_DARWIN_USER_CACHE_DIR, confStrBuf, sizeof(confStrBuf))) {
> +      return false;
> +    }
> +    flashCacheDir = realpath(confStrBuf, nullptr);

This will crash in cases where `realpath` returns an error (`NULL`).

::: security/sandbox/mac/Sandbox.mm:168
(Diff revision 2)
> +    params.push_back("DARWIN_USER_CACHE_DIR");
> +    char confStrBuf[PATH_MAX];
> +    if (!confstr(_CS_DARWIN_USER_CACHE_DIR, confStrBuf, sizeof(confStrBuf))) {
> +      return false;
> +    }
> +    flashCacheDir = realpath(confStrBuf, nullptr);

This will crash in cases where `realpath` returns an error (`NULL`).

::: security/sandbox/mac/Sandbox.mm:179
(Diff revision 2)
>      params.push_back("DARWIN_USER_TEMP_DIR");
> -    char tempDir[PATH_MAX];
> +    if (!confstr(_CS_DARWIN_USER_TEMP_DIR, confStrBuf, sizeof(confStrBuf))) {
> -    if (!confstr(_CS_DARWIN_USER_TEMP_DIR, tempDir, sizeof(tempDir))) {
>        return false;
>      }
> -    flashTempDir = realpath(tempDir, nullptr);
> +    flashTempDir = realpath(confStrBuf, nullptr);

Oh, I see we weren't handling that here iether. We should :-)

::: security/sandbox/mac/SandboxPolicies.h:777
(Diff revision 2)
>  
> +  (allow file-read*
> +      (literal "/Library/Preferences/com.apple.security.plist")
> +      (subpath "/private/var/db/mds"))
> +  (allow file-read* file-write*
> +      (cacheDir-regex "/mds/mdsDirectory\.db_$")

None of these are actually regexes. Is there a reason not to do `cacheDir-literal`?
Comment on attachment 8990472 [details]
Bug 1471977 - Mac Flash sandbox causing World Cup playback issues on foxsports.com

https://reviewboard.mozilla.org/r/255542/#review263392

> This will crash in cases where `realpath` returns an error (`NULL`).

Good catch. I realized we're also leaking the return value of realpath() because std::string creates a copy of the string on assignment from a char*.

> None of these are actually regexes. Is there a reason not to do `cacheDir-literal`?

No, using literals would be better and I've updated the code to do that. Thanks. (The use of regexes was leftover from an earlier version and it didn't occur to me the rules don't need to be regexes anymore.)
Comment on attachment 8990472 [details]
Bug 1471977 - Mac Flash sandbox causing World Cup playback issues on foxsports.com

https://reviewboard.mozilla.org/r/255542/#review264042

r+ with one small remaining comment.

::: security/sandbox/mac/SandboxPolicies.h:776
(Diff revision 3)
>        (home-library-regex "/Application Support/Macromedia/ss\.(cfg|cfn|sgn)$"))
>  
> +  (allow file-read*
> +      (literal "/Library/Preferences/com.apple.security.plist")
> +      (subpath "/private/var/db/mds"))
> +  (allow file-read* file-write*

Can we cut down `file-write*` to which `file-write` it actually needs? (I think we mostly found everything else needs `file-write-create file-write-data` at most).
Attachment #8990472 - Flags: review?(agaynor) → review+
Comment on attachment 8990472 [details]
Bug 1471977 - Mac Flash sandbox causing World Cup playback issues on foxsports.com

https://reviewboard.mozilla.org/r/255542/#review264054

::: security/sandbox/mac/SandboxPolicies.h:776
(Diff revision 3)
>        (home-library-regex "/Application Support/Macromedia/ss\.(cfg|cfn|sgn)$"))
>  
> +  (allow file-read*
> +      (literal "/Library/Preferences/com.apple.security.plist")
> +      (subpath "/private/var/db/mds"))
> +  (allow file-read* file-write*

I tried to narrow that down, but found we do need both file-write-create and file-write-data (and I'll add that as a comment) and left it at that. And the lock file in the mds/ has the extended attributes set. I could test and try to eliminate some of the other operations, but I would rather leave it as file-write* if that's OK with you because I'm worried it might work well on the limited number of test setups/sites I have but cause issues if some other setups require the other write operations. And this code doesn't get much coverage. For a data point, the profiles in /System/Library/Sandbox also use file-write*. Is there a particular file-write operation apart from file-write-create and file-write-data you think is more risky?
Comment on attachment 8990472 [details]
Bug 1471977 - Mac Flash sandbox causing World Cup playback issues on foxsports.com

https://reviewboard.mozilla.org/r/255542/#review264054

> I tried to narrow that down, but found we do need both file-write-create and file-write-data (and I'll add that as a comment) and left it at that. And the lock file in the mds/ has the extended attributes set. I could test and try to eliminate some of the other operations, but I would rather leave it as file-write* if that's OK with you because I'm worried it might work well on the limited number of test setups/sites I have but cause issues if some other setups require the other write operations. And this code doesn't get much coverage. For a data point, the profiles in /System/Library/Sandbox also use file-write*. Is there a particular file-write operation apart from file-write-create and file-write-data you think is more risky?

There's no specific thing I'm worried about, just general trying to keep the permissions as limited as possible. If it's not practical to restrict it further, can you add a comment explaining why, and we'll call that good enough.
Comment on attachment 8990472 [details]
Bug 1471977 - Mac Flash sandbox causing World Cup playback issues on foxsports.com

https://reviewboard.mozilla.org/r/255542/#review264054

> There's no specific thing I'm worried about, just general trying to keep the permissions as limited as possible. If it's not practical to restrict it further, can you add a comment explaining why, and we'll call that good enough.

OK. I'll spend some more time on this. Looking at the profiles that ship in /System, some have more restrictive modes for the lock file and so that could probably be improved.
Comment on attachment 8990472 [details]
Bug 1471977 - Mac Flash sandbox causing World Cup playback issues on foxsports.com

https://reviewboard.mozilla.org/r/255542/#review264042

> Can we cut down `file-write*` to which `file-write` it actually needs? (I think we mostly found everything else needs `file-write-create file-write-data` at most).

On 10.13, I could limit this to file-write-create, file-write-data, and file-write-flags and still get functioning video, but I don't think it's worth it given the blackbox nature of the plugin and how I've seen slight differences with 10.9 - 10.13 so far. In addition, debugging seems to hampered here so it's more trial-and-error than normal. I suspect some violations and errors are disabled for obscurity regarding HDCP. Added comment.
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a1fbf6a16f5
Mac Flash sandbox causing World Cup playback issues on foxsports.com r=Alex_Gaynor
https://hg.mozilla.org/mozilla-central/rev/4a1fbf6a16f5
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Please nominate this for Beta approval when you get a chance. Note that it'll need a tiny bit of rebasing too.
Flags: needinfo?(haftandilian)
I've requested uplift of bug 1474375 and listed this bug as a dependency.
Flags: needinfo?(haftandilian)
I'd like us to verify the fixes in nightly for Bug 1475722, Bug 1471977, and Bug 1475707 before we uplift for beta. (Hopefully for beta 13 next Monday)   Not sure if this is reproducible exactly but if you can find a similar flash livestream on FoxSports it might be verifiable. If not, please let me know.
Flags: qe-verify+
Comment on attachment 8990472 [details]
Bug 1471977 - Mac Flash sandbox causing World Cup playback issues on foxsports.com

[Copied/pasted from bug 1474375 for which is blocked by this bug]

Approval Request Comment
[Feature/Bug causing the regression]:
This set of changes fixes some bugs with the current implementation of the Mac Flash sandbox which is a new feature that is only enabled on Nightly and early Beta builds. These changes turn on the Mac Flash sandbox in Beta 62 so it can ship with 62. The dependent bugs address file dialog crashes on OSX 10.9 and 10.10 and video streaming issues reported on foxsports.com and hbonow.com.

[User impact if declined]:
This feature improves the process isolation of the Mac Flash plugin process. The bugs being fixed here are only present in Nightly and early Beta builds.

[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]: 
QE should validate that common Flash sites continue to work on the different Mac OS versions we support.

[List of other uplifts needed for the feature/fix]:
Bug 1475722 - Mac Flash sandbox causes empty file upload dialogs on OS X 10.9, 10.10 r=Alex_Gaynor
Bug 1471977 - Mac Flash sandbox causing World Cup playback issues on foxsports.com r=Alex_Gaynor
Bug 1475707 - Mac Flash sandbox on Nightly/Beta causes OS X 10.9 file upload dialog Flash plugin crash r=Alex_Gaynor

[Is the change risky?]:
Medium risk.

[Why is the change risky/not risky?]:
The Mac Flash sandbox was first enabled on Nightly on April 2nd and then on early beta builds of Beta61/62. It went through PI testing at that time. The changes don't make major changes, but most tests have been manual and the risk is that Flash sites could be regressed. The Mac Flash sandbox does prevent some lesser used Flash functionality from working (see the bug comments) and a SUMO article is planned to document that. SUMO bug to be filed. 

[String changes made/needed]:
None
Attachment #8990472 - Flags: approval-mozilla-beta?
I managed to reproduce the issue using an older version of Nightly (2018-07-13) on Mac OS 10.10.
I retested everything using latest Nightly 63.0a1 on Mac OS 10.10 and Mac OS 10.9 and the bug is not reproducing anymore.
status-firefox63: fixed → verified
Flags: qe-verify+
Comment on attachment 8990472 [details]
Bug 1471977 - Mac Flash sandbox causing World Cup playback issues on foxsports.com

Fix was verified on Nightly63, Beta62+
Attachment #8990472 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Adding qe-verify+ again for verification in beta 14.
Flags: qe-verify+
I verified the fix on beta 62.0b14 using Mac OS 10.10 and Mac OS 10.9 and the bug is not reproducing anymore.
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: