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+
Patch (11.68 KB, patch)
2020-06-19 14:52 PDT, Per Arne Vollan
no flags
Patch (11.50 KB, patch)
2020-06-19 15:49 PDT, Per Arne Vollan
no flags
Patch (10.94 KB, patch)
2020-06-30 13:22 PDT, Per Arne Vollan
no flags
Per Arne Vollan
Comment 1 2020-06-19 13:11:49 PDT
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
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
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
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
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.