Closed Bug 1329369 Opened 3 years ago Closed 3 years ago

Crash in angle::LoadToNative3To4<T>


(Core :: Canvas: WebGL, defect, critical)

Not set



Tracking Status
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- disabled
firefox-esr52 --- disabled
firefox53 + verified
firefox54 + verified


(Reporter: marcia, Assigned: mattwoodrow)



(4 keywords, Whiteboard: gfx-noted)

Crash Data


(1 file)

This bug was filed from the Socorro interface and is 
report bp-e6830064-8c3b-4873-8de6-9c68d2170107.

Seen while reviewing nightly crash stats. Crashes started using 20170106030204 build:

Possible regression range:
#8 topcrash in Nightly 20170106030204.

Here's one interesting correlation:

> (86.84% in signature vs 45.82% overall) platform_pretty_version = Windows 10

jrmuizel, any ideas?
Flags: needinfo?(jmuizelaar)
Passing this on to jgilbert.
Flags: needinfo?(jmuizelaar) → needinfo?(jgilbert)
Whiteboard: gfx-noted
There aren't any user comments, so it's pretty hard to figure out a repro here.
Flags: needinfo?(jgilbert)
Assignee: nobody → jgilbert
Very very weird that this would be Nightly53-only, given how we've been uplifting almost everything to at least Aurora52, and usually Beta51.

I didn't check all the reports, but they look like they come in through the texImage2D(HTMLVideoElement) path, so perhaps it's ancient BGRA stuff?
OS: Windows 10 → Windows
Peter, maybe somebody can take a look at this while Jeff is finishing up other stuff?
Since all the reports have valid-looking addresses for the read errors, marking as a sec bug.  Not sure as to sec severity, depends on what the data read is used for.

Milan, could you needinfo whichever Peter you wanted to look at it?  Thanks
Group: core-security
Flags: needinfo?(milan)
Keywords: csectype-uaf
Flags: needinfo?(milan) → needinfo?(howareyou322)
The same crash could be traced back to build 20161218030213.

I suspect bug 1323617 was related but it was also landed to beta.
Flags: needinfo?(howareyou322)
Peter - thoughts on severity?  Jeff, will you be looking into this now?
Flags: needinfo?(jgilbert)
Peter's team will.
Assignee: jgilbert → howareyou322
Flags: needinfo?(jgilbert)
Group: core-security → gfx-core-security
Ethan, please check comment 8 for possible root cause.
Assignee: howareyou322 → ethlin
I compared related files between beta and nightly. The patches in bug 1329815, bug 1250077 and bug 1316546 are not uplifted to 52, though they should be not related to this bug according to the push time.
(In reply to Peter Chang[:pchang] from comment #8)
> The same crash could be traced back to build 20161218030213.
> d14df2161219
> I suspect bug 1323617 was related but it was also landed to beta.
> pushloghtml?startdate=6+weeks+ago&enddate=5+weeks+ago

Strange. I didn't find any suspicious webgl patch near the build 20161218030213. Another non-uplifted webgl bug is bug 1325995, but the push date is 2017/01/04.
According to the minidump file from [1], the internalFormat/format are GL_RGB and the type is GL_UNSIGNED_BYTE. I'll see if I can reproduce on my local device.

Ethan - any word?
Flags: needinfo?(ethlin)
I wrote a test with the same format, but the crash didn't happen. And I noticed there is a user comment said the web page is webgl2 conformance test. So I reproduced the same crash with conformance tests on a laptop, though I'm not sure which test caused this problem. Next, I'll find out the test.
Flags: needinfo?(ethlin)
I can reproduce the problem with the test[1] on windows. And the regression bug is bug 1315141 - Enable GPU video decoding on Nightly. If I turn the pref off, the crash won't happen. Matt, do you have any idea for this? According to the coredump, it looks like the source buffer is corrupt. 

Flags: needinfo?(matt.woodrow)
See Also: → 1315141
Flags: needinfo?(matt.woodrow)
Attached patch video-sizeSplinter Review
WebGL uses the size of the layers::Image, not the underlying surface.

We were initializing the layers::Image to the display size of the video, not the actual pixel size of the buffer.
Assignee: ethlin → matt.woodrow
Attachment #8838348 - Flags: review?(jyavenard)
Comment on attachment 8838348 [details] [diff] [review]

Review of attachment 8838348 [details] [diff] [review]:

::: dom/media/ipc/VideoDecoderParent.cpp
@@ +185,5 @@
>      VideoDataIPDL output(
>        MediaDataIPDL(data->mOffset, data->mTime, data->mTimecode,
>                      data->mDuration, data->mFrames, data->mKeyframe),
>        video->mDisplay,
> +      texture ? texture->GetSize() : IntSize(),

when would texture be null?
Attachment #8838348 - Flags: review?(jyavenard) → review+
We'll want to uplift this to 53, right?
Comment on attachment 8838348 [details] [diff] [review]

Each decoded video frame specifies a 'display size' as well as containing the actual frame pixel contents (with a given size). WebGL was using the display size to determine how many bytes to upload to GL, which case overrun the buffer when the display size is larger than the actual size of the frame.

I'm not sure how exploitable that is, but it at least seems plausible that you could trigger sensitive bytes to be uploaded into a WebGL texture (and drawn to the screen, then read back into JS accessible memory).

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

Not super easily, the fix is in the code for sharing decoded frames between the decoding (GPU process) and content process. The buffer overrun is in WebGL code, one of the consumers of the decoder frame.

It certainly would be possible for someone to audit the consumers of decoded frames looking for ones that change behaviour based on the new size we're passing though.

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

Not particularly, same as above.

Which older supported branches are affected by this flaw?

53 is affected, 52 has the code but it's preffed off.

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

52 introduced it (preffed off), 53 enabled it.

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

Should be the exact same patch.

How likely is this patch to cause regressions; how much testing does it need?

Very unlikely, simple fix.
Attachment #8838348 - Flags: sec-approval?
Comment on attachment 8838348 [details] [diff] [review]

Approval Request Comment
See security-approval request

Simple patch
Attachment #8838348 - Flags: approval-mozilla-aurora?
Giving sec-approval since this is pref'd off in 52. I'll give Aurora approval as well.
Attachment #8838348 - Flags: sec-approval?
Attachment #8838348 - Flags: sec-approval+
Attachment #8838348 - Flags: approval-mozilla-aurora?
Attachment #8838348 - Flags: approval-mozilla-aurora+
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Group: gfx-core-security → core-security-release
Reproduced on Nightly 2017-01-15, Win 10.
Verified fixed Fx 53b10, 54.0a2 (2017-04-11).
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.