1357090 - (CVE-2017-7754) Out-of-bounds array access in WebGLTexture::ImageInfoAtFace

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Tobias Klein]

Attachment: testcase.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36

Steps to reproduce:

Tested on:
 - Mozilla Firefox 52.0.2 (32-bit) on Windows 10
 - Nightly 55.0a1 (2017-04-10) (64-bit) on Ubuntu 16.04 LTS
 
Details:

dom/canvas/WebGLTexture.h:

..
313    ImageInfo& ImageInfoAtFace(uint8_t face, uint32_t level) {
314        MOZ_ASSERT(face < mFaceCount);
315        MOZ_ASSERT(level < kMaxLevelCount);
316        size_t pos = (level * mFaceCount) + face;
317        return mImageInfoArr[pos];
318    }
..

By supplying a sufficiently large value to 'level' (see the third parameter of gl.texParameterf() in the provided testcase file), it is possible to cause an out-of-bounds array access in line 317. This may result in a bogus ImageInfo object being returned to the calling function, possibly leading to an exploitable state.

Provided files:

- testcase.html -- Testcase to trigger the issue.
- ASAN_output.txt -- Detailed ASAN output (Linux).
- WinDbg_output.txt -- Further debugging information (Windows).

Comment 1

[Tobias Klein]

Attachment: ASAN_output.txt

Comment 2

[Tobias Klein]

Attachment: WinDbg_output.txt

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Note that the attached testcase doesn't always crash instantaneously for me. Took a few seconds sometimes.

INFO: Last good revision: 9329689b3f83bd2545c1c8e56807a973dcdc1062
INFO: First bad revision: 5a10fa45d8eac53a005130946302c3fb2471d9e2
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9329689b3f83bd2545c1c8e56807a973dcdc1062&tochange=5a10fa45d8eac53a005130946302c3fb2471d9e2

Jeff, not sure which commit in that range actually is the culprit here. Can you please take a look?

Comment 4

[Gerry Chang [:gchang]]

Track 54+/55+ as sec-high.

Comment 5

[Julian Hector [:tedd] [:jhector]]

Attachment: GDB backtrace of ImageInfoAtFace()

I took a look at this bug, and maybe have some relevant information.
First off, attached is my gdb backtrace when hitting the function ImageInfoAtFace() which triggered immediately after I continued the execution with gdb once I set the breakpoint.

From what I can tell is that |mBaseMipmapLevel| holds the value of |level| passed to the quoted function by the reporter.

Further
> gl.texParameterf(gl.TEXTURE_CUBE_MAP, gl.TEXTURE_BASE_LEVEL, 0x50);
in the testcase seems to set that member variable here [1]

The vulnerable function seems to be triggered by calling
> gl.drawArrays(gl.TRIANGLES, 0, 1);
in the testcase. 

In my debug build (the one I used for debugging), the MOZ_ASSERTS, are present [2] and would cause a crash.

However since those are not MOZ_RELEASE_ASSERT, the reporter probably didn't hit them with his testcase.

As a side note, there are different code path that validate |mBaseMipmapLevel| to be less than |kMaxLevelCount| which in my build is 31. However in this particular code path (shown in my gdb backtrace and in the reporters windbg output) |mBaseMipmapLevel| is not validated, thus leading to the out-of-bound array access.

Also, I would consider increasing the security rating, because the object read by the out-of-bound is of the class ImageInfo [3].
Given that |mBaseMipmapLevel| is of type uint32_t [4], the accessible memory range is pretty big (on 64 bit, and the entire memory range on 32 bit) so crafting a malicious ImageInfo object and let it be accessed by this out-of-bounds shouldn't be too hard.


[1] http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/dom/canvas/WebGLTexture.cpp#1164
[2] http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/dom/canvas/WebGLTexture.h#314-315
[3] http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/dom/canvas/WebGLTexture.h#115
[4] http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/dom/canvas/WebGLTexture.h#84

Comment 6

[Stephanie Ouillon [:arroway] (needinfo me)]

Sec-high triage: Milan, we need somebody to be assigned to this bug.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Bug-1357090-Check-that-a-texture-s-level_base-is-not.patch

Comment 8

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8866566 [details] [diff] [review]
0001-Bug-1357090-Check-that-a-texture-s-level_base-is-not.patch

Review of attachment 8866566 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 9

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8866566 [details] [diff] [review]
0001-Bug-1357090-Check-that-a-texture-s-level_base-is-not.patch

Review of attachment 8866566 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLTexture.cpp
@@ +314,5 @@
>  {
>      *out_initFailed = false;
>  
> +    const auto maxLevel = kMaxLevelCount - 1;
> +    if (mBaseMipmapLevel > maxLevel) {

I prefer to use "if (mBaseMipmapLevel >= kMaxLevelCount)", this also follows the same style here, https://dxr.mozilla.org/mozilla-central/rev/ebbcdaa5b5802ecd39624dd2acbdda8547b8384d/dom/canvas/WebGLTexture.h#342

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8866566 [details] [diff] [review]
0001-Bug-1357090-Check-that-a-texture-s-level_base-is-not.patch

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

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: sec-high
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: run the testcase here and ensure it doesn't crash.
[List of other uplifts needed for the feature/fix]: aurora54
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple fix
[String changes made/needed]: none

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Easily.

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

Which older supported branches are affected by this flaw?
>45, but not 45.

If not all supported branches, which bug introduced the flaw?
WebGL texture refactor.

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

How likely is this patch to cause regressions; how much testing does it need?
Not likely to regress, but I will add tests to the upstream test suite.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8866566 [details] [diff] [review]
0001-Bug-1357090-Check-that-a-texture-s-level_base-is-not.patch

I guess beta is 54 also.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

I'm giving this sec-approval for checkin into trunk on May 22 (not earlier). 

The Aurora flag doesn't matter as Aurora doesn't exist anymore so I'll clear that.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d0d5775aec98a66fdb30594acedee5c80d78e5

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a5d0d5775aec

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8866566 [details] [diff] [review]
0001-Bug-1357090-Check-that-a-texture-s-level_base-is-not.patch

Fix a sec-high. Beta54+ & ESR52+. Should be in 54 beta 11.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dc0239918676
https://hg.mozilla.org/releases/mozilla-esr52/rev/d07f24a72ce4

Comment 17

[Andrei Vaida [:avaida]]

Reproduced the crash on Ubuntu 16.04 x64 using an affected build (55.0a1 linux64-asan from 2017-04-10, 20170410233251) and the test case provided by the reporter in Comment 0.


This bug is verified fixed on Ubuntu 16.04 x64, Windows 10 x64 and macOS 10.12.5 using the following builds:

  * 55.0a1 linux64-asan from 2017-06-09 (20170609011144)
  * 55.0a1 (2017-06-08)
  * 54.0 (20170608175746)
  * 52.2.0esr (20170607123825)

The testcase is no longer crashing.