Summary: | REGRESSION(249649): Unable to open local files in MiniBrowser on macOS | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||||||||||||||||||
Component: | WebKit Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||||||||||||||||||||
Status: | REOPENED --- | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | benjamin, bfulgham, cdumez, cmarcelo, commit-queue, dbates, ews-watchlist, rmorisset, ryanhaddad, sam, webkit-bug-importer | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=202575 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2019-09-14 08:00:33 PDT
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. 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> |