Closed Bug 617687 Opened 9 years ago Closed 9 years ago

WebGLFramebuffer::InitializeRenderbuffers stack arrays overrun

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: timeless, Assigned: bjacob)

References

(Blocks 1 open bug)

Details

(Keywords: coverity)

Attachments

(1 file, 1 obsolete file)

gfx/thebes/GLContext.h
line 70 -- typedef char realGLboolean;
gfx/thebes/GLDefs.h
line 53 -- typedef char realGLboolean;

1424 class WebGLFramebuffer :
1429 {
...
1599 protected:
1601     // protected because WebGLContext should only call InitializeRenderbuffers
1602     void InitializeRenderbuffers()
1603     {
...
1615         realGLboolean savedColorMask[] = {0}, savedDepthMask = 0;
1616         GLuint savedStencilMask = 0;
1617         GLfloat savedColorClearValue[] = {0.f}, savedDepthClearValue = 0.f;
1618         GLint savedStencilClearValue = 0;
1619         GLuint clearBits = 0;
...
1656         if (initializeColorBuffer) {
1657             mContext->gl->fColorMask(savedColorMask[0],
1658                                      savedColorMask[1],
1659                                      savedColorMask[2],
1660                                      savedColorMask[3]);
1661             mContext->gl->fClearColor(savedColorClearValue[0],
1662                                       savedColorClearValue[1],
1663                                       savedColorClearValue[2],
1664                                       savedColorClearValue[3]);
1665             mColorAttachment.Renderbuffer()->SetInitialized(PR_TRUE);
1666         }

Coverity thinks that savedColorMask[] = {0} is an array (of char) of size 1
But the code is trying to access elements 0, 1, 2 and 3, the last 3 of which aren't part of it.
Attached patch patch (obsolete) — Splinter Review
Thanks a ton, the fix follows trivially from the diagnostic...

This would have been very hard to find otherwise.
Assignee: nobody → bjacob
Status: NEW → ASSIGNED
Attachment #496239 - Flags: review?
Attachment #496239 - Flags: review? → review?(timeless)
Attached patch patchSplinter Review
refreshed
Attachment #496239 - Attachment is obsolete: true
Attachment #496241 - Flags: review?
Attachment #496239 - Flags: review?(timeless)
Attachment #496241 - Flags: review? → review?(timeless)
Attachment #496241 - Flags: review?(timeless) → review+
Attachment #496241 - Flags: approval2.0+
Summary: WebGLFramebuffer::InitializeRenderbuffers has an unusual savedColorMask array → WebGLFramebuffer::InitializeRenderbuffers stack arrays overrun
http://hg.mozilla.org/mozilla-central/rev/18c8f4b873b5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Is there really a security problem here? Looks like you'll just be reading unexpected (but predictable?) values off the stack but at worst fColorMask and fClearColor will be initialized to odd values.
Benoit, or maybe Timeless,  can you do some digging to answer the question in comment 5?  we need that to figure out if this is eligible for a bug bounty.
No one answered, but the openGL GetBooleanv() call will write to its second *params argument. I guess that will stomp the stack, anyone's guess whether the extra 3 bytes end up anywhere critical.
We had:

        realGLboolean savedColorMask[/*4*/] = {0};
        realGLboolean savedDepthMask = 0;
        GLuint savedStencilMask = 0;
        GLfloat savedColorClearValue[/*4*/] = {0.f};
        GLfloat savedDepthClearValue = 0.f;
        GLint savedStencilClearValue = 0;
        GLuint clearBits = 0;

        // ...

        mContext->gl->fGetBooleanv(LOCAL_GL_COLOR_WRITEMASK,
                                   savedColorMask);
        mContext->gl->fGetFloatv(LOCAL_GL_COLOR_CLEAR_VALUE,
                                 savedColorClearValue);

Each of these 2 calls was writing 4 elements when only 1 was allocated on the stack.

*If* the compiler allocated the above stack variables contiguously in the order of declaration, then this only resulted in them being overwritten, thus having wrong (even scriptable) values. That in itself is at most a DOS issue (these values are then 'restored' causing the GL context used for WebGL to have bad state, resulting in a broken WebGL display)

*However*, if the compiler allocated the above stack variables in a different order, or interleaved them with other stack variables, then potentially this bug could cause this function to behave arbitrarily badly. Although that wasn't really scriptable in itself, that would have been a potential security issue. Indeed, the big problem here is to ensure that WebGL buffers are initialized by zeros, so that WebGL doesn't allow scripts to read back arbitrary video memory. If this function doesn't behave as intended, then we can't guarantee that anymore, which means leaking whatever is in video memory to malicious scripts. That would be a security issue.

In conclusion, this is a very intricate issue, it's potentially a security issue depending on how the compiler allocates stack variables, in any case it was extremely helpful to receive this bug report.
(In reply to comment #7)
> No one answered, but the openGL GetBooleanv() call will write to its second
> *params argument. I guess that will stomp the stack, anyone's guess whether the
> extra 3 bytes end up anywhere critical.

And the GetFloatv() call writes 16 bytes, so that's 12 more bytes of potential scribbling.
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.