1364562 - Investigate potential integer overflow in dom/canvas/WebGLContextGL.cpp

Status: RESOLVED WORKSFORME

(Core :: Graphics: CanvasWebGL, defect)

Description

[Christian Holler (:decoder)]

Static analysis complained about this code in dom/canvas/WebGLContextGL.cpp:

http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/dom/canvas/WebGLContextGL.cpp#2099


On 64-bit, this is perfectly save (the malloc argument can never be large enough to overflow), but on 32-bit I'm not sure. A and B are constants from this FOO:

http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/dom/canvas/WebGLContext.h#874

with numMatsToUpload being capped at uint32_t / (A*B) by this:

http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/dom/canvas/WebGLContextValidate.cpp#317

So overall this can end up being ( (uint32_t / (A*B)) * A * B * 4 ) = uint32_t * 4 which overflows the allocation on 32-bit. Is this possible to happen and can an attacker control the parameters to make numMatsToUpload around the size of 2^30 ?

Needinfo on :jgilbert who worked on this code before.


Not setting a security rating for now because I can't really trace this back to any user inputs easily, but if this can overflow, then it would be sec-critical.

Comment 1

[Kelsey Gilbert [:jgilbert]]

The code implicitly guarantees that we never malloc a buffer larger than the source buffer, so there's no chance of overflow here. I can add an assert to help communicate this.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Do you still believe there is a security concern here?

Comment 3

[Christian Holler (:decoder)]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Do you still believe there is a security concern here?

If you say integer overflows can't happen here, then there is no concern from my side. I just filed this because it showed up on static analysis results and I couldn't figure this out myself.

Marking as WFM.