Bug 170700 - Restrict WebKit image formats to a known whitelist
Summary: Restrict WebKit image formats to a known whitelist
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: 170697 171042 171077
Blocks:
  Show dependency treegraph
 
Reported: 2017-04-10 15:24 PDT by Said Abou-Hallawa
Modified: 2017-04-26 13:35 PDT (History)
10 users (show)

See Also:


Attachments
Patch for EWS (65.71 KB, patch)
2017-04-21 22:16 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch for review (3.15 KB, patch)
2017-04-21 22:18 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (686.97 KB, application/zip)
2017-04-21 23:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (667.58 KB, application/zip)
2017-04-21 23:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (707.51 KB, application/zip)
2017-04-21 23:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (9.58 MB, application/zip)
2017-04-21 23:24 PDT, Build Bot
no flags Details
Patch for EWS (67.40 KB, patch)
2017-04-22 01:04 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch for review (3.42 KB, patch)
2017-04-23 16:32 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (17.68 KB, patch)
2017-04-25 12:02 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (29.48 KB, patch)
2017-04-25 14:04 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (29.48 KB, patch)
2017-04-25 14:05 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (16.35 KB, patch)
2017-04-26 12:10 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 2017-04-10 15:24:11 PDT
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.
Comment 1 Radar WebKit Bug Importer 2017-04-10 15:26:02 PDT
<rdar://problem/31543425>
Comment 2 Said Abou-Hallawa 2017-04-21 22:16:46 PDT
Created attachment 307883 [details]
Patch for EWS
Comment 3 Said Abou-Hallawa 2017-04-21 22:18:36 PDT
Created attachment 307885 [details]
Patch for review
Comment 4 Build Bot 2017-04-21 23:10:49 PDT
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.
Comment 5 Build Bot 2017-04-21 23:10:51 PDT
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
Comment 6 Build Bot 2017-04-21 23:12:49 PDT
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.
Comment 7 Build Bot 2017-04-21 23:12:50 PDT
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
Comment 8 Build Bot 2017-04-21 23:16:13 PDT
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.
Comment 9 Build Bot 2017-04-21 23:16:14 PDT
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
Comment 10 Build Bot 2017-04-21 23:23:59 PDT
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.
Comment 11 Build Bot 2017-04-21 23:24:01 PDT
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
Comment 12 Said Abou-Hallawa 2017-04-22 01:04:03 PDT
Created attachment 307900 [details]
Patch for EWS
Comment 13 Ryosuke Niwa 2017-04-22 01:11:48 PDT
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
> +2017-04-21  Said Abou-Hallawa  <sabouhallawa@apple.com>

You have three change log entries in your patch.
Comment 14 Ryosuke Niwa 2017-04-22 01:13:38 PDT
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.
Comment 15 Said Abou-Hallawa 2017-04-23 16:29:35 PDT
(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".
Comment 16 Said Abou-Hallawa 2017-04-23 16:32:43 PDT
Created attachment 307945 [details]
Patch for review
Comment 17 Said Abou-Hallawa 2017-04-23 17:21:30 PDT
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
>> +2017-04-21  Said Abou-Hallawa  <sabouhallawa@apple.com>
> 
> 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.
Comment 18 Tim Horton 2017-04-24 16:04:59 PDT
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?
Comment 19 Said Abou-Hallawa 2017-04-25 12:02:42 PDT
Created attachment 308125 [details]
Patch
Comment 20 Said Abou-Hallawa 2017-04-25 14:04:10 PDT
Created attachment 308140 [details]
Patch
Comment 21 Said Abou-Hallawa 2017-04-25 14:05:41 PDT
Created attachment 308141 [details]
Patch
Comment 22 WebKit Commit Bot 2017-04-25 15:24:00 PDT
Comment on attachment 308141 [details]
Patch

Clearing flags on attachment: 308141

Committed r215767: <http://trac.webkit.org/changeset/215767>
Comment 23 WebKit Commit Bot 2017-04-25 15:24:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Ryan Haddad 2017-04-26 09:04:41 PDT
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.
Comment 25 Ryan Haddad 2017-04-26 09:13:07 PDT
Reverted r215767 for reason:

The LayoutTest for this change is a flaky failure.

Committed r215803: <http://trac.webkit.org/changeset/215803>
Comment 26 Said Abou-Hallawa 2017-04-26 09:34:29 PDT
(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.
Comment 27 Said Abou-Hallawa 2017-04-26 12:10:33 PDT
Created attachment 308272 [details]
Patch
Comment 28 WebKit Commit Bot 2017-04-26 13:35:11 PDT
Comment on attachment 308272 [details]
Patch

Clearing flags on attachment: 308272

Committed r215829: <http://trac.webkit.org/changeset/215829>
Comment 29 WebKit Commit Bot 2017-04-26 13:35:13 PDT
All reviewed patches have been landed.  Closing bug.