Bug 228812 - Ease sandboxing restrictions for Mail to allow HEIF image decoding
Summary: Ease sandboxing restrictions for Mail to allow HEIF image decoding
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-08-05 00:18 PDT by Said Abou-Hallawa
Modified: 2022-05-23 17:37 PDT (History)
12 users (show)

See Also:


Attachments
Patch (13.25 KB, patch)
2021-08-05 00:22 PDT, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (13.27 KB, patch)
2021-08-05 00:48 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (15.02 KB, patch)
2021-08-05 11:00 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (28.56 KB, patch)
2021-08-06 23:17 PDT, Said Abou-Hallawa
pvollan: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (28.68 KB, patch)
2021-08-09 10:42 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2021-08-05 00:18:15 PDT
Hardware decoding calls methods from sandboxed frameworks so it fails.
Comment 1 Said Abou-Hallawa 2021-08-05 00:22:45 PDT
Created attachment 434967 [details]
Patch
Comment 2 Said Abou-Hallawa 2021-08-05 00:23:51 PDT
<rdar://80967782>
Comment 3 Said Abou-Hallawa 2021-08-05 00:48:45 PDT
Created attachment 434968 [details]
Patch
Comment 4 Sam Weinig 2021-08-05 07:49:00 PDT
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.
Comment 5 Said Abou-Hallawa 2021-08-05 11:00:33 PDT
Created attachment 435009 [details]
Patch
Comment 6 Said Abou-Hallawa 2021-08-05 13:21:38 PDT
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.
Comment 7 Said Abou-Hallawa 2021-08-06 23:17:51 PDT
Created attachment 435118 [details]
Patch
Comment 8 Sam Weinig 2021-08-07 11:26:36 PDT
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.
Comment 9 Said Abou-Hallawa 2021-08-08 16:38:28 PDT
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?
Comment 10 Sam Weinig 2021-08-09 08:41:53 PDT
(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.
Comment 11 Per Arne Vollan 2021-08-09 10:35:51 PDT
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.
Comment 12 Said Abou-Hallawa 2021-08-09 10:38:21 PDT
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.
Comment 13 Said Abou-Hallawa 2021-08-09 10:42:27 PDT
Created attachment 435194 [details]
Patch
Comment 14 EWS 2021-08-09 12:06:48 PDT
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].
Comment 15 Per Arne Vollan 2021-08-09 12:10:05 PDT
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.
Comment 16 Said Abou-Hallawa 2021-08-09 12:14:15 PDT
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".