The commit <https://trac.webkit.org/changeset/249649> introduced a MiniBrowser regression on macOS where MiniBrowser is not able to open local files. The change set r249649 fixed a problem where the WebContent process PID was not ready to be used when creating a sandbox extension. This happened in the cases where the WebContent process had not finished launching when the load started. The WebContent process is also creating sandbox extensions for the Networking process for the files being loaded, and also needs to be passing the PID of the Networking process when creating these.
Created attachment 378797 [details] Patch
Created attachment 378798 [details] Patch
As I mentioned in the previous bug, this really should not be using a pid, as they are prone to re-use attacks, and instead should be using audit_token_t. Any reason this can't be unit tested?
Comment on attachment 378798 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378798&action=review r=me. Let's get this integrated since this could impact clients on our new release. > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:74 > +#endif Is MiniBrowser the only client for this? Or might you be fixing bugs in other applications we just haven't seen yet?
(In reply to Sam Weinig from comment #3) > As I mentioned in the previous bug, this really should not be using a pid, > as they are prone to re-use attacks, and instead should be using > audit_token_t. > > Any reason this can't be unit tested? Per Arne: This is a great point. After you land this regression fix, can you spin up a task to switch the pid-based stuff over to audit tokens instead?
(In reply to Sam Weinig from comment #3) > As I mentioned in the previous bug, this really should not be using a pid, > as they are prone to re-use attacks, and instead should be using > audit_token_t. > > Any reason this can't be unit tested? That's a good point. I will file a bug to start using audit tokens instead of PIDs, and add an API test. Thanks for reviewing!
(In reply to Brent Fulgham from comment #4) > Comment on attachment 378798 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378798&action=review > > r=me. Let's get this integrated since this could impact clients on our new > release. > > > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:74 > > +#endif > > Is MiniBrowser the only client for this? Or might you be fixing bugs in > other applications we just haven't seen yet? As you suggest, I think this bug can affect other applications in addition to MiniBrowser. Thanks for reviewing!
(In reply to Brent Fulgham from comment #5) > (In reply to Sam Weinig from comment #3) > > As I mentioned in the previous bug, this really should not be using a pid, > > as they are prone to re-use attacks, and instead should be using > > audit_token_t. > > > > Any reason this can't be unit tested? > > Per Arne: This is a great point. After you land this regression fix, can you > spin up a task to switch the pid-based stuff over to audit tokens instead? Filed https://bugs.webkit.org/show_bug.cgi?id=201828.
Wait, why the r+ without a test? Especially since this was a regression, this needs to be tested.
Comment on attachment 378798 [details] Patch Clearing flags on attachment: 378798 Committed r249910: <https://trac.webkit.org/changeset/249910>
All reviewed patches have been landed. Closing bug.
<rdar://problem/55409379>
Reverted r249910 for reason: Caused layout test failures and timeouts on Catalina Committed r249942: <https://trac.webkit.org/changeset/249942>
(In reply to Ryan Haddad from comment #13) > Reverted r249910 for reason: > > Caused layout test failures and timeouts on Catalina > > Committed r249942: <https://trac.webkit.org/changeset/249942> Details in radar.
Created attachment 379034 [details] Patch
(In reply to Per Arne Vollan from comment #15) > Created attachment 379034 [details] > Patch Updated patch to add API test and fix layout test failures. Rewriting this to use audit tokens instead of PIDs will come in a follow up patch.
Created attachment 379074 [details] Patch
Created attachment 380060 [details] Patch
Created attachment 380061 [details] Patch
Created attachment 380062 [details] Patch
Comment on attachment 380062 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380062&action=review > Source/WebKit/ChangeLog:3 > + REGRESSION(249649): Unable to open local files in MiniBrowser on macOS Is this really a fix for the regression? I thought this was about adopting Audit Tokens? > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:69 > +#if HAVE(SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_PID) Does this use PID if we can't use Audit Tokens? If not, I think we should rename the macro to SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_AUDIT_TOKEN > Source/WebKit/Shared/SandboxExtension.h:52 > + ReadByProcess Looks like we don't use PID -- so I think we do need to rename the macro. > Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:46 > + std::unique_ptr<SandboxExtensionImpl> impl { new SandboxExtensionImpl(path, type, process) }; I suggest we call these parameters "processToken" or "auditToken". We use process in a lot of places to represent our own concept of a process abstraction.
Comment on attachment 380062 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380062&action=review > Source/WebKit/Platform/IPC/ArgumentCoders.cpp:191 > +#if OS(DARWIN) This should be a more specific conditional. e.g. HAVE(AUDIT_TOKEN)
(In reply to Brent Fulgham from comment #21) > Comment on attachment 380062 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380062&action=review > > > Source/WebKit/ChangeLog:3 > > + REGRESSION(249649): Unable to open local files in MiniBrowser on macOS > > Is this really a fix for the regression? I thought this was about adopting > Audit Tokens? > Originally, this was just about fixing the regression using PIDs, but now it is both a regression fix, and a fix to adopt audit tokens. > > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:69 > > +#if HAVE(SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_PID) > > Does this use PID if we can't use Audit Tokens? If not, I think we should > rename the macro to SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_AUDIT_TOKEN > It does not use PIDs in case we can't use Audit Token. I will use SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_AUDIT_TOKEN instead. > > Source/WebKit/Shared/SandboxExtension.h:52 > > + ReadByProcess > > Looks like we don't use PID -- so I think we do need to rename the macro. > > > Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:46 > > + std::unique_ptr<SandboxExtensionImpl> impl { new SandboxExtensionImpl(path, type, process) }; > > I suggest we call these parameters "processToken" or "auditToken". We use > process in a lot of places to represent our own concept of a process > abstraction. Will do. Thanks for reviewing!
(In reply to Sam Weinig from comment #22) > Comment on attachment 380062 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380062&action=review > > > Source/WebKit/Platform/IPC/ArgumentCoders.cpp:191 > > +#if OS(DARWIN) > > This should be a more specific conditional. e.g. HAVE(AUDIT_TOKEN) Will fix. Thanks for reviewing!
Comment on attachment 380062 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380062&action=review > Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:126 > + SandboxExtensionImpl(const char* path, SandboxExtension::Type type, Optional<Variant<pid_t, audit_token_t>> process) Why is this a variant? A pid should never be used.
Created attachment 380069 [details] Patch
(In reply to Sam Weinig from comment #25) > Comment on attachment 380062 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380062&action=review > > > Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:126 > > + SandboxExtensionImpl(const char* path, SandboxExtension::Type type, Optional<Variant<pid_t, audit_token_t>> process) > > Why is this a variant? A pid should never be used. This patch only partially adopt audit tokens. When the UI process is creating extensions for the WebContent process, PIDs are still used, but I will rewrite this to use audit tokens in a follow-up patch coming soon. We can then remove the variant.
Created attachment 380120 [details] Patch
Created attachment 380128 [details] Patch
Comment on attachment 380128 [details] Patch This seems reasonable. Can you also file a bug to convert the other PID-based code to use Audit Tokens?
Created attachment 380152 [details] Patch
(In reply to Brent Fulgham from comment #30) > Comment on attachment 380128 [details] > Patch > > This seems reasonable. Can you also file a bug to convert the other > PID-based code to use Audit Tokens? Filed https://bugs.webkit.org/show_bug.cgi?id=202540. Thanks for reviewing!
Comment on attachment 380152 [details] Patch Rejecting attachment 380152 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 380152, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit NOBODY Brent Fulgham found in /Volumes/Data/EWS/WebKit/Source/WebKit/ChangeLog does not appear to be a valid reviewer according to contributors.json. /Volumes/Data/EWS/WebKit/Source/WebKit/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: https://webkit-queues.webkit.org/results/13091824
Created attachment 380153 [details] Patch
Comment on attachment 380153 [details] Patch Clearing flags on attachment: 380153 Committed r250673: <https://trac.webkit.org/changeset/250673>