timeless Reporter
	Description • 14 years ago

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.

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 1 • 14 years ago

Thanks a ton, the fix follows trivially from the diagnostic...

This would have been very hard to find otherwise.

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 2 • 14 years ago

refreshed

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 3 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/18c8f4b873b5

	Daniel Veditz [:dveditz]
	Comment 4 • 14 years ago

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.

	chris hofmann
	Comment 5 • 14 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 6 • 14 years ago

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.

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 7 • 14 years ago

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.

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 8 • 14 years ago

(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.