Bug 163670

Summary: Refine assertions in WebCore::ImageData constructors
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: ImagesAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, commit-queue, darin, esprehn+autocc, gyuyoung.kim, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 10   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=163696
https://bugs.webkit.org/show_bug.cgi?id=163757
https://bugs.webkit.org/show_bug.cgi?id=163762
Bug Depends on:    
Bug Blocks: 163696    
Attachments:
Description Flags
Patch v1 bfulgham: review+, ddkilzer: commit-queue?

Description David Kilzer (:ddkilzer) 2016-10-19 10:35:10 PDT
The ASSERT_WITH_SECURITY_IMPLICATION() macros in WebCore::ImageData constructors identify nullptr dereferences as security issues, which they are not.
Comment 1 David Kilzer (:ddkilzer) 2016-10-19 10:35:19 PDT
<rdar://problem/27497338>
Comment 2 David Kilzer (:ddkilzer) 2016-10-19 10:37:21 PDT
Created attachment 292082 [details]
Patch v1
Comment 3 Brent Fulgham 2016-10-19 11:41:55 PDT
Comment on attachment 292082 [details]
Patch v1

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

r=me

> Source/WebCore/html/ImageData.cpp:126
> +    ASSERT_WITH_SECURITY_IMPLICATION(!m_data || static_cast<unsigned>(size.width() * size.height() * 4) <= m_data->length());

Suggest: size.area() * 4
Comment 4 David Kilzer (:ddkilzer) 2016-10-19 12:09:02 PDT
(In reply to comment #3)
> Comment on attachment 292082 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292082&action=review
> 
> r=me
> 
> > Source/WebCore/html/ImageData.cpp:126
> > +    ASSERT_WITH_SECURITY_IMPLICATION(!m_data || static_cast<unsigned>(size.width() * size.height() * 4) <= m_data->length());
> 
> Suggest: size.area() * 4

Should I change the other constructor as well?

ImageData::ImageData(const IntSize& size)
    : m_size(size)
    , m_data(Uint8ClampedArray::createUninitialized(size.width() * size.height() * 4))
Comment 5 David Kilzer (:ddkilzer) 2016-10-19 12:14:16 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 292082 [details]
> > Patch v1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=292082&action=review
> > 
> > r=me
> > 
> > > Source/WebCore/html/ImageData.cpp:126
> > > +    ASSERT_WITH_SECURITY_IMPLICATION(!m_data || static_cast<unsigned>(size.width() * size.height() * 4) <= m_data->length());
> > 
> > Suggest: size.area() * 4
> 
> Should I change the other constructor as well?
> 
> ImageData::ImageData(const IntSize& size)
>     : m_size(size)
>     , m_data(Uint8ClampedArray::createUninitialized(size.width() *
> size.height() * 4))

Talked to Brent in person, and we agreed that we should change this one as well to match.
Comment 6 David Kilzer (:ddkilzer) 2016-10-19 12:55:34 PDT
Committed r207560: <http://trac.webkit.org/changeset/207560>
Comment 7 Darin Adler 2016-10-20 09:43:30 PDT
What protects the area and * 4 computations from overflowing?
Comment 8 Brent Fulgham 2016-10-20 09:45:30 PDT
(In reply to comment #7)
> What protects the area and * 4 computations from overflowing?

Nothing!

David and I were talking about this specific issue yesterday. David is working on a separate patch to do CheckedArithmetic math on these calculations to avoid overflowing.
Comment 9 David Kilzer (:ddkilzer) 2016-10-20 15:32:28 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > What protects the area and * 4 computations from overflowing?
> 
> Nothing!
> 
> David and I were talking about this specific issue yesterday. David is
> working on a separate patch to do CheckedArithmetic math on these
> calculations to avoid overflowing.

I originally filed:

Bug 163696: Extract common code for computing byte count for images

But Simon didn't like the direction there, so I broke out that patch into:

Bug 163757: Use IntSize::unclampedArea() in PDFDocumentImage::updateCachedImageIfNeeded()
Bug 163762: InSize::area() should used checked arithmetic