Bug 228812 – Ease sandboxing restrictions for Mail to allow HEIF image decoding

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Hardware decoding calls methods from sandboxed frameworks so it fails.

[Said Abou-Hallawa]

Created attachment 434967 [details]
Patch

[Said Abou-Hallawa]

<rdar://80967782>

[Said Abou-Hallawa]

Created attachment 434968 [details]
Patch

[Sam Weinig]

Is this really disabling all hardware decoding for WebKit? Or only in the WebContent process and the GPU process will still be allowed? 

If it is more constrained, please update the title and add a little explanation of the reasoning in the ChangeLog.

[Said Abou-Hallawa]

Created attachment 435009 [details]
Patch

[Said Abou-Hallawa]

The test fast/images/heic-as-background-image.html is failing on mac-AS-debug-wk2 because it is the only bot which has macOS Big Sur. Big Sur has more tight rules for HW and in-process rules for image and video decoding. I think this test should be removed from WebKit.

[Said Abou-Hallawa]

Created attachment 435118 [details]
Patch

[Sam Weinig]

Comment on attachment 435118 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:469
> +    if (MacApplication::isAppleMail())

The more resilient way to do this is to create a new entitlement for this capability, check for the entitlement on the host process, and then have Mail add that entitlement.

[Said Abou-Hallawa]

Comment on attachment 435118 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:469
>> +    if (MacApplication::isAppleMail())
> 
> The more resilient way to do this is to create a new entitlement for this capability, check for the entitlement on the host process, and then have Mail add that entitlement.

This seems a good idea.

But can we reverse the order such that Mail will not be broken: land this patch, ask Mail to add that entitlement then replace this condition by checking for this entitlement?

[Sam Weinig]

(In reply to Said Abou-Hallawa from comment #9)
> Comment on attachment 435118 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435118&action=review
> 
> >> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:469
> >> +    if (MacApplication::isAppleMail())
> > 
> > The more resilient way to do this is to create a new entitlement for this capability, check for the entitlement on the host process, and then have Mail add that entitlement.
> 
> This seems a good idea.
> 
> But can we reverse the order such that Mail will not be broken: land this
> patch, ask Mail to add that entitlement then replace this condition by
> checking for this entitlement?

Yes. That is totally fine.

After discussing with Tim, he made the good point that you could just a preference instead of using an entitlement in this case.

[Per Arne Vollan]

Comment on attachment 435118 [details]
Patch

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

R=me.

> Source/WebCore/PAL/ChangeLog:9
> +        Delete unneeded SPIs and their sof-linking.

soft-linking

> Source/WebKit/Shared/WebProcessCreationParameters.h:214
> +    SandboxExtension::HandleArray videoDecoderExtensionHandles;

This can also be just a handle, not an array of handles.

[Said Abou-Hallawa]

Comment on attachment 435118 [details]
Patch

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

>> Source/WebKit/Shared/WebProcessCreationParameters.h:214
>> +    SandboxExtension::HandleArray videoDecoderExtensionHandles;
> 
> This can also be just a handle, not an array of handles.

This is true for IOS but not for macOS since we have add extensions for "com.apple.coremedia.videodecoder" and "com.apple.trustd.agent" for macOS.

[Said Abou-Hallawa]

Created attachment 435194 [details]
Patch

[EWS]

Committed r280789 (240368@main): <https://commits.webkit.org/240368@main>

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

[Per Arne Vollan]

Comment on attachment 435118 [details]
Patch

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

>>> Source/WebKit/Shared/WebProcessCreationParameters.h:214
>>> +    SandboxExtension::HandleArray videoDecoderExtensionHandles;
>> 
>> This can also be just a handle, not an array of handles.
> 
> This is true for IOS but not for macOS since we have add extensions for "com.apple.coremedia.videodecoder" and "com.apple.trustd.agent" for macOS.

Is access to "com.apple.trustd.agent" required when you allow access to "com.apple.coremedia.videodecoder"? I think it would be good to confirm that.

[Said Abou-Hallawa]

Comment on attachment 435118 [details]
Patch

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

>>>> Source/WebKit/Shared/WebProcessCreationParameters.h:214
>>>> +    SandboxExtension::HandleArray videoDecoderExtensionHandles;
>>> 
>>> This can also be just a handle, not an array of handles.
>> 
>> This is true for IOS but not for macOS since we have add extensions for "com.apple.coremedia.videodecoder" and "com.apple.trustd.agent" for macOS.
> 
> Is access to "com.apple.trustd.agent" required when you allow access to "com.apple.coremedia.videodecoder"? I think it would be good to confirm that.

Yes. I found this through "sbutil log stream" when the HEIF was not displayed correctly even after allowing access to "com.apple.coremedia.videodecoder".