Closed Bug 1357090 (CVE-2017-7754) Opened 3 years ago Closed 3 years ago

Out-of-bounds array access in WebGLTexture::ImageInfoAtFace

Categories

(Core :: Canvas: WebGL, defect)

51 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- wontfix
firefox-esr52 54+ verified
firefox53 --- wontfix
firefox54 + verified
firefox55 + verified

People

(Reporter: tk.mozilla, Assigned: jgilbert)

Details

(Keywords: csectype-bounds, sec-high, testcase, Whiteboard: [post-critsmash-triage][adv-main54+][adv-esr52.2+])

Attachments

(5 files)

Attached file 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).
Attached file ASAN_output.txt
Attached file WinDbg_output.txt
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?
Group: firefox-core-security → core-security
Has Regression Range: --- → yes
Component: Untriaged → Canvas: WebGL
Flags: needinfo?(jgilbert)
Flags: in-testsuite?
Product: Firefox → Core
Version: 52 Branch → 51 Branch
Flags: sec-bounty?
Group: core-security → gfx-core-security
Track 54+/55+ as sec-high.
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
Keywords: testcase
Sec-high triage: Milan, we need somebody to be assigned to this bug.
Flags: needinfo?(milan)
Assignee: nobody → jgilbert
Flags: needinfo?(milan)
Flags: needinfo?(jgilbert)
Attachment #8866566 - Flags: review?(dmu)
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
Attachment #8866566 - Flags: review?(dmu) → review+
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 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.
Attachment #8866566 - Flags: sec-approval?
Attachment #8866566 - Flags: approval-mozilla-esr52?
Attachment #8866566 - Flags: approval-mozilla-aurora?
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.
Attachment #8866566 - Flags: approval-mozilla-beta?
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.
Whiteboard: [checkin on 5/22]
Attachment #8866566 - Flags: sec-approval?
Attachment #8866566 - Flags: sec-approval+
Attachment #8866566 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d0d5775aec98a66fdb30594acedee5c80d78e5
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: [checkin on 5/22]
https://hg.mozilla.org/mozilla-central/rev/a5d0d5775aec
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Attachment #8866566 - Flags: approval-mozilla-esr52?
Attachment #8866566 - Flags: approval-mozilla-esr52+
Attachment #8866566 - Flags: approval-mozilla-beta?
Attachment #8866566 - Flags: approval-mozilla-beta+
Group: gfx-core-security → core-security-release
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Flags: sec-bounty? → sec-bounty+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54+][adv-esr52.2+]
Alias: CVE-2017-7754
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.