Bug 174692 – [CG] An image should not invoke many system calls before confirming its format is supported

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 316029 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/33388544>

[Jeremy Jones]

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

[Said Abou-Hallawa]

Created attachment 316046 [details]
Patch

[Said Abou-Hallawa]

(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.

[Jeremy Jones]

(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+

[WebKit Commit Bot]

Comment on attachment 316046 [details]
Patch

Clearing flags on attachment: 316046

Committed r219738: <http://trac.webkit.org/changeset/219738>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.