Uninitialized variable in DIBPixelData can cause a dangerous memory write

[Zheng Huang]

Created attachment 282757 [details]
poc and analysis and debug code

void DIBPixelData::setRGBABitmapAlpha(HDC hdc, const IntRect& dstRect, unsigned char level)
{    
HBITMAP bitmap = static_cast<HBITMAP>(GetCurrentObject(hdc, OBJ_BITMAP)); // hdc=NULL, so bitmap=NULL
    DIBPixelData pixelData(bitmap);    // when bitmap=NULL, initialize pixelData failed

	ASSERT(pixelData.bitsPerPixel() == 32);

    IntRect drawRect(dstRect);
    XFORM trans;
    GetWorldTransform(hdc, &trans);
    IntSize transformedPosition(trans.eDx, trans.eDy);
    drawRect.move(transformedPosition);

    int pixelDataWidth = pixelData.size().width();    // read from initialize failed pixelData object
    int pixelDataHeight = pixelData.size().height();   // read from initialize failed pixelData object
    IntRect bitmapRect(0, 0, pixelDataWidth, pixelDataHeight);
    drawRect.intersect(bitmapRect);
    if (drawRect.isEmpty())
        return;

    RGBQUAD* bytes = reinterpret_cast<RGBQUAD*>(pixelData.buffer());
    bytes += drawRect.y() * pixelDataWidth;    //bytes tainted by pixelDataWidth

    size_t width = drawRect.width();
    size_t height = drawRect.height();
	
    int x = drawRect.x();
    for (size_t i = 0; i < height; i++) {
        RGBQUAD* p = bytes + x;    //p tainted by bytes
        for (size_t j = 0; j < width; j++) {
            p->rgbReserved = level;     -------------------- crash here, write to uninitialized memory
            p++;
        }
        bytes += pixelDataWidth;
    }
}

[Radar WebKit Bug Importer]

<rdar://problem/27324600>

[Per Arne Vollan]

Created attachment 283653 [details]
Patch

[Brent Fulgham]

Comment on attachment 283653 [details]
Patch

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

r+, but please revise the title a little to read more clearly. Also, can you make some clarifying statements about what constitutes a passing or failing test? Do we crash or something in the original "unfixed" case?

> Source/WebCore/ChangeLog:3
> +        DIBPixelData use uninitialized variable cause dangerous memory write

Can we rephrase this:

Uninitialized variable in DIBPixelData can cause a dangerous memory write

> Tools/TestWebKitAPI/Tests/WebCore/win/DIBPixelData.cpp:38
> +    DIBPixelData::setRGBABitmapAlpha(nullptr, IntRect(), 0);

How do we know if this test passes? Do we get a crash before your fix? Or if we run with DebugMalloc do we get an error in the original code case?

[Per Arne Vollan]

(In reply to comment #3)
> Comment on attachment 283653 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283653&action=review
> 
> r+, but please revise the title a little to read more clearly. Also, can you
> make some clarifying statements about what constitutes a passing or failing
> test? Do we crash or something in the original "unfixed" case?
> 
> > Source/WebCore/ChangeLog:3
> > +        DIBPixelData use uninitialized variable cause dangerous memory write
> 
> Can we rephrase this:
> 
> Uninitialized variable in DIBPixelData can cause a dangerous memory write
> 
> > Tools/TestWebKitAPI/Tests/WebCore/win/DIBPixelData.cpp:38
> > +    DIBPixelData::setRGBABitmapAlpha(nullptr, IntRect(), 0);
> 
> How do we know if this test passes? Do we get a crash before your fix? Or if
> we run with DebugMalloc do we get an error in the original code case?

Yes, the test would crash before the fix.

Thanks for reviewing! I will update the patch accordingly before landing :)

[Per Arne Vollan]

Committed r203274: <https://trac.webkit.org/changeset/203274>

[Zheng Huang]

Hi, I want to ask about this bug, is that I can get a CVE for this bug and get an acknowledgment by apple?

[Brent Fulgham]

(In reply to comment #6)
> Hi, I want to ask about this bug, is that I can get a CVE for this bug and
> get an acknowledgment by apple?

I'll talk to Product Security and have someone contact you directly. Thank you for filing the bug!

[Zheng Huang]

Hi，Brent Fulgham，nobody contact me for 20 days for this bug, is it normal?