WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
213379
[macOS] Connections to the preference daemon are established before entering the sandbox
https://bugs.webkit.org/show_bug.cgi?id=213379
Summary
[macOS] Connections to the preference daemon are established before entering ...
Per Arne Vollan
Reported
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.
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
View All
Add attachment
proposed patch, testcase, etc.
Per Arne Vollan
Comment 1
2020-06-19 13:11:49 PDT
Created
attachment 402313
[details]
Patch
Darin Adler
Comment 2
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.
Per Arne Vollan
Comment 3
2020-06-19 14:52:43 PDT
Created
attachment 402329
[details]
Patch
Per Arne Vollan
Comment 4
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!
Darin Adler
Comment 5
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.
Darin Adler
Comment 6
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?
Per Arne Vollan
Comment 7
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?
Per Arne Vollan
Comment 8
2020-06-19 15:49:26 PDT
Created
attachment 402341
[details]
Patch
Per Arne Vollan
Comment 9
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!
EWS
Comment 10
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]
.
Radar WebKit Bug Importer
Comment 11
2020-06-19 16:38:19 PDT
<
rdar://problem/64549175
>
Simon Fraser (smfr)
Comment 12
2020-06-24 15:23:25 PDT
This broke setting log channels via -WebCoreLogging
Yusuke Suzuki
Comment 13
2020-06-24 15:30:25 PDT
Re-opened since this is blocked by
bug 213577
Per Arne Vollan
Comment 14
2020-06-30 13:22:22 PDT
Created
attachment 403234
[details]
Patch
EWS
Comment 15
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]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug