2D texture corruption on Mac/Intel with large texture sizes >= 4993

VERIFIED FIXED in Firefox 12

Status

()

Core
Graphics
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: hillzy, Assigned: bjacob)

Tracking

unspecified
mozilla14
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox12 verified, firefox13 verified, firefox14 verified, firefox-esr1012+ verified)

Details

(Whiteboard: [sg:vector-high], URL)

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 607300 [details]
Image corruption image

When I go to: http://www.dannijo.com/product_info.php?products_id=766 and click on the image to view details, check the file I attached to see what happens:

Hardware Info:
11" Macbook Air - Mid 2011
Graphics  Intel HD Graphics 3000 384 MB
Software  Mac OS X Lion 10.7.3 (11D50b)

Running Firefox 11.0
STR: Click on the image

I can confirm the bug by running on my integrated gpu, not discrete:
15-inch, Early 2011
Intel Inc. -- Intel HD Graphics 3000 OpenGL Engine -- 2.1 APPLE-7.14.5

Vague regression range (between firefox 9 and 10):
http://hg.mozilla.org/releases/mozilla-release/rev/72ad46d416ce is affected
http://hg.mozilla.org/releases/mozilla-release/rev/c4405d7a95f6 is not
This is a really big image -

(gdb) p iwidth
$1 = 5455
(gdb) p iheight
$2 = 5455

While the Intel card apparently allows 8192, this makes me suspicious....
Assignee: nobody → joe
(Assignee)

Comment 3

5 years ago
Our experience in bug 684882 may be worth keeping in mind here (intel/mac driver too optimistic with its max tex size)
It's not a strict image size problem; resizing the image to 4097x4097 lets it work, as does 4500x4500, but 5000x5000 doesn't.
Given that this is not a simple problem, I'm going to bow out of this in favour of working on higher-priority items.
Assignee: joe → nobody
Created attachment 607633 [details]
testcase

This is a simple save-as of the website, and reproduces the bug on load.
Group: core-security
Whiteboard: [sg:high]
(Assignee)

Comment 7

5 years ago
Thanks for hiding this bug. uninitialized-video-memory-exposed bugs with a reproducible testcase are definitely sg:high.
(Assignee)

Comment 8

5 years ago
Here is a testcase using WebGL, showing this to be a driver bug with huge 2D textures. It uses a 5455x5455 texture. This was directly adapted from the cube map testcase from bug 684882.

http://people.mozilla.org/~bjacob/webgltexture2D5k.html

STR:
1. use gfxCardStatus to force usage of the Intel GPU
2. go to above testcase

Confirmed to reproduce in both Firefox and Chrome.

Probable solution: like in bug 684882, limit texture size on Mac/Intel. Here, 4096 seems like the natural choice, unless someone can reproduce the bug at sizes <= 4096. By that occasion, we should probably move the existing cube map workaround from WebGL implementation into GLContext as well.
(Assignee)

Comment 9

5 years ago
Created attachment 607753 [details]
small testcase using WebGL
(Assignee)

Updated

5 years ago
Assignee: nobody → bjacob
(Assignee)

Updated

5 years ago
Summary: Image corruption → 2D texture corruption on Mac/Intel with large texture sizes >= 5k
Created attachment 607893 [details]
near-minimal testcase using only canvas2D

Here's a more or less minimal testcase, without WebGL, which means in particular that the Mac will naturally stay on the integrated Intel GPU i.e. no need for gfxCardStatus anymore to reproduce.

public URL:
http://people.mozilla.org/~bjacob/canvas2D5k.html

STR:
1. use Mac OSX 10.6+ (so we do accelerated layers) with Intel graphics, make sure that the integrated Intel GPU is in use.
2. click above link

Confirmed to reproduce in Firefox.
Created attachment 607909 [details] [diff] [review]
limit max tex size on mac/intel

This should fix it, assuming that 2D texture sizes <= 4096 are safe.

I wasn't sure what was the best way to simulate a ERROR_INVALID_VALUE. In WebGLContext we supplant the native GL error system, but that's not the case in GLContext outside of debug mode at the moment. So I thought the simplest was to change the width and height to -1. Indeed, GLsizei is signed, negative values explicitly generate INVALID_VALUE, and even if they were interpreted as a huge positive value (in case of a driver bug), that would still be INVALID_VALUE (larger than max value).

I've been wondering how safe it is to let this workaround depend on the VENDOR string, given Mac GPU switching. I think it's quite safe as the GLContext is the LayerManager's, which is created when the Window is created, which is earlier than when content can force a GPU switch (by creating a WebGL context). One possible attack though would go as follows:
 1. browser starts, main window using GL layers on Intel GPU
 2. create a WebGL context -> switch all to discrete GPU
 3. create a new window. This step probably requires some social engineering i.e. I don't think that content can create new windows entirely by itself.
 4. New window's GLContext is created on discrete GPU, hence it doesn't have the texture size restriction.
 5. drop reference to WebGL context object and canvas
 6. wait for GC to happen (about 20 seconds)
 7. When the WebGL context is GC'd, the Mac switches back to Intel GPU
 8. in the second browser window created at step 3, create a very large 2d canvas as in the testcase here.

Should we worry about such a convoluted attack path?
Attachment #607909 - Flags: review?(joe)
Created attachment 607910 [details] [diff] [review]
limit max tex size on mac/intel

Let's initialize these max-size variables in the ctor to avoid any headache if the initialization code setting them is ever not called.
Attachment #607909 - Attachment is obsolete: true
Attachment #607909 - Flags: review?(joe)
Created attachment 607912 [details]
drop WebGL cube map workaround

Once the above GLContext patch lands, we won't need the WebGL-specific tweak anymore.

Keeping this a separate patch because I want to only ask for aurora/beta approval for the GLContext patch, not for this one.
Attachment #607912 - Flags: review?(jmuizelaar)
(Assignee)

Updated

5 years ago
Attachment #607910 - Flags: review?(joe)
(In reply to Benoit Girard (:BenWa) from comment #1)
> Vague regression range (between firefox 9 and 10):
> http://hg.mozilla.org/releases/mozilla-release/rev/72ad46d416ce is affected
> http://hg.mozilla.org/releases/mozilla-release/rev/c4405d7a95f6 is not

This could be explained by the fact that we used to have a bug whereby we stayed on the discrete GPU all the time (see bug 646043) and eventually fixed it.
We bisected the minimum texture size reproducing this on Etienne's Mac. The smallest size reproducing this is 4993.
Summary: 2D texture corruption on Mac/Intel with large texture sizes >= 5k → 2D texture corruption on Mac/Intel with large texture sizes >= 4993
(In reply to Benoit Jacob [:bjacob] from comment #14)
> (In reply to Benoit Girard (:BenWa) from comment #1)
> > Vague regression range (between firefox 9 and 10):
> > http://hg.mozilla.org/releases/mozilla-release/rev/72ad46d416ce is affected
> > http://hg.mozilla.org/releases/mozilla-release/rev/c4405d7a95f6 is not
> 
> This could be explained by the fact that we used to have a bug whereby we
> stayed on the discrete GPU all the time (see bug 646043) and eventually
> fixed it.

That's true, I didn't pay attention to that.
Comment on attachment 607910 [details] [diff] [review]
limit max tex size on mac/intel

Review of attachment 607910 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContext.h
@@ +1691,5 @@
> +        // max texture size that they report. So we check ourselves against our own values
> +        // (mMax[CubeMap]TextureSize) which we tweak as needed.
> +        GLsizei maxSize = target == LOCAL_GL_TEXTURE_2D
> +                          ? mMaxTextureSize
> +                          : mMaxCubeMapTextureSize;

Why do we want to use MaxCubeMapTextureSize for non GL_TEXTURE_2D target? I know for example on mobile we want to use the external image target and I don't think it has any relationship with CubeMaps.

@@ +2399,5 @@
>          AFTER_GL_CALL;
>      }
>  
>      void fTexImage2D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLint border, GLenum format, GLenum type, const GLvoid *pixels) {
> +        HandleIllegalTextureSizes(target, &width, &height);

This will return w=-1,h=-1. Do we know for sure that glTexImage2D and the likes will handle this? I don't like relaying on drivers for error checking. If so I'd like to see a comment explaining why we know it's safe.
(In reply to Benoit Girard (:BenWa) from comment #17)
> Comment on attachment 607910 [details] [diff] [review]
> limit max tex size on mac/intel
> 
> Review of attachment 607910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContext.h
> @@ +1691,5 @@
> > +        // max texture size that they report. So we check ourselves against our own values
> > +        // (mMax[CubeMap]TextureSize) which we tweak as needed.
> > +        GLsizei maxSize = target == LOCAL_GL_TEXTURE_2D
> > +                          ? mMaxTextureSize
> > +                          : mMaxCubeMapTextureSize;
> 
> Why do we want to use MaxCubeMapTextureSize for non GL_TEXTURE_2D target? I
> know for example on mobile we want to use the external image target and I
> don't think it has any relationship with CubeMaps.

Oh. I didn't know of other texture targets than 2D and Cube Maps. Are you referring to some extension that we are using somewhere? Link needed.

> 
> @@ +2399,5 @@
> >          AFTER_GL_CALL;
> >      }
> >  
> >      void fTexImage2D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLint border, GLenum format, GLenum type, const GLvoid *pixels) {
> > +        HandleIllegalTextureSizes(target, &width, &height);
> 
> This will return w=-1,h=-1. Do we know for sure that glTexImage2D and the
> likes will handle this? I don't like relaying on drivers for error checking.
> If so I'd like to see a comment explaining why we know it's safe.

True. I was just thinking that we've never seen a driver bug whereby texImage2D would fail to generate INVALID_VALUE on -1 sizes, but OK. So, if we don't want to rely on the driver to generate INVALID_VALUE for us, we have to implement our own GL error system, like we do in the WebGL impl. The patch is going to be a lot more intrusive. Do you prefer that still?
Alternative approach: to further increase the likelihood that the GL impl will generate INVALID_VALUE on our texImage2D call, we could set level to -1.
I was thinking of just having an early return (We could use a macro maybe). I know that we could use GL_TEXTURE_RECTANGLE, the other target isn't used for glTexImage2D but is used for others calls like glBind.

http://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image_external.txt

I don't want to implement our own GL error system. If your sure we will get INVALID_VALUE everywhere just add a comment to explain that we rely on this behavior.
If we just early-return without somehow generating INVALID_VALUE, we break the GL spec and we'll be scratching our heads some time down the road about strange bugs ("why is this texture not correctly uploaded?").

I'd much rather keep GLContext compliant with the GL spec as much as possible.
Whiteboard: [sg:high] → [sg:vector-high]
I don't understand why this would be only vector-high and not high. This bug directly, by itself, allows an attacker to read random video memory.
Is it vector-high only because this is a bug in the Mac/Intel driver that's exposed by Firefox, not a bug in Firefox itself?
Comment on attachment 607910 [details] [diff] [review]
limit max tex size on mac/intel

Review of attachment 607910 [details] [diff] [review]:
-----------------------------------------------------------------

I take Benoit's review comments and adopt them as my own!
Attachment #607910 - Flags: review?(joe) → review-
Comment on attachment 607912 [details]
drop WebGL cube map workaround

Dropping review until the issues with the other patch are sorted out.
Attachment #607912 - Flags: review?(jmuizelaar)
Created attachment 609188 [details] [diff] [review]
limit max tex size on mac/intel

How about this approach instead. The helper becomes IsTextureSizeLegalForTarget() returning bool; it checks for cube map targets and assumes that anything else behaves like TEXTURE_2D, which is probably safer; and in case of an illegal size, it now also passes -1 for |level| and for |border|, maximizing chances to get an INVALID_VALUE (there are now 4 different invalid values passed to these GL functions, so the GL would have to be really wrong to fail to generate INVALID_VALUE). Also, |null| is passed for the texture data pointer so that in the worst case, we'd be derefing null instead of an arbitrary pointer.

If you don't like that, here are the other possibilities, choose one:
 1. we use a different GL function to artificially generate a INVALID_VALUE. Downside: APItraces will look very, very mysterious ("wtf does glCopyTexImage2D call glSomeUnrelatedFunc??")
 2. we implement our own GL error system cooperating with the native GL error system. There is all the code for that in the WebGL impl. Downside: more intrusive change, won't get beta approval.
 3. we fail silently without generating an error. Downside: we break the GL spec, will cause headaches.
Attachment #607910 - Attachment is obsolete: true
Attachment #609188 - Flags: review?(bgirard)
Comment on attachment 609188 [details] [diff] [review]
limit max tex size on mac/intel

Review of attachment 609188 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, minot comment nit.

::: gfx/gl/GLContext.cpp
@@ +446,5 @@
> +        if (mVendor == VendorIntel) {
> +            // see bug 737182 for 2D textures, bug 684822 for cube map textures.
> +            mMaxTextureSize        = NS_MIN(mMaxTextureSize,        4096);
> +            mMaxCubeMapTextureSize = NS_MIN(mMaxCubeMapTextureSize, 512);
> +            // for good measure, we align renderbuffers on what we do for 2D textures

The grammar seems off to me. How about:
For good measure we do the same for renderbuffers.

::: gfx/gl/GLContext.h
@@ +2405,5 @@
>      }
>  
>      void fTexImage2D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLint border, GLenum format, GLenum type, const GLvoid *pixels) {
>          BEFORE_GL_CALL;
> +        if (IsTextureSizeLegalForTarget(target, width, height)) {

If we cared about performance we could add the check only for Intel by setting fTexImage2D to a wrapper function. But we shouldn't be calling these often.
Attachment #609188 - Flags: review?(bgirard) → review+
Regarding the performance comment: maybe we can rename IsTextureSizeLegalForTarget to something slightly different and have it do the platform check.

I am not worried about texImage2D which is very expensive anyway, but it is indeed not great to add overhead to copyTexImage2D as a matter of principle (in practice, this is still negligible overhead).
Created attachment 609343 [details] [diff] [review]
limit max tex size on mac/intel

How about this, for performance?
Attachment #609188 - Attachment is obsolete: true
Attachment #609343 - Flags: review?(bgirard)
Comment on attachment 609343 [details] [diff] [review]
limit max tex size on mac/intel

LGTM
Attachment #609343 - Flags: review?(bgirard) → review+
(Assignee)

Updated

5 years ago
Attachment #607912 - Flags: review?(jmuizelaar)
http://hg.mozilla.org/integration/mozilla-inbound/rev/7474b9e08e0d
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/7474b9e08e0d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox14: --- → fixed
Resolution: --- → FIXED
Attachment #607912 - Flags: review?(jmuizelaar) → review+
Comment on attachment 609343 [details] [diff] [review]
limit max tex size on mac/intel

[Approval Request Comment]
Regression caused by (bug #): ever since we enabled GL layers and WebGL on Mac (pre-Firefox 4)
User impact if declined: video memory leakage (information exposure) vulnerability on Mac. Exploitable just by creating a 2d canvas of size 5kx5k, or an image of that size, or a WebGL texture of that size.
Testing completed (on m-c, etc.): Been on m-c for a few days
Risk to taking this patch (and alternatives if risky): seems low risk. At worst  (if we were crazy unlucky) it could expose a different driver bug.
String changes made by this patch: none
Attachment #609343 - Flags: approval-mozilla-beta?
Attachment #609343 - Flags: approval-mozilla-aurora?
Comment on attachment 609343 [details] [diff] [review]
limit max tex size on mac/intel

[Triage Comment]
We'll be on the lookout for driver regressions after this lands - approving for Aurora 13 and Beta 12.
Attachment #609343 - Flags: approval-mozilla-beta?
Attachment #609343 - Flags: approval-mozilla-beta+
Attachment #609343 - Flags: approval-mozilla-aurora?
Attachment #609343 - Flags: approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/3674e711982a
http://hg.mozilla.org/releases/mozilla-beta/rev/f80a1c880b7a
http://hg.mozilla.org/integration/mozilla-inbound/rev/bd9c99b055ab
(In reply to Benoit Jacob [:bjacob] from comment #36)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/bd9c99b055ab

https://hg.mozilla.org/mozilla-central/rev/bd9c99b055ab

Updated

5 years ago
status-firefox-esr10: --- → affected
tracking-firefox-esr10: --- → 12+
[Triage Comment]
Now that there's been some bake time on aurora/beta please go ahead and nominate for ESR landing as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
(Assignee)

Updated

5 years ago
Attachment #609343 - Flags: approval-mozilla-esr10?
Attachment #609343 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
status-firefox12: --- → fixed
status-firefox13: --- → fixed
http://hg.mozilla.org/releases/mozilla-esr10/rev/d35ae8519966
status-firefox-esr10: affected → fixed
Whiteboard: [sg:vector-high] → [sg:vector-high][qa+]

Comment 40

5 years ago
Does this issue have a CVE id?
Notice that this bug is completely Mac-specific (seeing your redhat email address ;-) )
Verified with Fx 12 beta6
Status: RESOLVED → VERIFIED

Updated

5 years ago
status-firefox12: fixed → verified
Whiteboard: [sg:vector-high][qa+] → [sg:vector-high]
Can we unhide this bug now? It's fixed in Release and in ESR.
What's the status of the underlying driver bug? Will we be exposing users of other applications?
See my reply in bug 684882 comment 92 as this is very similar.

Yes, other applications are affected, all the more since this bug is about 2D textures which are used by all the applications that use OpenGL to accelerate any kind of graphics. However, this has been known publicly for a while as a google search suggests:

https://www.google.ca/search?q=mac+texture+corruption

The discussions returned by this search don't have as specific information as we discussed in this bug though, but I don't think that our conclusion here, "2D texture uploads are garbage above a certain critical size that is roughly 5000", is particularly hard to arrive to.
Tracy, can you please take a moment to verify this fixed with Firefox 14 and the latest ESR nightly? Thanks.
Group: core-security

Updated

5 years ago
status-firefox-esr10: fixed → verified
status-firefox13: fixed → verified
status-firefox14: fixed → verified
You need to log in before you can comment on or make changes to this bug.