Bug 163670 - Refine assertions in WebCore::ImageData constructors
Summary: Refine assertions in WebCore::ImageData constructors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks: 163696
  Show dependency treegraph
 
Reported: 2016-10-19 10:35 PDT by David Kilzer (:ddkilzer)
Modified: 2016-10-20 15:32 PDT (History)
8 users (show)

See Also:


Attachments
Patch v1 (1.98 KB, patch)
2016-10-19 10:37 PDT, David Kilzer (:ddkilzer)
bfulgham: review+
ddkilzer: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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