1393259 - [Mac] Remote access to fonts from custom directories, font managers

Status: RESOLVED FIXED

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

Description

[Haik Aftandilian [:haik]]

With the fix for bug 1382260, the Mac content sandbox permits content processes to read any file that appears to be a font based on the file extension. The file-extension-based rule was added as a low-risk fix to be improved upon later. This is more permissive than it needs to be and should be improved so that only activated font files can be read by content.

This was added to deal with font managers storing fonts in arbitrary locations on the filesystem that content must be able to read.

Some possible solutions to consider:

1) If we can get a callback to fire when a font fails to load, we could ask the parent to send the child the font. The child can then activate the font from memory. See CGFontCreateWithDataProvider().

2) Use CTFontManagerCopyAvailableFontURLs() to build a list of font paths and whitelist those font paths in the content sandbox.

Note: it's possible for the list of font directories and fonts in those directories to change while Firefox is running.

Comment 1

[Haik Aftandilian [:haik]]

(In reply to Haik Aftandilian [:haik] from comment #0)
> 1) If we can get a callback to fire when a font fails to load, we could ask
> the parent to send the child the font. The child can then activate the font
> from memory. See CGFontCreateWithDataProvider().

I don't think we'll need a callback. When we create a Skia type face (ScaledFontMac::GetSkTypeface()), we have the opportunity to test if the font file is accessible from the content process and at that point could do the remoting. Needs more investigation.

Comment 3

[Haik Aftandilian [:haik]]

Attachment: Prototype WIP patch that remotes access to fonts inaccessible from content processes

This patch is a rough prototype for remoting font access on Mac.

The patch adds IPC support so that content processes can request a font by name from the parent and receive back a file descriptor for the font file. The file is read into a buffer and a font is created in the content process for the named font. This is a synchronous message and for now PBackground is used. Instead of PBackground, this should be changed to use a sync top level protocol because disk I/O shouldn't be done on the PBackground thread in the parent.

The remoting is triggered in content processes in MacOSFontEntry::GetFontRef() when loading the font via CGFontCreateWithFontName() and [NSFont fontWithName:...] both fail. Comments in MacOSFontEntry::GetFontRef() there indicate that failure can happen for some fonts without sandboxing, but on macOS 10.13 I haven't observed that. Needs testing on older OS versions.

There are some other code paths that should trigger the font being read from a disk which aren't remoted yet and might also need to be changed: I've found nsCocoaUtils::GetNSMutableAttributedString(), gfxSingleFaceMacFontFamily::LocalizedName(), and _cairo_quartz_font_face_create_for_toy().

So far, I've tested this by removing access to font directories from the sandbox rules. Will be testing with some font managers.

Comment 4

[Haik Aftandilian [:haik]]

Attachment: Prototype WIP patch that remotes access to fonts inaccessible from content processes

Updated to use a new top-level protocol. Still a WIP. IPC teardown cleanup needed.

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 8934238 [details] [diff] [review]
Prototype WIP patch that remotes access to fonts inaccessible from content processes

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

This seems like a promising start, but I think there's some complexity still to be tackled: in particular, font files that contain multiple faces.

::: gfx/ipc/FontLoader.mm
@@ +25,5 @@
> +
> +  NSString* fontName = [NSString stringWithCharacters:reinterpret_cast<const unichar*>(aFontName.BeginReading()) length:aFontName.Length()];
> +
> +  /* XXX understand __bridge casting ownership */
> +  /* XXX check if we need to release any of these CF objects */

IIUC, you'd need to release the cfURLRef, because you obtain it from a Copy API.

@@ +29,5 @@
> +  /* XXX check if we need to release any of these CF objects */
> +  NSFont* font = [NSFont fontWithName:fontName size:0];
> +  CTFontRef coreFont = (__bridge CTFontRef)(font);
> +  CFURLRef cfURLRef = (CFURLRef)
> +    ::CTFontCopyAttribute(coreFont, kCTFontURLAttribute);

We'll need to figure out how to handle the case where the file (pointed at by the URL attribute) actually contains multiple font faces. This can be true for .dfont and .ttc/.otc fonts, and also for the legacy "font suitcase" files that some users appear to be activating via their font managers. (From the sandboxing-related bug reports we've seen, that seems to be more common than I'd have guessed.)

::: gfx/ipc/FontLoaderChild.cpp
@@ +154,5 @@
> +  auto platformHandle = fontFileFD.ClonePlatformHandle();
> +  PRFileDesc* prFileDesc = PR_ImportFile(PROsfd(platformHandle.release()));
> +
> +  fontBuffer->SetLength(fontFileSize);
> +  size_t bytesRead = PR_Read(prFileDesc, fontBuffer->Elements(), fontFileSize);

This tries to read the whole file; but in the cases where the file contains multiple faces, I don't think the resulting buffer will work to pass to CGFontCreateWithDataProvider which expects to deal with a single font resource.

I also wonder about resource-fork fonts (in the legacy "suitcase" files), which it seems some font managers still support.... does PR_Read have any idea what to do with that?

Maybe such cases could be handled by creating a CGFontRef, reading its font tables, and serializing to an in-memory sfnt buffer, like UnscaledFontMac::GetFontFileData does?

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +423,5 @@
> +      return nullptr;
> +    }
> +
> +    NSData* data = [NSData dataWithBytesNoCopy:mRemoteFontBuffer.Elements()
> +                           length:mRemoteFontBuffer.Length()];

I don't think there's any reason to mix Cocoa and CoreFoundation APIs here, just use CFDataCreateWithBytesNoCopy to create a CFDataRef instead.

@@ +435,5 @@
> +    if (!dataProvider) {
> +        return nullptr;
> +    }
> +
> +    CGFontRef fontRef = CGFontCreateWithDataProvider(dataProvider);

...and then release the dataProvider (it's up to the CGFont to retain a reference to it if it wants to continue having access).

Comment 6

[Haik Aftandilian [:haik]]

It looks as though we won't need any of this code.

I experimented more with CGFontCreateWithDataProvider() and found that there aren't any Apple API's to deal with multi-face fonts loaded with CGFontCreateWithDataProvider(). There is some support in Skia with makeFromStream(..., int ttcIndex).

I started to look into using sandbox extensions and found that calling CoreGraphics`CGFontCreateWithFontName() (as well as other font calls) already sets up a sandbox extension for the font file. In order to enable these extensions, we just need to add the catch-all rule to the sandbox to allow read access to these paths: (allow file-read* (extension "com.apple.app-sandbox.read")).

More testing is needed to confirm this works as expected and what the implications are. In limited testing, all the sandbox extension consume calls hit with the debugger when running Firefox were for font loading. That's important because we'd want to know if other sandbox extensions were automatically setup.

Comment 7

[Haik Aftandilian [:haik]]

Attachment: Enables sandbox extensions for read access to fonts outside standard dirs

Not review-ready yet. This patch adds read access to the sandbox extensions as described in comment 6.

Comment 8

[Haik Aftandilian [:haik]]

I'm going to target build 60 with this fix.

Comment 9

[Haik Aftandilian [:haik]]

Attachment: Bug 1393259 - Enable sandbox read access extensions for font access.

Enable sandbox read access extensions to allow content processes
to access fonts stored in non-standard locations without whitelisting
hardcoded directories. This is needed for configurations with third
party font managers that store fonts in their own directories or
user-specified directories.

Now that font access is not dependent on the filename extension
such as .otf and .ttf, remove the relevent tests.

Review commit: https://reviewboard.mozilla.org/r/190794/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190794/

Comment 10

[Haik Aftandilian [:haik]]

The posted fix removes whitelisting for 3rd-party-specific paths and adds "/Library/Application Support/Apple/Fonts" which contains some international fonts. We access those fonts directly during startup before the sandbox is enabled in gfxMacPlatformFontList::ActivateFontsFromDir(), but we plan to start the sandbox earlier with the fix for bug 1431441. We shouldn't need access to the system font dirs, but I'm leaving them in to be conservative. I'm working on an automated test on bug 1432567, but it's not ready to land yet. I've done manual tests so far by installing fonts in dirs the content process doesn't have access to and making sure the font was displayed correctly in Firefox.

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8944970 [details]
Bug 1393259 - Enable sandbox read access extensions for font access.

https://reviewboard.mozilla.org/r/190794/#review220884

Two nits, but otherwise LGTM! Amazing how simple this turned out to be :-)

::: security/sandbox/mac/SandboxPolicies.h:316
(Diff revision 1)
>        (vnode-type REGULAR-FILE)))
>  
> -  ; bug 1382260
> -  ; We may need to load fonts from outside of the standard
> -  ; font directories whitelisted above. This is typically caused
> -  ; by a font manager. For now, whitelist any file with a
> +  ; Fonts
> +  (allow file-read*
> +    (subpath "/Library/Fonts")
> +    (subpath "/System/Library/Fonts")

We already allow all of `/System` above, so this isn't needed.

::: security/sandbox/mac/SandboxPolicies.h:319
(Diff revision 1)
> -  (allow file-read*
> -    (regex #"\.[oO][tT][fF]$"           ; otf
> -           #"\.[tT][tT][fF]$"           ; ttf
> +    ; Allow read access to paths allowed via sandbox extensions.
> +    ; This is needed for fonts in non-standard locations normally
> +    ; due to third party font managers.

I think it'd be helpful to mention that the font server will automatically issue extensions.

Comment 12

[Haik Aftandilian [:haik]]

Comment on attachment 8944970 [details]
Bug 1393259 - Enable sandbox read access extensions for font access.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190794/diff/1-2/

Comment 13

[Haik Aftandilian [:haik]]

Comment on attachment 8944970 [details]
Bug 1393259 - Enable sandbox read access extensions for font access.

https://reviewboard.mozilla.org/r/190794/#review220884

Fixed. Thanks for the review. Yeah, gladly no new IPDL required!

Comment 14

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 91b42a52079159527dd2b2f5e2b62762eed866b8 -d 781a2e1af4a8: rebasing 443898:91b42a520791 "Bug 1393259 - Enable sandbox read access extensions for font access. r=Alex_Gaynor" (tip)
merging security/sandbox/mac/SandboxPolicies.h
warning: conflicts while merging security/sandbox/mac/SandboxPolicies.h! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 15

[Haik Aftandilian [:haik]]

Comment on attachment 8944970 [details]
Bug 1393259 - Enable sandbox read access extensions for font access.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190794/diff/2-3/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9218e87c25fb
Enable sandbox read access extensions for font access. r=Alex_Gaynor

Comment 17

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9218e87c25fb

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: Make the mac font activation/sandbox test (browser_Bug 1393259.js) more robust. r=haik

When the test font is activated, the notification of the font configuration change
may be handled asynchronously by content processes, so that it's possible the content
process has not yet handled the update at the point when registerFont() detects the
change and returns to the test script.

(This issue becomes more acute with the upcoming shared-font-list system, where the
OS notification is not handled by the content process at all; it's only handled by
the parent process, which then notifies content processes after it has updated the
font list. So there's an inherent latency between the update being recognized by the
chrome process - and therefore "ready" as far as the test script is concerned - and
content processes receiving and handling the change.)

To handle this, we can explicitly wait for the width of the rendered content to change,
which will indicate that the font configuration change has been handled by the content
process.

Comment 19

[Jonathan Kew [:jfkthame]]

Argh, that patch was supposed to be attached to bug 1542773, sorry! Let's see if I can change it in phabricator...

Comment 20

[Phabricator Automation]

Comment on attachment 9056632 [details]
Make the mac font activation/sandbox test (browser_Bug 1393259.js) more robust. r=haik

Revision D26570 was moved to bug 1542773. Setting attachment 9056632 [details] to obsolete.