Closed
Bug 1329369
Opened 7 years ago
Closed 7 years ago
Crash in angle::LoadToNative3To4<T>
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
VERIFIED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox-esr45 | --- | unaffected |
firefox50 | --- | unaffected |
firefox51 | --- | unaffected |
firefox52 | --- | disabled |
firefox-esr52 | --- | disabled |
firefox53 | + | verified |
firefox54 | + | verified |
People
(Reporter: marcia, Assigned: mattwoodrow)
References
Details
(4 keywords, Whiteboard: gfx-noted)
Crash Data
Attachments
(1 file)
2.71 KB,
patch
|
jya
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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: http://bit.ly/2j0q5O4 Possible regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f13abb8ba9f366c9f32a3146245adf642528becd&tochange=a14094edbad78fc1d16e8d4c57902537cf286fd1
Comment 1•7 years ago
|
||
#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)
Comment 2•7 years ago
|
||
Passing this on to jgilbert.
Flags: needinfo?(jmuizelaar) → needinfo?(jgilbert)
Updated•7 years ago
|
Whiteboard: gfx-noted
Comment 3•7 years ago
|
||
There aren't any user comments, so it's pretty hard to figure out a repro here.
Flags: needinfo?(jgilbert)
Updated•7 years ago
|
Assignee: nobody → jgilbert
Updated•7 years ago
|
status-firefox50:
--- → unaffected
status-firefox51:
--- → unaffected
status-firefox52:
--- → unaffected
Comment 4•7 years ago
|
||
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?
Updated•7 years ago
|
OS: Windows 10 → Windows
Peter, maybe somebody can take a look at this while Jeff is finishing up other stuff?
Maybe bug 1328559?
See Also: → 1328559
Comment 7•7 years ago
|
||
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
Updated•7 years ago
|
Flags: needinfo?(milan) → needinfo?(howareyou322)
Comment 8•7 years ago
|
||
The same crash could be traced back to build 20161218030213. https://crash-stats.mozilla.com/report/index/40bd1e6c-ad4a-411e-91fc-d14df2161219 I suspect bug 1323617 was related but it was also landed to beta. https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=6+weeks+ago&enddate=5+weeks+ago
Flags: needinfo?(howareyou322)
Comment 9•7 years ago
|
||
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)
Updated•7 years ago
|
Group: core-security → gfx-core-security
Comment 11•7 years ago
|
||
Ethan, please check comment 8 for possible root cause.
Assignee: howareyou322 → ethlin
Comment 12•7 years ago
|
||
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.
Comment 13•7 years ago
|
||
(In reply to Peter Chang[:pchang] from comment #8) > The same crash could be traced back to build 20161218030213. > > https://crash-stats.mozilla.com/report/index/40bd1e6c-ad4a-411e-91fc- > d14df2161219 > > I suspect bug 1323617 was related but it was also landed to beta. > > https://hg.mozilla.org/mozilla-central/ > 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.
Comment 14•7 years ago
|
||
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. [1] https://crash-stats.mozilla.com/report/index/38ba475e-337b-4046-b25c-4e49b2170202
Comment 16•7 years ago
|
||
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)
Comment 17•7 years ago
|
||
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. [1] https://www.khronos.org/registry/webgl/sdk/tests/conformance2/textures/misc/npot-video-sizing.html?webglVersion=2&quiet=0
Flags: needinfo?(matt.woodrow)
See Also: → 1315141
Updated•7 years ago
|
status-firefox54:
--- → affected
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 18•7 years ago
|
||
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 19•7 years ago
|
||
Comment on attachment 8838348 [details] [diff] [review] video-size 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?
Assignee | ||
Comment 21•7 years ago
|
||
Comment on attachment 8838348 [details] [diff] [review] video-size 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 22•7 years ago
|
||
Comment on attachment 8838348 [details] [diff] [review] video-size Approval Request Comment See security-approval request Simple patch
Attachment #8838348 -
Flags: approval-mozilla-aurora?
Comment 23•7 years ago
|
||
Giving sec-approval since this is pref'd off in 52. I'll give Aurora approval as well.
status-firefox-esr45:
--- → unaffected
status-firefox-esr52:
--- → disabled
tracking-firefox53:
--- → +
tracking-firefox54:
--- → +
Keywords: sec-high
Updated•7 years ago
|
Attachment #8838348 -
Flags: sec-approval?
Attachment #8838348 -
Flags: sec-approval+
Attachment #8838348 -
Flags: approval-mozilla-aurora?
Attachment #8838348 -
Flags: approval-mozilla-aurora+
Comment 24•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c52a38e6fd https://hg.mozilla.org/releases/mozilla-aurora/rev/06b5866b0bd8
https://hg.mozilla.org/mozilla-central/rev/1c52a38e6fd4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•7 years ago
|
Group: gfx-core-security → core-security-release
Comment 26•7 years ago
|
||
Reproduced on Nightly 2017-01-15, Win 10. Verified fixed Fx 53b10, 54.0a2 (2017-04-11).
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•