Bug 211103

<rdar://problem/62533632>

Created attachment 397866 [details]
Patch

Comment on attachment 397866 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397866&action=review

I think this is a great idea. R=me.

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:296
> +Optional<SandboxExtension::HandleArray> SandboxExtension::createHandlesForFiles(const String& logLabel, const Vector<String>& paths, Type type)

Since it seems this method is only called with the ReadOnly type, maybe the type argument can be removed?

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:299
> +    if (paths.isEmpty())
> +        return WTF::nullopt;

If the method did not return an Optional, but only a HandleArray, this if statement could be removed.

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:304
> +    for (size_t i = 0; i < paths.size(); ++i) {

Can this be a modern for loop?

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:306
> +        if (!SandboxExtension::createHandle(paths[i], type, handleArray[i])) {
> +            // This can legitimately fail if a directory containing the file is deleted after the file was chosen.

Perhaps you could add an ASSERT_NOT_REACHED() here, so we can catch it in the debugger?

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:379
> +Optional<SandboxExtension::HandleArray> SandboxExtension::createHandlesForMachLookup(const Vector<String>& services, Optional<audit_token_t> auditToken, OptionSet<Flags> flags)

Could this simply return a HandleArray?

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:387
> +    for (size_t i = 0; i < services.size(); ++i) {

Can a modern for loop be used?

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:389
> +        if (!SandboxExtension::createHandleForMachLookup(services[i], auditToken, handleArray[i], flags))
> +            return WTF::nullopt;

I think it would be good to keep going, even if this call should fail for a service. Also, I think it would be good to add an ASSERT_NOT_REACHED() in the case it fails.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:452
> +        if (auto mediaExtensionHandles = SandboxExtension::createHandlesForMachLookup(mediaRelatedMachServices(), WTF::nullopt))
> +            parameters.mediaExtensionHandles = WTFMove(*mediaExtensionHandles);

If the function simply returned a HandleArray, this could be just one line.

Created attachment 398006 [details]
Patch

Comment on attachment 397866 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397866&action=review

>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:296
>> +Optional<SandboxExtension::HandleArray> SandboxExtension::createHandlesForFiles(const String& logLabel, const Vector<String>& paths, Type type)
> 
> Since it seems this method is only called with the ReadOnly type, maybe the type argument can be removed?

Good point. I'll try that.

>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:299
>> +        return WTF::nullopt;
> 
> If the method did not return an Optional, but only a HandleArray, this if statement could be removed.

Will do!

>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:304
>> +    for (size_t i = 0; i < paths.size(); ++i) {
> 
> Can this be a modern for loop?

Sure -- that way I can only fill the handle array with paths that succeed, rather than null handles. I'm not sure if that ever happens, but it doesn't seem like it's valid to consume them.

>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:379
>> +Optional<SandboxExtension::HandleArray> SandboxExtension::createHandlesForMachLookup(const Vector<String>& services, Optional<audit_token_t> auditToken, OptionSet<Flags> flags)
> 
> Could this simply return a HandleArray?

Sure.

>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:387
>> +    for (size_t i = 0; i < services.size(); ++i) {
> 
> Can a modern for loop be used?

Will do.

>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:389
>> +            return WTF::nullopt;
> 
> I think it would be good to keep going, even if this call should fail for a service. Also, I think it would be good to add an ASSERT_NOT_REACHED() in the case it fails.

Will do!

>> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:452
>> +            parameters.mediaExtensionHandles = WTFMove(*mediaExtensionHandles);
> 
> If the function simply returned a HandleArray, this could be just one line.

Fixed!

Created attachment 398009 [details]
Patch

Created attachment 398014 [details]
Patch

Comment on attachment 398014 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398014&action=review

R=me.

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:296
> +static SandboxExtension::HandleArray createHandlesForInput(const Vector<String>& services, Function<bool(const String&, SandboxExtension::Handle& handle)>&& createFunction)

The name of the first parameter does not seem to describe all variations of possible input types, since it can be services, paths, and IOKit classes. Perhaps this function could be renamed 'createHandlesForResources', and the first parameter could be renamed 'resources', or something similar?

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:300
> +    handleArray.allocate(services.size());

Is allocating with size 0 gracefully handled by HandleArray? On a related note, would it be better to typedef HandleArray to Vector<Handle>? That's outside the scope of this patch, though. In that case, allocating up front would not be needed, we could just append in the loop.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:401
> +        parameters.dynamicIOKitExtensionHandles = createHandlesForIOKitClassExtensions(agxCompilerClasses(), WTF::nullopt);

I think this should be SandboxExtension::createHandlesForIOKitClassExtensions(agxCompilerClasses(), WTF::nullopt);

Comment on attachment 398014 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398014&action=review

>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:296
>> +static SandboxExtension::HandleArray createHandlesForInput(const Vector<String>& services, Function<bool(const String&, SandboxExtension::Handle& handle)>&& createFunction)
> 
> The name of the first parameter does not seem to describe all variations of possible input types, since it can be services, paths, and IOKit classes. Perhaps this function could be renamed 'createHandlesForResources', and the first parameter could be renamed 'resources', or something similar?

Good idea.

>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:300
>> +    handleArray.allocate(services.size());
> 
> Is allocating with size 0 gracefully handled by HandleArray? On a related note, would it be better to typedef HandleArray to Vector<Handle>? That's outside the scope of this patch, though. In that case, allocating up front would not be needed, we could just append in the loop.

Maybe. I'll take a look at that in a future patch.

>> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:401
>> +        parameters.dynamicIOKitExtensionHandles = createHandlesForIOKitClassExtensions(agxCompilerClasses(), WTF::nullopt);
> 
> I think this should be SandboxExtension::createHandlesForIOKitClassExtensions(agxCompilerClasses(), WTF::nullopt);

Whoops!

Created attachment 398022 [details]
Patch for landing

Committed r260932: <https://trac.webkit.org/changeset/260932>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398022 [details].

Comment on attachment 398022 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=398022&action=review

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:313
> +SandboxExtension::HandleArray SandboxExtension::createReadOnlyHandlesForFiles(const String& logLabel, const Vector<String>& paths)

You could also pass an error handler function instead of the log label String, which would be invoked for each error. The caller would then be responsible for customizing the logging.

It looks like the changes in https://trac.webkit.org/changeset/260932/webkit

broke TestWebKitAPI.WebKit.UploadDirectory on Mac Debug with an Assert

history:
https://results.webkit.org/?suite=api-tests&suite=api-tests&suite=api-tests&suite=api-tests&test=TestWebKitAPI.WebKit.UploadDirectory&test=TestWebKitAPI.URLSchemeHandler.DisableCORSCredentials&test=TestWebKitAPI.MultipleClientCertificateConnections.NonPersistentDataStore&test=TestWebKitAPI.Fullscreen.Delegate

Crash Log:
Crashed

    TestWebKitAPI.WebKit.UploadDirectory
        ASSERTION FAILED: ok
        /Volumes/Data/slave/catalina-debug/build/Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm(506) : static bool WebKit::SandboxExtension::consumePermanently(const WebKit::SandboxExtension::HandleArray &)
        1   0x108267609 WTFCrash
        2   0x11147aeeb WTFCrashWithInfo(int, char const*, char const*, int)
        3   0x111c649ac WebKit::SandboxExtension::consumePermanently(WebKit::SandboxExtension::HandleArray const&)
        4   0x11171c71b IPC::FormDataReference::decode(IPC::Decoder&)
        5   0x11171c49c WTF::Optional<IPC::FormDataReference> IPC::ArgumentCoder<IPC::FormDataReference>::decode<IPC::FormDataReference, (void*)0>(IPC::Decoder&)

Better history link: https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.WebKit.UploadDirectory

It looks like this also broke these two tests:
http/tests/misc/form-submit-file-cross-site-redirect.html
http/tests/misc/form-submit-file-cross-site.html

History:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=http%2Ftests%2Fmisc%2Fform-submit-file-cross-site.html&test=http%2Ftests%2Fmisc%2Fform-submit-file-cross-site-redirect.html

Results:
https://build.webkit.org/results/Apple-Catalina-Debug-WK2-Tests/r260952%20(3943)/results.html

(In reply to Truitt Savell from comment #14)
> Better history link:
> https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.WebKit.
> UploadDirectory

Interesting. This test is triggering a new assert I added to detect when we are unable to consume a sandbox extension.

In this case, an extension is being sent from the WebContent process (which cannot issue extensions) to the UIProcess, and the UIProcess is correctly reporting that it cannot consume this extension.

I suspect the code resulting in a sandbox extension being passed from the WCP to the UIProcess is wrong, so this may have uncovered a real bug.

Note: We should consider disabling this API test until we fix the underlying bug. We should not roll this patch out.

(In reply to Truitt Savell from comment #15)
> It looks like this also broke these two tests:
> http/tests/misc/form-submit-file-cross-site-redirect.html
> http/tests/misc/form-submit-file-cross-site.html
> 
> History:
> https://results.webkit.org/?suite=layout-tests&suite=layout-
> tests&test=http%2Ftests%2Fmisc%2Fform-submit-file-cross-site.
> html&test=http%2Ftests%2Fmisc%2Fform-submit-file-cross-site-redirect.html
> 
> Results:
> https://build.webkit.org/results/Apple-Catalina-Debug-WK2-Tests/
> r260952%20(3943)/results.html

These two crashes are the same issue. WCP is sending a sandbox extension to the UIProcess.

There is never a case where the WCP has access to something the UIProcess does not. Access always flows from the UIProcess to its helper processes.

New bug to track this regression: https://bugs.webkit.org/show_bug.cgi?id=211253