Bug 213379 – [macOS] Connections to the preference daemon are established before entering the sandbox

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

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.

[Per Arne Vollan]

Created attachment 402313 [details]
Patch

[Darin Adler]

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.

[Per Arne Vollan]

Created attachment 402329 [details]
Patch

[Per Arne Vollan]

(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!

[Darin Adler]

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.

[Darin Adler]

(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?

[Per Arne Vollan]

(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?

[Per Arne Vollan]

Created attachment 402341 [details]
Patch

[Per Arne Vollan]

(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!

[EWS]

Committed r263295: <https://trac.webkit.org/changeset/263295>

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

[Radar WebKit Bug Importer]

<rdar://problem/64549175>

[Simon Fraser (smfr)]

This broke setting log channels via -WebCoreLogging

[Yusuke Suzuki]

Re-opened since this is blocked by bug 213577

[Per Arne Vollan]

Created attachment 403234 [details]
Patch

[EWS]

Committed r263773: <https://trac.webkit.org/changeset/263773>

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