Bug 201798 – REGRESSION(249649): Unable to open local files in MiniBrowser on macOS

• Component: WebKit Misc.
• Status: REOPENED

[Per Arne Vollan]

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.

[Per Arne Vollan]

Created attachment 378797 [details]
Patch

[Per Arne Vollan]

Created attachment 378798 [details]
Patch

[Sam Weinig]

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?

[Brent Fulgham]

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?

[Brent Fulgham]

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

[Per Arne Vollan]

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

[Per Arne Vollan]

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

[Per Arne Vollan]

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

[Sam Weinig]

Wait, why the r+ without a test? Especially since this was a regression, this needs to be tested.

[WebKit Commit Bot]

Comment on attachment 378798 [details]
Patch

Clearing flags on attachment: 378798

Committed r249910: <https://trac.webkit.org/changeset/249910>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55409379>

[Ryan Haddad]

Reverted r249910 for reason:

Caused layout test failures and timeouts on Catalina

Committed r249942: <https://trac.webkit.org/changeset/249942>

[Ryan Haddad]

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

[Per Arne Vollan]

Created attachment 379034 [details]
Patch

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 379074 [details]
Patch

[Per Arne Vollan]

Created attachment 380060 [details]
Patch

[Per Arne Vollan]

Created attachment 380061 [details]
Patch

[Per Arne Vollan]

Created attachment 380062 [details]
Patch

[Brent Fulgham]

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.

[Sam Weinig]

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)

[Per Arne Vollan]

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

[Per Arne Vollan]

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

[Sam Weinig]

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.

[Per Arne Vollan]

Created attachment 380069 [details]
Patch

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 380120 [details]
Patch

[Per Arne Vollan]

Created attachment 380128 [details]
Patch

[Brent Fulgham]

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?

[Per Arne Vollan]

Created attachment 380152 [details]
Patch

[Per Arne Vollan]

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

[WebKit Commit Bot]

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

[Per Arne Vollan]

Created attachment 380153 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 380153 [details]
Patch

Clearing flags on attachment: 380153

Committed r250673: <https://trac.webkit.org/changeset/250673>