Bug 212634

Summary:

REGRESSION(r261387): Introduced sandbox violations

Product:

WebKit

Reporter:

Per Arne Vollan <pvollan>

Component:

WebKit Misc.

Assignee:

Per Arne Vollan <pvollan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, darin, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	darin: review+
Patch	none

Description Per Arne Vollan 2020-06-02 07:19:31 PDT

The change set r261387 introduced some new mach lookup sandbox violations, when attempting to close Launch Services connections that were not open.

Comment 1 Per Arne Vollan 2020-06-02 07:22:18 PDT

rdar://problem/63676658

Comment 2 Per Arne Vollan 2020-06-02 07:30:57 PDT

Created attachment 400819 [details]
Patch

Comment 3 Darin Adler 2020-06-02 09:28:12 PDT

Comment on attachment 400819 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:181
> +        auto connection = [objc_getClass("_LSDReadService") XPCConnectionToService];
> +        [connection invalidate];

How do we know this is the only connection we need to close? Seems very magical.

Also seems unnecessary to use a local variable here.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:186
>          ASSERT(String(uti.get()) = String(adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassMIMEType, CFSTR("text/html"), 0)).get()));

This assertion is using assignment ("="), not an equality check ("==").

Comment 4 Per Arne Vollan 2020-06-02 11:23:28 PDT

Created attachment 400845 [details]
Patch

Comment 5 Per Arne Vollan 2020-06-02 11:26:47 PDT

(In reply to Darin Adler from comment #3)
> Comment on attachment 400819 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400819&action=review
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:181
> > +        auto connection = [objc_getClass("_LSDReadService") XPCConnectionToService];
> > +        [connection invalidate];
> 
> How do we know this is the only connection we need to close? Seems very
> magical.
> 
> Also seems unnecessary to use a local variable here.
> 

This is the only Launch Services connection type which is allowed (when holding the extension), so there cannot be any other Launch Services connections open.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:186
> >          ASSERT(String(uti.get()) = String(adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassMIMEType, CFSTR("text/html"), 0)).get()));
> 
> This assertion is using assignment ("="), not an equality check ("==").

Ah, good catch!

Thanks for reviewing!

Comment 6 EWS 2020-06-02 13:04:04 PDT

Committed r262435: <https://trac.webkit.org/changeset/262435>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400845 [details].

Comment 7 Simon Fraser (smfr) 2020-06-09 10:55:30 PDT

Comment on attachment 400845 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/cocoa/LaunchServicesSPI.h:93
> +@interface _LSDReadService : _LSDService
> +@end

Let's not use non-SPI stuff in future.