Closed Bug 1188010 (CVE-2015-7198) Opened 4 years ago Closed 4 years ago

Overflow in TextureStorage11::setData can cause memory-safety bug

Categories

(Core :: Canvas: WebGL, defect)

39 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox40 --- wontfix
firefox41 + wontfix
firefox42 + fixed
firefox43 + fixed
firefox44 --- fixed
firefox-esr38 42+ fixed

People

(Reporter: q1, Assigned: jgilbert)

Details

(Keywords: csectype-bounds, sec-high, Whiteboard: [post-critsmash-triage][adv-main42+][adv-esr38.4+])

Attachments

(3 files)

TextureStorage11::setData (gfx\angle\src\libGLESv2\renderer\d3d\d3d11\TextureStorage11.cpp) can experience an overflow if its caller specifies a destBox or image argument with unusually-large dimensions. On overflow, setData allocates a buffer that is far too small, then loads an image into it, thus writing memory it does not own.

It is unclear whether checks elsewhere that limit the size of images, such as those in WebGL2Content::TexImage3D (dom\canvas\WebGL2ContentTextures.cpp), make TextureStorage11::setData safe in practice. In any case, it is safer for each function or object to protect itself from insecure operation than for a function in a deeper layer to rely upon checks made by a function closer to the security perimeter.

Details: Using ints and UINTs, setData computes the quantity:

   outputPixelSize * width * height * depth

where outputPixelSize is the number of bytes per pixel (probably always 4), and width, height, and depth are derived from either the destBox argument (if specified) or else the |image| argument:

461: gl::Error TextureStorage11::setData(const gl::ImageIndex &index, Image *image, const gl::Box *destBox, GLenum type,
462:                                     const gl::PixelUnpackState &unpack, const uint8_t *pixelData)
...
485:     int width = destBox ? destBox->width : static_cast<int>(image->getWidth());
486:     int height = destBox ? destBox->height : static_cast<int>(image->getHeight());
487:     int depth = destBox ? destBox->depth : static_cast<int>(image->getDepth());
488:     UINT srcRowPitch = internalFormatInfo.computeRowPitch(type, width, unpack.alignment);
489:     UINT srcDepthPitch = internalFormatInfo.computeDepthPitch(type, width, height, unpack.alignment);
490: 
491:     const d3d11::TextureFormat &d3d11Format = d3d11::GetTextureFormatInfo(image->getInternalFormat());
492:     const d3d11::DXGIFormat &dxgiFormatInfo = d3d11::GetDXGIFormatInfo(d3d11Format.texFormat);
493:
494:     size_t outputPixelSize = dxgiFormatInfo.pixelBytes;
495: 
496:     UINT bufferRowPitch = outputPixelSize * width;
497:     UINT bufferDepthPitch = bufferRowPitch * height;
498: 
499:     MemoryBuffer conversionBuffer;
500:     if (!conversionBuffer.resize(bufferDepthPitch * depth))
501:     {
502:         return gl::Error(GL_OUT_OF_MEMORY, "Failed to allocate internal buffer.");
503:     }
503: 
505:     // TODO: fast path
506:     LoadImageFunction loadFunction = d3d11Format.loadFunctions.at(type);
507:     loadFunction(width, height, depth,
508:                  pixelData, srcRowPitch, srcDepthPitch,
509:                  conversionBuffer.data(), bufferRowPitch, bufferDepthPitch);
...

Thus, a 3D image as small as 1024x1024x1025x32bpp or a 2D image as small as 32768x32769x32bpp could cause an overflow.
Flags: sec-bounty?
Jeff: Similar to the other -- are we up to date with our version of ANGLE? Do you know if we have any image size restrictions that would protect against this?

Christoph: do you know if your webgl fuzzer ever generated images that would exceed those sizes?
Flags: needinfo?(jgilbert)
Flags: needinfo?(cdiehl)
Our worst-case live format here is RGBA32F at 4*32/8=4*4=16 bytes per pixel.
The max 2D texture size ceiling is 16ki (16384).
Worst-case 2D is 16ki*16ki*16, which is 33 bits long. Oops. I'll check whether our WebGL layer protects us.

We don't expose 3D textures yet, but this looks like it will be a problem for them as well.

The fuzzers may never have enabled the extensions it requires to get RGBA32F. Without extensions, RGBA8 is the largest, which is only 4 bytes per pixel, and thus comes in at only 31 bits needed. (safe)
Flags: needinfo?(jgilbert)
EnsureInitializedImageData is not robust against this in non-DEBUG builds.
As the assert in EnsureInitializedImageData states, this should be caught in TexImage2D (or similar).

We should upgrade the assert to a RELEASE_ASSERT.
To be clear, we should be unaffected from the reported bug, but I'm making a tiny patch to guarantee this more strongly.
Actually, we may be vulnerable through CopyTexImage2D.
We should really just refuse all TexImages that have byteSize >= UINT32_MAX.
This is actually basically what D3D does: There's a max resource size.

I think we'd be fine setting a max resource size at only one or two gigs, even.
Component: Graphics → Canvas: WebGL
Assignee: nobody → jgilbert
Attachment #8640755 - Flags: review?(dglastonbury) → review+
(In reply to Daniel Veditz [:dveditz] from comment #1)
> Jeff: Similar to the other -- are we up to date with our version of ANGLE?
> Do you know if we have any image size restrictions that would protect
> against this?
> 
> Christoph: do you know if your webgl fuzzer ever generated images that would
> exceed those sizes?

Yes / No. In general I tried to stay conform with what is allowed and what not, especially for Textures because the failure rate was for those calls were high (very restricted set of parameters were allowed). I am definitely using >= UINT32_MAX from time to time.
Flags: needinfo?(cdiehl)
(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> Actually, we may be vulnerable through CopyTexImage2D.

Marking sec-high because of this.
Group: core-security → gfx-core-security
Jeff, is this ready to get sec-approval and land? Thanks.
Flags: needinfo?(jgilbert)
Comment on attachment 8640755 [details] [diff] [review]
0001-Use-MOZ_RELEASE_ASSERT-when-failure-means-overflows.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It's not obvious how to exploit it.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
All.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Trivial.

How likely is this patch to cause regressions; how much testing does it need?
Not likely to cause regressions; no testing needed.
Flags: needinfo?(jgilbert)
Attachment #8640755 - Flags: sec-approval?
Comment on attachment 8640755 [details] [diff] [review]
0001-Use-MOZ_RELEASE_ASSERT-when-failure-means-overflows.patch

Sec-approval+ for trunk. Please create/nominate patches for ESR38, Aurora, and Beta as well.
Attachment #8640755 - Flags: sec-approval? → sec-approval+
Jeff, could you please request uplift to beta soon (today)? We gtb Beta9 Thursday noon PST so we need this to land asap. Thanks!
Flags: needinfo?(jgilbert)
Milan, if we don't have a patch nominated for uplift by weekend, then this will miss 41RC. Any help here would be awesome.
Flags: needinfo?(milan)
Approval Request Comment
Out of bounds, though without a clear exploit.
Flags: needinfo?(milan)
Attachment #8660097 - Flags: review+
Attachment #8660097 - Flags: approval-mozilla-aurora?
Approval Request Comment
Out of bounds, though without a clear exploit.
Attachment #8660098 - Flags: review+
Attachment #8660098 - Flags: approval-mozilla-beta?
While we should be a little careful about causing crashes on Beta since it's not E10S, this should be fine.
Flags: needinfo?(jgilbert)
We also need an ESR38 patch -- we either uplift to all or wait until next cycle.
Comment on attachment 8660098 [details] [diff] [review]
Beta patch. r=dglastonbury

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: Possible sec-high
Fix Landed on Version: 43
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None

This same patch applies cleanly to esr38.
Attachment #8660098 - Flags: approval-mozilla-esr38?
Comment on attachment 8660098 [details] [diff] [review]
Beta patch. r=dglastonbury

Milan, thanks for a quick uplift request on this one. I thought about it some more and chatted with DVeditz on IRC. We do not feel the need to take this in 41RC even though it seems safe. I think we could have taken it for beta9, but we will just keep 41RC for the sec-crits or other sec issues which are more easily exploitable. Sorry for any inconvenience caused.
Attachment #8660098 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
OK, so we should hold off on landing this for ESR38 as well. It can wait for 38.4.0.
Too late for 41, updating status as wontfix.
Sylvestre, do you want to land this (across the board) for 42.0b2, maybe over the weekend? How do you want to handle it?
Flags: needinfo?(sledru)
If so we should reset the approval flags so that patches will land on the correct branches...
Don't know. Al, can we land this now or should we wait to be closer from the 42 release? Thanks
Flags: needinfo?(sledru) → needinfo?(abillings)
I'd normally suggest landing on October 6, two weeks into the new cycle.
Flags: needinfo?(abillings)
Comment on attachment 8660098 [details] [diff] [review]
Beta patch. r=dglastonbury

Re-setting nom for beta as now we are in Beta42 cycle (this was denied for Beta41)
Attachment #8660098 - Flags: approval-mozilla-beta- → approval-mozilla-beta?
abillings, should we approve these to land everywhere now? Please let me know if I should do it.
Flags: needinfo?(abillings)
Yes, land land land!
Flags: needinfo?(abillings)
Attachment #8660097 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8660098 - Flags: approval-mozilla-esr38?
Attachment #8660098 - Flags: approval-mozilla-esr38+
Attachment #8660098 - Flags: approval-mozilla-beta?
Attachment #8660098 - Flags: approval-mozilla-beta+
this failed to apply to beta

 patching file dom/canvas/WebGLTexture.cpp
Hunk #1 FAILED at 644
1 out of 1 hunks FAILED -- saving rejects to file dom/canvas/WebGLTexture.cpp.rej

can you take a look ? thanks!
Flags: needinfo?(jgilbert)
https://hg.mozilla.org/mozilla-central/rev/a11512671244
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: gfx-core-security → core-security-release
Flags: needinfo?(jgilbert)
clearing c-n flag ...i guess this landed everywhere :)
Keywords: checkin-needed
Flags: sec-bounty? → sec-bounty+
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42+][adv-esr38.4+]
Alias: CVE-2015-7198
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.