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

RESOLVED FIXED in Firefox 60

Status

()

P1
normal
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: haik, Assigned: haik)

Tracking

(Blocks: 1 bug)

57 Branch
Future
Unspecified
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: sb+)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
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.

Updated

a year ago
Priority: -- → P3
Whiteboard: sb+
(Assignee)

Updated

a year ago
See Also: → bug 1382260
(Assignee)

Updated

a year ago
See Also: → bug 1404919
(Assignee)

Updated

a year ago
See Also: → bug 1407457
(Assignee)

Comment 1

a year ago
(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.
Blocks: 1359559
(Assignee)

Updated

a year ago
Summary: Tighten font rules in the Mac content sandbox → [Mac] Remote access to fonts from custom directories, font managers

Updated

a year ago
Depends on: 1417420
(Assignee)

Updated

a year ago
Duplicate of this bug: 1417242
(Assignee)

Updated

a year ago
Assignee: nobody → haftandilian
(Assignee)

Updated

a year ago
Priority: P3 → P1
(Assignee)

Comment 3

a year ago
Created attachment 8932162 [details] [diff] [review]
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.
(Assignee)

Comment 4

a year ago
Created attachment 8934238 [details] [diff] [review]
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.
Attachment #8932162 - Attachment is obsolete: true
Attachment #8934238 - Flags: feedback?(jkew)
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).
Attachment #8934238 - Flags: feedback?(jkew)
(Assignee)

Comment 6

11 months ago
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.
(Assignee)

Comment 7

11 months ago
Created attachment 8940404 [details] [diff] [review]
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.
Attachment #8934238 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Depends on: 1405088
(Assignee)

Updated

10 months ago
No longer depends on: 1405088
(Assignee)

Comment 8

10 months ago
I'm going to target build 60 with this fix.
Target Milestone: --- → Future
(Assignee)

Updated

10 months ago
See Also: → bug 1432567
Comment hidden (mozreview-request)
(Assignee)

Comment 10

10 months ago
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

10 months ago
mozreview-review
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.
Attachment #8944970 - Flags: review?(agaynor) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 13

10 months ago
mozreview-review-reply
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

10 months ago
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 hidden (mozreview-request)

Comment 16

10 months ago
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9218e87c25fb
Enable sandbox read access extensions for font access. r=Alex_Gaynor

Comment 17

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9218e87c25fb
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
(Assignee)

Updated

7 months ago
Blocks: 1432567
(Assignee)

Updated

6 months ago
Depends on: 1460917
status-firefox57: affected → ---
You need to log in before you can comment on or make changes to this bug.