Bug 170700 – Restrict WebKit image formats to a known whitelist

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

Instead of allowing any image format to be loaded, decoded and drawn, WebKit is going to restrict its image formats to a known whitelist. We are going to start by using the list of image formats which CGImageSourceCopyTypeIdentifiers() return.

[Radar WebKit Bug Importer]

<rdar://problem/31543425>

[Said Abou-Hallawa]

Created attachment 307883 [details]
Patch for EWS

[Said Abou-Hallawa]

Created attachment 307885 [details]
Patch for review

[Build Bot]

Comment on attachment 307883 [details]
Patch for EWS

Attachment 307883 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3582463

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307890 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307883 [details]
Patch for EWS

Attachment 307883 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3582467

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307891 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307883 [details]
Patch for EWS

Attachment 307883 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3582454

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307892 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307883 [details]
Patch for EWS

Attachment 307883 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3582377

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307894 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 307900 [details]
Patch for EWS

[Ryosuke Niwa]

Comment on attachment 307900 [details]
Patch for EWS

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

> Source/WebCore/ChangeLog:16
> [phone]  Said Abou-Hallawa  <[e-mail]>

You have three change log entries in your patch.

[Ryosuke Niwa]

Comment on attachment 307900 [details]
Patch for EWS

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

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:49
> +    // CG at least supports the following standard image types:
> +    static NeverDestroyed<HashSet<String>> s_allowedImageUTIs = std::initializer_list<String> {

We should make sure we're adverting and only advertising these image types in HTTP's accept header.

[Said Abou-Hallawa]

(In reply to Ryosuke Niwa from comment #14)
> Comment on attachment 307900 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307900&action=review
> 
> > Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:49
> > +    // CG at least supports the following standard image types:
> > +    static NeverDestroyed<HashSet<String>> s_allowedImageUTIs = std::initializer_list<String> {
> 
> We should make sure we're adverting and only advertising these image types
> in HTTP's accept header.

In the function acceptHeaderValueFromType() and for the case CachedResource::Type::ImageResource, we return: ASCIILiteral("image/png,image/svg+xml,image/*;q=0.8,*/*;q=0.5"). Including "image/*" in the accept header will match image/png, image/svg, image/gif and any other image types. 

I can change this string to include the MIME types of the supported image formats only. The only problem is UTTypeCopyPreferredTagWithClass() does not return a valid MIME type for: "com.microsoft.cur" and "public.mpo-image".

[Said Abou-Hallawa]

Created attachment 307945 [details]
Patch for review

[Said Abou-Hallawa]

Comment on attachment 307900 [details]
Patch for EWS

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

>> Source/WebCore/ChangeLog:16
>> [phone]  Said Abou-Hallawa  <[e-mail]>
> 
> You have three change log entries in your patch.

This patch is not for review. It is for EWS only. It includes the patches of the bugs

https://bugs.webkit.org/show_bug.cgi?id=171042
https://bugs.webkit.org/show_bug.cgi?id=171077
https://bugs.webkit.org/show_bug.cgi?id=170700

I combined all of them to be able to verify the EWS will pass if the first two patches get landed. I attached a smaller patch for review which is the difference between this patch and the combination of the first two patches.

[Tim Horton]

Comment on attachment 307945 [details]
Patch for review

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

> Source/WebCore/loader/cache/CachedImage.cpp:394
> +EncodedDataStatus CachedImage::setImageIncrementalDataBuffer(SharedBuffer& data, bool allDataReceived)

it's not incremental if allDataReceived is true. Maybe a different name?

[Said Abou-Hallawa]

Created attachment 308125 [details]
Patch

[Said Abou-Hallawa]

Created attachment 308140 [details]
Patch

[Said Abou-Hallawa]

Created attachment 308141 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 308141 [details]
Patch

Clearing flags on attachment: 308141

Committed r215767: <http://trac.webkit.org/changeset/215767>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

The LayoutTest for this change is a flaky failure on the bots:

https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK1%20(Tests)/r215798%20(793)/results.html

The test also crashes when run under GuardMalloc / ASan.

[Ryan Haddad]

Reverted r215767 for reason:

The LayoutTest for this change is a flaky failure.

Committed r215803: <http://trac.webkit.org/changeset/215803>

[Said Abou-Hallawa]

(In reply to Ryan Haddad from comment #25)
> Reverted r215767 for reason:
> 
> The LayoutTest for this change is a flaky failure.
> 
> Committed r215803: <http://trac.webkit.org/changeset/215803>

The test has four images. The order of receiving onload or onerror for these images might be different on EWS from the order I got on my machine. Sorting the output messages or making setting their src attributes sequential should fix the flakness issue.

[Said Abou-Hallawa]

Created attachment 308272 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 308272 [details]
Patch

Clearing flags on attachment: 308272

Committed r215829: <http://trac.webkit.org/changeset/215829>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.