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)

defect
Not set
normal

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)

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.
Introduced in bug 1436791.
Flags: needinfo?(kgilbert)
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)
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'.
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
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.
Attachment #8958964 - Flags: review?(agaynor)
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: nobody → kgilbert
Blocks: 1436791
Group: core-security → gfx-core-security
Dan, can you drop the s-s; this only ever affected central and the fix is now landed. Thanks!
Flags: needinfo?(dveditz)
Group: gfx-core-security
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: