Bug 213379 - [macOS] Connections to the preference daemon are established before entering the sandbox
Summary: [macOS] Connections to the preference daemon are established before entering ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on: 213577
Blocks:
  Show dependency treegraph
 
Reported: 2020-06-19 08:02 PDT by Per Arne Vollan
Modified: 2020-06-30 15:19 PDT (History)
6 users (show)

See Also:


Attachments
Patch (11.86 KB, patch)
2020-06-19 13:11 PDT, Per Arne Vollan
darin: review+
Details | Formatted Diff | Diff
Patch (11.68 KB, patch)
2020-06-19 14:52 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (11.50 KB, patch)
2020-06-19 15:49 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (10.94 KB, patch)
2020-06-30 13:22 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2020-06-19 08:02:12 PDT
On macOS, connections to the preference daemon are established before entering the sandbox. These connections also persist after entering the sandbox and denying access to the preference daemon. There should not be attempts to connect to the preference daemon before entering the sandbox, since these attempts will not be stopped by the sandbox.
Comment 1 Per Arne Vollan 2020-06-19 13:11:49 PDT
Created attachment 402313 [details]
Patch
Comment 2 Darin Adler 2020-06-19 14:13:55 PDT
Comment on attachment 402313 [details]
Patch

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

> Source/WebKit/NetworkProcess/mac/NetworkProcessMac.mm:96
> +    StringBuilder path;
> +    path.append([webKit2Bundle resourcePath]);
> +    path.append("/");
> +    path.append("com.apple.WebKit.NetworkProcess.sb");
> +    sandboxParameters.setOverrideSandboxProfilePath(path.toString());

This is a job for makeString, not StringBuilder.

    sandboxParameters.setOverrideSandboxProfilePath(makeString([webKit2Bundle resourcePath], "/com.apple.WebKit.NetworkProcess.sb"));

Or use a local String variable if you like.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:229
> +    // We don't need to talk to the dock.

Capitalize Dock.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:233
> +    if (Class nsApplicationClass = NSClassFromString(@"NSApplication")) {
> +        if ([nsApplicationClass respondsToSelector:@selector(_preventDockConnections)])
> +            [nsApplicationClass _preventDockConnections];
> +    }

I know this code is pre-existing and only being moved, but this does seem risky — if this method is renamed, moved, or removed this code will just silently stop having any effect.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:589
> +    StringBuilder path;
> +    path.append([webKit2Bundle resourcePath]);
> +    path.append("/");
> +    path.append("com.apple.WebProcess.sb");
> +    sandboxParameters.setOverrideSandboxProfilePath(path.toString());

This is a job for makeString, not StringBuilder.

    sandboxParameters.setOverrideSandboxProfilePath(makeString([webKit2Bundle resourcePath], "/com.apple.WebKit.WebProcess.sb"));

Or use a local String variable if you like.
Comment 3 Per Arne Vollan 2020-06-19 14:52:43 PDT
Created attachment 402329 [details]
Patch
Comment 4 Per Arne Vollan 2020-06-19 14:54:12 PDT
(In reply to Darin Adler from comment #2)
> Comment on attachment 402313 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402313&action=review
> 
> > Source/WebKit/NetworkProcess/mac/NetworkProcessMac.mm:96
> > +    StringBuilder path;
> > +    path.append([webKit2Bundle resourcePath]);
> > +    path.append("/");
> > +    path.append("com.apple.WebKit.NetworkProcess.sb");
> > +    sandboxParameters.setOverrideSandboxProfilePath(path.toString());
> 
> This is a job for makeString, not StringBuilder.
> 
>    
> sandboxParameters.setOverrideSandboxProfilePath(makeString([webKit2Bundle
> resourcePath], "/com.apple.WebKit.NetworkProcess.sb"));
> 
> Or use a local String variable if you like.
> 

Fixed.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:229
> > +    // We don't need to talk to the dock.
> 
> Capitalize Dock.
> 

Done.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:233
> > +    if (Class nsApplicationClass = NSClassFromString(@"NSApplication")) {
> > +        if ([nsApplicationClass respondsToSelector:@selector(_preventDockConnections)])
> > +            [nsApplicationClass _preventDockConnections];
> > +    }
> 
> I know this code is pre-existing and only being moved, but this does seem
> risky — if this method is renamed, moved, or removed this code will just
> silently stop having any effect.
> 

I could be wrong, but it seems this method is supported.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:589
> > +    StringBuilder path;
> > +    path.append([webKit2Bundle resourcePath]);
> > +    path.append("/");
> > +    path.append("com.apple.WebProcess.sb");
> > +    sandboxParameters.setOverrideSandboxProfilePath(path.toString());
> 
> This is a job for makeString, not StringBuilder.
> 
>    
> sandboxParameters.setOverrideSandboxProfilePath(makeString([webKit2Bundle
> resourcePath], "/com.apple.WebKit.WebProcess.sb"));
> 
> Or use a local String variable if you like.

Fixed.

Thanks for reviewing!
Comment 5 Darin Adler 2020-06-19 14:55:44 PDT
Comment on attachment 402329 [details]
Patch

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

> Source/WebKit/NetworkProcess/mac/NetworkProcessMac.mm:92
> +    sandboxParameters.setOverrideSandboxProfilePath(makeString(String([webKitBundle resourcePath]), "/com.apple.WebKit.NetworkProcess.sb"));

You should not need String() around [webKitBundle resourcePath]. If you do, that’s a bug in StringConcatenation.h implementation that we should fix.
Comment 6 Darin Adler 2020-06-19 14:59:51 PDT
(In reply to Per Arne Vollan from comment #4)
> (In reply to Darin Adler from comment #2)
> > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:233
> > > +    if (Class nsApplicationClass = NSClassFromString(@"NSApplication")) {
> > > +        if ([nsApplicationClass respondsToSelector:@selector(_preventDockConnections)])
> > > +            [nsApplicationClass _preventDockConnections];
> > > +    }
> > 
> > I know this code is pre-existing and only being moved, but this does seem
> > risky — if this method is renamed, moved, or removed this code will just
> > silently stop having any effect.
> 
> I could be wrong, but it seems this method is supported.

Why then are we calling it in such an unusual way?
Comment 7 Per Arne Vollan 2020-06-19 15:28:38 PDT
(In reply to Darin Adler from comment #6)
> (In reply to Per Arne Vollan from comment #4)
> > (In reply to Darin Adler from comment #2)
> > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:233
> > > > +    if (Class nsApplicationClass = NSClassFromString(@"NSApplication")) {
> > > > +        if ([nsApplicationClass respondsToSelector:@selector(_preventDockConnections)])
> > > > +            [nsApplicationClass _preventDockConnections];
> > > > +    }
> > > 
> > > I know this code is pre-existing and only being moved, but this does seem
> > > risky — if this method is renamed, moved, or removed this code will just
> > > silently stop having any effect.
> > 
> > I could be wrong, but it seems this method is supported.
> 
> Why then are we calling it in such an unusual way?

That is a good point! I wonder if it is done this way for it to compile on OS's where this method is not supported?
Comment 8 Per Arne Vollan 2020-06-19 15:49:26 PDT
Created attachment 402341 [details]
Patch
Comment 9 Per Arne Vollan 2020-06-19 15:50:40 PDT
(In reply to Per Arne Vollan from comment #7)
> (In reply to Darin Adler from comment #6)
> > (In reply to Per Arne Vollan from comment #4)
> > > (In reply to Darin Adler from comment #2)
> > > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:233
> > > > > +    if (Class nsApplicationClass = NSClassFromString(@"NSApplication")) {
> > > > > +        if ([nsApplicationClass respondsToSelector:@selector(_preventDockConnections)])
> > > > > +            [nsApplicationClass _preventDockConnections];
> > > > > +    }
> > > > 
> > > > I know this code is pre-existing and only being moved, but this does seem
> > > > risky — if this method is renamed, moved, or removed this code will just
> > > > silently stop having any effect.
> > > 
> > > I could be wrong, but it seems this method is supported.
> > 
> > Why then are we calling it in such an unusual way?
> 
> That is a good point! I wonder if it is done this way for it to compile on
> OS's where this method is not supported?

Trying out a patch without using respondsToSelector.

Thanks for reviewing!
Comment 10 EWS 2020-06-19 16:38:01 PDT
Committed r263295: <https://trac.webkit.org/changeset/263295>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402341 [details].
Comment 11 Radar WebKit Bug Importer 2020-06-19 16:38:19 PDT
<rdar://problem/64549175>
Comment 12 Simon Fraser (smfr) 2020-06-24 15:23:25 PDT
This broke setting log channels via -WebCoreLogging
Comment 13 Yusuke Suzuki 2020-06-24 15:30:25 PDT
Re-opened since this is blocked by bug 213577
Comment 14 Per Arne Vollan 2020-06-30 13:22:22 PDT
Created attachment 403234 [details]
Patch
Comment 15 EWS 2020-06-30 15:19:00 PDT
Committed r263773: <https://trac.webkit.org/changeset/263773>

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