Bug 174692 - [CG] An image should not invoke many system calls before confirming its format is supported
Summary: [CG] An image should not invoke many system calls before confirming its forma...
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: 2017-07-20 15:03 PDT by Said Abou-Hallawa
Modified: 2017-07-21 13:38 PDT (History)
9 users (show)

See Also:


Attachments
Patch (5.18 KB, patch)
2017-07-20 15:04 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (5.19 KB, patch)
2017-07-20 17:26 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-07-20 15:03:29 PDT
WebKit has its whitelist of image formats. Loading images with any format out of this whitelist will be cancelled. To know the image type, WebKit creates an ImageSource and sends it the encoded data when it's received. It calls CGImageSourceGetStatus() to know the status of the encoded data to know whether the type is available or not. When the image is available, WebKit calls CGImageSourceGetType() to decide whether to continue loading this image or cancel loading it and marking it a broken image.

We should be careful when invoking system calls before confirming the image type is available and it is one of the whitelist formats. Otherwise we will be invoking the parsers of the unsupported formats. The fix should be the following:

After setting new data to the ImageSource:

-- Call CGImageSourceGetType() — if that returns nil, return Unknown.
-- if CGImageSourceGetType()  returns non-nil, then use can call CGImageSourceGetStatus()
-- If CGImageSourceGetStatus() returns kCGImageStatusIncomplete or kCGImageStatusComplete, we check isAllowedImageUTI(CGImageSourceGetType()) - if it returns false we return Error.
-- If isAllowedImageUTI(CGImageSourceGetType()) returns true, we can call CGImageSourceCopyPropertiesAtIndex() and other system functions.
Comment 1 Said Abou-Hallawa 2017-07-20 15:04:22 PDT
Created attachment 316029 [details]
Patch
Comment 2 Said Abou-Hallawa 2017-07-20 15:05:11 PDT
<rdar://problem/33388544>
Comment 3 Jeremy Jones 2017-07-20 17:13:39 PDT
Comment on attachment 316029 [details]
Patch

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

Is there any chance this UTI check is too strict and will prevent loading of images that we don't have a (correct) UTI for but are otherwise displayable? I.e. mime-type is incorrect and it is actually a valid image type?

Is it possible to test this?

> Source/WebCore/ChangeLog:26
> +        before knowing the uti of the image. When knowing it, we call CGImageSourceGetStatus()

nit: UIT

> Source/WebCore/ChangeLog:29
> +        false, return Error which will make the CachedImage cancels loading the 

nit: cancel loading
Comment 4 Said Abou-Hallawa 2017-07-20 17:26:12 PDT
Created attachment 316046 [details]
Patch
Comment 5 Said Abou-Hallawa 2017-07-20 17:37:36 PDT
(In reply to Jeremy Jones from comment #3)
> Comment on attachment 316029 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316029&action=review
> 
> Is there any chance this UTI check is too strict and will prevent loading of
> images that we don't have a (correct) UTI for but are otherwise displayable?
> I.e. mime-type is incorrect and it is actually a valid image type?
> 

Actually the requirement is to load and display only the images whose formats are in the WebKit whitelist image formats. The system can display more formats than this whitelist. But these formats are not standard and they may cause security vulnerabilities. So yes we need our check for the image UTI be strict.

> Is it possible to test this?
> 

This work is covered by LayoutTests/fast/images/image-formats-support.html. This patch is about making less system calls in case the image format is not supported. I do not think I can make a test for that.
Comment 6 Jeremy Jones 2017-07-20 17:47:25 PDT
(In reply to Said Abou-Hallawa from comment #5)
> (In reply to Jeremy Jones from comment #3)
> > Comment on attachment 316029 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=316029&action=review
> > 
> > Is there any chance this UTI check is too strict and will prevent loading of
> > images that we don't have a (correct) UTI for but are otherwise displayable?
> > I.e. mime-type is incorrect and it is actually a valid image type?
> > 
> 
> Actually the requirement is to load and display only the images whose
> formats are in the WebKit whitelist image formats. The system can display
> more formats than this whitelist. But these formats are not standard and
> they may cause security vulnerabilities. So yes we need our check for the
> image UTI be strict.
> 
> > Is it possible to test this?
> > 
> 
> This work is covered by LayoutTests/fast/images/image-formats-support.html.
> This patch is about making less system calls in case the image format is not
> supported. I do not think I can make a test for that.

provisional r+
Comment 7 WebKit Commit Bot 2017-07-21 13:38:24 PDT
Comment on attachment 316046 [details]
Patch

Clearing flags on attachment: 316046

Committed r219738: <http://trac.webkit.org/changeset/219738>
Comment 8 WebKit Commit Bot 2017-07-21 13:38:26 PDT
All reviewed patches have been landed.  Closing bug.