New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 446164 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security
Nag



Sign in to add a comment

Security: Integer Overflow in WebGL

Reported by dem...@gmail.com, Jan 5 2015

Issue description

VULNERABILITY DETAILS
When calling texImage2D in JavaScript, it will call WebGLRenderingContextBase::texImage2D. 
If the image type is svg it will drawImageIntoBuffer, during which a large image width (>= 0x40000000) will cause multiply integer overflow.


// JavaScript
textureImage.width = 0x40000001;
gl.texImage2D(0, 0, 0, 0, 0, textureImage);


// E:\depot_tools\src\third_party\WebKit\Source\core\html\canvas\WebGLRenderingContextBase.cpp
void WebGLRenderingContextBase::texImage2D(GLenum target, GLint level, GLenum internalformat,
        GLenum format, GLenum type, HTMLImageElement* image, ExceptionState& exceptionState) {
    ...
    if (imageForRender->isSVGImage())
        imageForRender = drawImageIntoBuffer(imageForRender.get(), image->width(), image->height(), "texImage2D");
    ...
}

// E:\depot_tools\src\third_party\skia\src\core\SkMallocPixelRef.cpp 
SkMallocPixelRef* SkMallocPixelRef::NewAllocate(const SkImageInfo& info, size_t requestedRowBytes, SkColorTable* ctable) {
    // info {fWidth=0x40000001 fHeight=0x00000096 ...}
    ...
    int32_t minRB = SkToS32(info.minRowBytes()); // Overflow: see minRowBytes(), minRB = 0x00000004
    ...
    rowBytes = minRB; 

    int64_t bigSize = (int64_t)info.height() * rowBytes;
    ...
    size_t size = sk_64_asS32(bigSize);
    SkASSERT(size >= info.getSafeSize(rowBytes));
    void* addr = sk_malloc_flags(size, 0); // Allocate memory with overflowed size = 0x00000258
    ...
}

size_t minRowBytes() const {
    return (size_t)this->minRowBytes64();
    // return 0x00000004
}

uint64_t minRowBytes64() const {
    return sk_64_mul(fWidth, this->bytesPerPixel()); // fWidth = 0x40000001, this->bytesPerPixel() = 0x4
    // return 0x0000000100000004
}


Call Stack:
skia.dll!SkMallocPixelRef::NewAllocate(const SkImageInfo & info={...}, unsigned int requestedRowBytes=0x00000000, SkColorTable * ctable=0x00000000)
skia.dll!SkSurface::NewRaster(const SkImageInfo & info={...}, const SkSurfaceProps * props=0x0018e380)
blink_platform.dll!blink::UnacceleratedImageBufferSurface::UnacceleratedImageBufferSurface(const blink::IntSize & size={...}, blink::OpacityMode opacityMode=NonOpaque)
blink_platform.dll!blink::ImageBuffer::create(const blink::IntSize & size={...}, blink::OpacityMode opacityMode=NonOpaque)
blink_web.dll!blink::WebGLRenderingContextBase::LRUImageBufferCache::imageBuffer(const blink::IntSize & size={...})
blink_web.dll!blink::WebGLRenderingContextBase::drawImageIntoBuffer(blink::Image * image=0x322748d0, int width=0x40000001, int height=0x00000096, const char * functionName=0x1e15332c)
blink_web.dll!blink::WebGLRenderingContextBase::texImage2D(unsigned int target=0x00000000, int level=0x00000000, unsigned int internalformat=0x00000000, unsigned int format=0x00000000, unsigned int type=0x00000000, blink::HTMLImageElement * image=0x5ee383d0, blink::ExceptionState & exceptionState={...})
blink_web.dll!blink::WebGLRenderingContextV8Internal::texImage2D3Method(const v8::FunctionCallbackInfo<v8::Value> & info={...})
blink_web.dll!blink::WebGLRenderingContextV8Internal::texImage2DMethod(const v8::FunctionCallbackInfo<v8::Value> & info={...})
blink_web.dll!blink::WebGLRenderingContextV8Internal::texImage2DMethodCallback(const v8::FunctionCallbackInfo<v8::Value> & info={...})
v8.dll!v8::internal::FunctionCallbackArguments::Call(void (const v8::FunctionCallbackInfo<v8::Value> &)
v8.dll!v8::internal::HandleApiCallHelper<0>(v8::internal::`anonymous-namespace'::BuiltinArguments<1> args={...}, v8::internal::Isolate * isolate=0x2c108091)
v8.dll!v8::internal::Builtin_HandleApiCall(int args_length=0x00000008, v8::internal::Object * * args_object=0x0018e77c, v8::internal::Isolate * isolate=0x2bc52100)
...
JavaScript JIT code

VERSION
Chrome Version: 41.0.2246.0 + dev
Operating System: Windows 7 ultimate 64-bit Service Pack 1

REPRODUCTION CASE
attachment
 
2015-01-05@Integer Overflow in WebGL.html
708 bytes View Download
demiSvg.svg
46 bytes Download
Project Member

Comment 1 by ClusterFuzz, Jan 6 2015

ClusterFuzz is analyzing your testcase. Chromium developers can follow the progress at https://cluster-fuzz.appspot.com/testcase?key=6298003131334656
Cc: zmo@chromium.org
Owner: kbr@chromium.org
Status: Assigned
Ken, can you please help with an owner.

Comment 3 by zmo@chromium.org, Jan 6 2015

Cc: -zmo@chromium.org bajones@chromium.org kbr@chromium.org
Owner: zmo@chromium.org
Labels: Pri-1 Security_Severity-High Security_Impact-Stable OS-All Cr-Blink-WebGL
Labels: M-40

Comment 6 by zmo@chromium.org, Jan 8 2015

Cc: zmo@chromium.org
Owner: reed@chromium.org
I did some code reading.  It seems to me that there is nothing to be done in Chromium/Blink side, as it expects an invalid surface from SkSurface::NewRaster at overflow.

However, SkMallocPixelRef::NewAllocate needs to be more overflow-aware.

Assigning to @reed.

Comment 7 by dem...@gmail.com, Jan 23 2015

@zmo: Good analysis, thank you for helping reading the code!
Project Member

Comment 8 by ClusterFuzz, Jan 23 2015

Labels: Nag
reed@: Uh oh! This issue is still open and hasn't been updated in the last 14 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz

Comment 9 by reed@chromium.org, Jan 23 2015

Cc: caryclark@google.com
Owner: reed@google.com

Comment 10 by dem...@gmail.com, Feb 5 2015

Well, I hope I can help...

Comment 11 by dem...@gmail.com, Feb 6 2015

clusterfuzz@: I have tried to disable these nags, while I can't find how to add 'WIP' label and create an optional codereview link. Can you please help telling me how to do it?
demi6d: the nags are for the developer fixing bug and not you.
Project Member

Comment 13 by ClusterFuzz, Feb 6 2015

reed@: Uh oh! This issue is still open and hasn't been updated in the last 28 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz

Comment 14 by dem...@gmail.com, Feb 7 2015

inferno: Thank you very much for your information! I just think when developer is focusing on developing one module, it really disrupt uncomfortably to change to another one, so I just want to help...
And according to @zmo's analysis "expects an invalid surface from SkSurface::NewRaster at overflow", I can do check in SkMallocPixelRef::NewAllocate and if overflowed then set surface to null, because invalid surface is null according to isValid():
bool UnacceleratedImageBufferSurface::isValid() const {
    return m_surface;
}
Although I'm not familiar with the code base, this logic seems not very complicate or maybe I miss something... I'm thinking about that maybe I can register a chromium account in the future when I am qualified :)
demi6d: thanks a lot for all your analysis. do you want to take a shot at uploading a chromium patch on codereview.chromium.org. and please mark zmo@, kbr@ as reviewiers. Uploading a patch will make this qualify for the higher rewards.

Comment 16 by dem...@gmail.com, Feb 8 2015

inferno: thank you for your help! I just learnt how to upload a cl, updated my chromium code and compile tools today and then plan to write a fix... While reading the related code, I find that reed@ has already fixed it with same idea but very nice code two weeks ago just when assigned...

https://codereview.chromium.org/871993003

@reed: quick and great fix, thank you!
Project Member

Comment 18 by ClusterFuzz, Feb 8 2015

Labels: -Restrict-View-SecurityTeam Merge-Triage M-41 Restrict-View-SecurityNotify
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

- Your friendly ClusterFuzz

Comment 19 by dem...@gmail.com, Feb 9 2015

inferno: thank you for always quick response to the report!


If there is a credit, I hope it the same as before: 

Chen Zhang (demi6od) of the NSFOCUS Security Team

Comment 20 Deleted

Labels: reward-topanel
Sometimes we forget to add the reward-topanel label, but we're usually pretty good about going through the bugs to see if we missed any before voting on reward amounts for a release. Either way, this one definitely should have it.

Comment 22 by dem...@gmail.com, Feb 11 2015

Thank you for your information!
Cc: timwillis@chromium.org
Labels: -M-40 -Merge-Triage Merge-Requested
Merge Requested to M41 (Branch 2272)
Labels: -Merge-Requested Merge-Review Hotlist-Merge-Review
[Automated comment] No bugdroid (commit) comments found, couldn't auto-approve, needs manual review.
Labels: -Merge-Review -Hotlist-Merge-Review Merge-Approved
Merge approved for M41 branch 2272.
Note: M41 stable cut happens in days, and you're approved for merge.  Get it in there!  (Let me know if you need any help, or aren't confident.)
reed: Please merge your fix to M41 (branch 2272). Thanks.
Cc: hcm@chromium.org
You have about 18 hours to get this into M41, or it'll be punted.  I can't do the merge for you in skia.  Adding Heather Miller in case she can help.

Comment 29 by hcm@chromium.org, Feb 24 2015

Thanks for the CC. I can cherry pick into our Skia chrome/m41 branch, but want to confirm with Mike (and have him do it/test if possible).. will fup early AM.

Comment 30 by hcm@chromium.org, Feb 24 2015

Labels: -Merge-Approved Merge-Merged
I cherry-picked and tested the change into the chrome/m41 branch of Skia.  It is on tip of branch now and should get picked up in the next build.
Labels: Release-0-M41
Labels: -reward-topanel reward-3000 reward-unpaid CVE-2015-1219
Congratulations - $3000 for this report.

Comment 33 by dem...@gmail.com, Mar 4 2015

Thank you very much!
Labels: -reward-unpaid reward-inprocess
Labels: -reward-inprocess
Processing via our e-payment system can take up to six weeks, but the reward should be on its way to you. Thanks again for your help!

Comment 36 by dem...@gmail.com, Mar 18 2015

Thank you for your work and message!
Project Member

Comment 37 by ClusterFuzz, May 17 2015

Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Project Member

Comment 38 by sheriffbot@chromium.org, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 39 by sheriffbot@chromium.org, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic
Labels: CVE_description-submitted

Sign in to add a comment