Closed
Bug 1445647
Opened 6 years ago
Closed 6 years ago
warning: array index 16 is past the end of the array in gfxVROpenVR.cpp
Categories
(Core :: WebVR, defect)
Core
WebVR
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | --- | unaffected |
firefox61 | --- | fixed |
People
(Reporter: Alex_Gaynor, Assigned: kip)
References
Details
(Keywords: csectype-bounds, regression, sec-low)
Attachments
(1 file)
1.35 KB,
patch
|
Alex_Gaynor
:
review+
|
Details | Diff | Splinter Review |
Seen when building: 4:12.36 /Users/agaynor/projects/mozilla-central/gfx/vr/gfxVROpenVR.cpp:191:5: warning: array index 16 is past the end of the array (which contains 16 elements) [-Warray-bounds] 4:12.36 state.mSittingToStandingTransform[16] = 1.0f; 4:12.36 ^ ~~ 4:12.36 /Users/agaynor/projects/mozilla-central/obj-opt-x86_64-apple-darwin17.4.0/dist/include/moz_external_vr.h:228:3: note: array 'mSittingToStandingTransform' declared here 4:12.36 float mSittingToStandingTransform[16]; 4:12.36 ^ I'm not at all familiar with this code, so apologies if this is wrong for some reason, but it looks like a fairly straightforward buffer overwrite. Looking at the code in gfxVROpenVR.cpp it appears to be writing to the array at each of the 16 indices, but skips over the '8' index and so it ends up past the end of the array by one.
Assignee | ||
Comment 2•6 years ago
|
||
Indeed, this was an inadvertent change within Bug 1436791, which involved some refactoring. We can simply remove this line. I'll prepare a patch shortly.
Flags: needinfo?(kgilbert)
Assignee | ||
Comment 3•6 years ago
|
||
After closer review, rather than deleting the line, the indexes on that line and the prior should be adjusted to not skip over the '8', and and at '15'.
Assignee | ||
Comment 4•6 years ago
|
||
I suspect that the overflowed value would have stomped onto the first VRHMDSensorState, asserted to be a POD, in the VRDisplayInfo struct. This value is later written before first use. I wouldn't expect there to be any security vulnerability as a result; however, the affected code is part of a fallback routine if the OpenVR hardware doesn't return stage bounds. If this fallback was hit, the user's position in space may be incorrect. Patch incoming shortly
Reporter | ||
Comment 5•6 years ago
|
||
Thanks, I didn't do a deep analysis for how this could be composed into an exploit :-) It's also nightly only, so we can drop the s-s as soon as it's fixed.
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Attachment #8958964 -
Flags: review?(agaynor)
Reporter | ||
Comment 7•6 years ago
|
||
Comment on attachment 8958964 [details] [diff] [review] Bug 1445647 - Fixed initialization of SittingToStandingTransform matrix for OpenVR Confirmed that this uses the same values, but now without skipping [8] and writing past the end of the buffer. I don't know nearly enough about this code to say if it's correct, but I can confirm it fixes this bug!
Attachment #8958964 -
Flags: review?(agaynor) → review+
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → kgilbert
Updated•6 years ago
|
Blocks: 1436791
Group: core-security → gfx-core-security
status-firefox59:
--- → unaffected
status-firefox60:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Comment 8•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fbbec16a7777c009bac453df3dd0e7874c30aa4 https://hg.mozilla.org/mozilla-central/rev/4fbbec16a777
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Reporter | ||
Comment 9•6 years ago
|
||
Dan, can you drop the s-s; this only ever affected central and the fix is now landed. Thanks!
Flags: needinfo?(dveditz)
Updated•6 years ago
|
Group: gfx-core-security
Flags: needinfo?(dveditz)
You need to log in
before you can comment on or make changes to this bug.
Description
•