Closed Bug 1393259 Opened 3 years ago Closed 2 years ago

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


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

57 Branch



Tracking Status
firefox60 --- fixed


(Reporter: haik, Assigned: haik)


(Blocks 1 open bug)


(Whiteboard: sb+)


(2 files, 3 obsolete files)

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.
Priority: -- → P3
Whiteboard: sb+
See Also: → 1382260
See Also: → 1404919
See Also: → 1407457
(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.
Summary: Tighten font rules in the Mac content sandbox → [Mac] Remote access to fonts from custom directories, font managers
Depends on: 1417420
Duplicate of this bug: 1417242
Assignee: nobody → haftandilian
Priority: P3 → P1
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.
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/
@@ +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/
@@ +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)
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 "")).

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.
Not review-ready yet. This patch adds read access to the sandbox extensions as described in comment 6.
Attachment #8934238 - Attachment is obsolete: true
Depends on: 1405088
No longer depends on: 1405088
I'm going to target build 60 with this fix.
Target Milestone: --- → Future
See Also: → 1432567
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 on attachment 8944970 [details]
Bug 1393259 - Enable sandbox read access extensions for font access.

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 on attachment 8944970 [details]
Bug 1393259 - Enable sandbox read access extensions for font access.

Fixed. Thanks for the review. Yeah, gladly no new IPDL required!
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)
Pushed by
Enable sandbox read access extensions for font access. r=Alex_Gaynor
Closed: 2 years ago
Resolution: --- → FIXED
Blocks: 1432567
Depends on: 1460917
You need to log in before you can comment on or make changes to this bug.