Bug 221610

Summary: [macOS] Deny mach-lookup to the fonts service
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebKit Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, bfulgham, cdumez, cmarcelo, ews-watchlist, mmaxfield, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
bfulgham: review+
Patch none

Description Per Arne Vollan 2021-02-09 08:47:17 PST 
Deny mach-lookup to the fonts service in the WebContent process on macOS.
Comment 1 Per Arne Vollan 2021-02-09 08:47:40 PST 
<rdar://problem/69168609>
Comment 2 Per Arne Vollan 2021-02-09 08:50:21 PST 
Created attachment 419727 [details]
Patch
Comment 3 Brent Fulgham 2021-02-09 09:54:56 PST 
Comment on attachment 419727 [details]
Patch

r=me
Comment 4 Per Arne Vollan 2021-02-09 10:00:27 PST 
Comment on attachment 419727 [details]
Patch

Thanks for reviewing!
Comment 5 EWS 2021-02-09 10:03:56 PST 
Committed r272586: <https://commits.webkit.org/r272586>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419727 [details].
Comment 6 Myles C. Maxfield 2021-02-10 08:46:09 PST 
Comment on attachment 419727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419727&action=review

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:1114
>  #if __MAC_OS_X_VERSION_MIN_REQUIRED <= 110000

shouldn't this be "< 120000"?
Comment 7 Per Arne Vollan 2021-02-10 12:17:59 PST 
(In reply to Myles C. Maxfield from comment #6)
> Comment on attachment 419727 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419727&action=review
> 
> > Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:1114
> >  #if __MAC_OS_X_VERSION_MIN_REQUIRED <= 110000
> 
> shouldn't this be "< 120000"?

That is a good point, although in this case, I think these should also be blocked when OS version > 11.

Thanks for reviewing!
Comment 8 Per Arne Vollan 2021-02-10 12:20:04 PST 
Reverted in <https://trac.webkit.org/changeset/272657/webkit>
Comment 9 Per Arne Vollan 2021-02-16 06:21:41 PST 
Created attachment 420465 [details]
Patch
Comment 10 Brent Fulgham 2021-02-16 10:04:35 PST 
Comment on attachment 420465 [details]
Patch

Nice! r=me
Comment 11 Per Arne Vollan 2021-02-17 07:21:11 PST 
Created attachment 420641 [details]
Patch
Comment 12 Per Arne Vollan 2021-02-17 07:22:30 PST 
Comment on attachment 420641 [details]
Patch

Thanks for reviewing!
Comment 13 EWS 2021-02-17 08:16:26 PST 
Committed r272999: <https://commits.webkit.org/r272999>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420641 [details].
Comment 14 Myles C. Maxfield 2021-03-10 17:15:50 PST 
Comment on attachment 420641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420641&action=review

> Source/WebKit/UIProcess/WebPageProxy.cpp:7692
> +static bool disableStaticFontRegistry()

"disable" is a verb, but the implementation of this function appears to not actually disable anything.
Comment 15 Chris Dumez 2021-03-10 17:25:18 PST 
Comment on attachment 420641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420641&action=review

> Source/WebKit/UIProcess/WebPageProxy.cpp:7686
> +        if (String(static_cast<CFStringRef>(font)) != "System")

Seems needlessly expensive to construct a WTF::String here:
if (CFStringCompare(font, CFSTR("System"), kCFCompareCaseSensitive) != kCFCompareEqualTo)