879656 - (CVE-2013-1729) Texture in Inspector's 3D View showing parts of the OS and other applications

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Victor Porof [:vporof][:vp]]

Attachment: screenshot

STR:

1. Go to http://creativityblueprints.ro/#/home
2. Open Inspector
3. Select the HTML node
4. Open Tilt (click on the little cube on the right)

Results: The webpage mesh has a texture applied that contains parts of the desktop, my dock and the markup panel. In other scenarios, the texture only contains large of black and white.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20130603 Firefox/24.0 (today's Nightly)

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

What Mac is this on?
What graphics card is being used, according to gfxCardStatus (a small utility that you have to install)? If you force usage of one card or the other, is this problem affected?
What is the texture target (TEXTURE_2D ?) and width, height of the texture in question?

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Also, what precise mac os 10.8.x version?

Comment 3

[Victor Porof [:vporof][:vp]]

Mac:               MacBook Pro Retina, Mid 2012
Discrete graphics: NVIDIA GeForce GT 650M 1024 MB
Integrated:        Intel HD Graphics 4000 512 MB
Software:          OS X 10.8.3 (12D78)

The problem only occurs when using discrete graphics.
The problem also occurs regardless if Firefox is in low or high resolution mode.

The texture target is TEXTURE_2D, the active texture is TEXTURE0, uniform n° is 0.

Texture width and height is always guaranteed to be lower than MAX_TEXTURE_SIZE (which on my system is 16384). In this case, the generated texture size is exactly 8627x9179.

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: screenshot 2

Another screenshot, this time with the code in my Sublime Text :)

Comment 5

[Kelsey Gilbert [:jgilbert]]

Sounds familiar:
https://bugzilla.mozilla.org/show_bug.cgi?id=737182

Comment 6

[Kelsey Gilbert [:jgilbert]]

We need to bisect a known-good max size. Hopefully it's north of 8192, but we might have to pull it down to 4096 like mac+intel.

Comment 7

[Victor Porof [:vporof][:vp]]

(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> We need to bisect a known-good max size. Hopefully it's north of 8192, but
> we might have to pull it down to 4096 like mac+intel.

This bug can be reproduced only with the nvidia card, not intel.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

That's because we already hit similar issues on Intel in the past and addressed them by limiting the max texture size there. This report shows that we now need to do something similar on NVIDIA, as Jeff says.

Comment 9

[Victor Porof [:vporof][:vp]]

(In reply to Benoit Jacob [:bjacob] from comment #8)

That kinda' sucks :(

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Quick test for our assumption this is caused by the texture size

This is not the proper fix, but just to check our assumption that it is the texture size, Victor, can you try applying this patch?

Comment 11

[Victor Porof [:vporof][:vp]]

(In reply to Milan Sreckovic [:milan] from comment #10)
> Created attachment 762268 [details] [diff] [review]

This seems to do the trick.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: See if 8k limit is OK in this case.

Victor, if you don't mind, we'll try to find out what size works.  I've attached the one with 8k limit, and if that works, we can probably set the maximum to that, keeping it a power of 2, or if you can play with the values and increase it until it fails, we can see where between 16k (fails) and 4k (works from the previous patch) this actually fails. 8192 makes certain sense (16k/2), and apparently so would 11585 (16k / sqrt(2)/2).

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Of course I meant 16k / sqrt(2) :)

Comment 14

[Victor Porof [:vporof][:vp]]

(In reply to Milan Sreckovic [:milan]

Yeah, I was meaning to play with the values myself. Will post my findings asap.

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Once we have the values, we may find the fix to bug 877949 simplified.

Comment 16

[Victor Porof [:vporof][:vp]]

Um.. Guys. Guys! 8191 works like a charm, 8192 fails miserably. (╯°□°）╯︵ ┻━┻)

Also, it looks like only mMaxTextureSize had to be clamped. The renderbuffer size has nothing to do with it.

I couldn't find or create a test case in which unlimited cubemaps have any consequences.

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Victor Porof [:vp] from comment #16)
> Also, it looks like only mMaxTextureSize had to be clamped. The renderbuffer
> size has nothing to do with it.

That's only because we _happen_ to use a texture here, but I would feel uncomfortable about assuming that renderbuffers are not affected by a bug that affects textures.

> 
> I couldn't find or create a test case in which unlimited cubemaps have any
> consequences.

bug 684882

Comment 18

[Victor Porof [:vporof][:vp]]

(In reply to Benoit Jacob [:bjacob] from comment #17)

Yeah, totally. I'm not saying that we shouldn't clamp them, just posting my findings.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: 8k-1 limit on the texture sizes on OSX Nvidia combination for >= 10.8. < 10.8 already had a 4k limit from bug 877949.

Seems a bit strange to set the limit as a non-power-of-two, but it does reproduce at 8k, and works at 8k-1, so it seems a waste to kill the performance for textures between 4k and 8k.

Comment 20

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 772769 [details] [diff] [review]
8k-1 limit on the texture sizes on OSX Nvidia combination for >= 10.8.  < 10.8 already had a 4k limit from bug 877949.

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

A nit for making the code more cleanerest.

::: gfx/gl/GLContext.cpp
@@ +657,5 @@
>                  }
> +                else {
> +                    // See bug 879656.  8192 fails, 8191 works.
> +                    mMaxTextureSize = std::min(mMaxTextureSize, 8191);
> +                    mMaxRenderbufferSize   = std::min(mMaxRenderbufferSize, 8191);

You have some weird spacing here, though it looks like it's just copied from above. Please fix both while we're here.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: 8k-1 limit on the texture sizes on OSX Nvidia combination for >= 10.8. < 10.8 already had a 4k limit from bug 877949.

Carry over r=jgilbert from attachment 772769 [details] [diff] [review], just making a white space change.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Probably not easily, I don't think you can choose which part of the desktop shows up.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
23, 24.

If not all supported branches, which bug introduced the flaw?
It's been around as long as this hardware configuration.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Would be simple.

How likely is this patch to cause regressions; how much testing does it need?
The limit is still fairly high, 8k-1.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Is 22 affected by this? How about ESR 17?

Comment 23

[Kelsey Gilbert [:jgilbert]]

(In reply to Al Billings [:abillings] from comment #22)
> Is 22 affected by this? How about ESR 17?

This should affect all branches.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

Please mark status flags, not tracking flags, when marking affected versions. :-)

I'll approve for trunk. Please prepare aurora, beta, and ESR17 patches and nominate them.

Release Management, this looks like a pretty safe and contained fix.

Comment 25

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 773300 [details] [diff] [review]
8k-1 limit on the texture sizes on OSX Nvidia combination for >= 10.8.  < 10.8 already had a 4k limit from bug 877949.

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: Vulnerability to stealing screenshots of the user's desktop.
Testing completed: Preliminary, non-m-c yet
Risk to taking this patch (and alternatives if risky): Very low.
String or UUID changes made by this patch: None.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: Desktop image stealing.
Fix Landed on Version: None yet.
Risk to taking this patch (and alternatives if risky): Very low.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: Desktop image stealing.
Testing completed (on m-c, etc.): Custom builds.
Risk to taking this patch (and alternatives if risky): Very low.
String or IDL/UUID changes made by this patch: None.

Comment 26

[Kelsey Gilbert [:jgilbert]]

Well, this is basically not a problem on b2g, except if someone's running b2g on a mac. Shouldn't we wontfix this for b2g18?

Comment 27

[Kelsey Gilbert [:jgilbert]]

wontfix/unaffacted?

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Calling it "unaffected" for b2g.

Comment 29

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 773300 [details] [diff] [review]
8k-1 limit on the texture sizes on OSX Nvidia combination for >= 10.8.  < 10.8 already had a 4k limit from bug 877949.

Per discussions with Alex the other day, I'm actually going to approve this for branches.

Comment 30

[Milan Sreckovic [:milan] (needinfo for best results)]

Checkin-needed for the trunk.  I will post the patches for the other three separately.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9c559b68ead

Comment 32

[Ed Morley [:emorley]]

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

Comment 33

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bf0be86bcb77

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Is the plan to uplift bug 877949 to beta/esr17 since this bug obviously depends heavily on it?

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 773300 [details] [diff] [review]
8k-1 limit on the texture sizes on OSX Nvidia combination for >= 10.8.  < 10.8 already had a 4k limit from bug 877949.

This obviously isn't going to land on the branches as-is. Clearing the approval so it doesn't show up as needing uplift.

Comment 36

[Milan Sreckovic [:milan] (needinfo for best results)]

Let's hold off.  This fix also goes against the convention, though not explicitly the standard of GL max texture being a power of two, so we need a couple of days to sort this out.  I'll reopen it for now, and please no beta uplifts yet.

Comment 37

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Jeff Gilbert [:jgilbert] from comment #33)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/bf0be86bcb77

Jeff, would you mind backing this out of Aurora while we're sorting things out, since we removed the approval at this point?

Comment 38

[Kelsey Gilbert [:jgilbert]]

(In reply to Milan Sreckovic [:milan] from comment #37)
> (In reply to Jeff Gilbert [:jgilbert] from comment #33)
> > https://hg.mozilla.org/releases/mozilla-aurora/rev/bf0be86bcb77
> 
> Jeff, would you mind backing this out of Aurora while we're sorting things
> out, since we removed the approval at this point?

I would first like to know why we removed approval-mozilla-aurora when the patch applied cleanly, had approval, and was already marked 'fixed'.

Comment 39

[Milan Sreckovic [:milan] (needinfo for best results)]

We're in the middle of the conversation as to the appropriateness of non-power-of-two max.  We're leaning towards "it's OK", but I wanted to avoid the extra traffic if we decide that it's a "not OK".

Comment 40

[Kelsey Gilbert [:jgilbert]]

(In reply to Milan Sreckovic [:milan] from comment #39)
> We're in the middle of the conversation as to the appropriateness of
> non-power-of-two max.  We're leaning towards "it's OK", but I wanted to
> avoid the extra traffic if we decide that it's a "not OK".

The minimal traffic at this point is to only back it out in the unlikely situation that we decide it's not OK in the next few days. Uplift isn't until the 5th, so we have plenty of time. I can still do it if you feel strongly about it, but I prefer to not chance messing up a backout, or otherwise breaking aurora.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jeff Gilbert [:jgilbert] from comment #38)
> I would first like to know why we removed approval-mozilla-aurora when the
> patch applied cleanly, had approval, and was already marked 'fixed'.

Oh crap, I am so sorry. I meant to do the flag clearing on bug 807096, not this one :-(

Comment 42

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Jeff Gilbert [:jgilbert] from comment #40)
> (In reply to Milan Sreckovic [:milan] from comment #39)
> > We're in the middle of the conversation as to the appropriateness of
> > non-power-of-two max.  We're leaning towards "it's OK", but I wanted to
> > avoid the extra traffic if we decide that it's a "not OK".
> 
> The minimal traffic at this point is to only back it out in the unlikely
> situation that we decide it's not OK in the next few days. Uplift isn't
> until the 5th, so we have plenty of time. I can still do it if you feel
> strongly about it, but I prefer to not chance messing up a backout, or
> otherwise breaking aurora.

Agreed.  Let's make it official and speed up the answer - Jeff, under a superreview, are we OK setting the max size as non-power-of-two, in this case in particular?

Comment 43

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #41)
> (In reply to Jeff Gilbert [:jgilbert] from comment #38)
> > I would first like to know why we removed approval-mozilla-aurora when the
> > patch applied cleanly, had approval, and was already marked 'fixed'.
> 
> Oh crap, I am so sorry. I meant to do the flag clearing on bug 807096, not
> this one :-(

Ryan, would you mind setting those flags back on?  We reconfirmed we want this change as is.  For aurora, beta, esr17.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Only release drivers can.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

BTW, I'll note comment 34 again as blocking any uplifts to beta/esr17.

Comment 46

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #45)
> BTW, I'll note comment 34 again as blocking any uplifts to beta/esr17.

Yes, that should be clarified.  I will submit a beta/esr17 patch that is different, in that it does not need bug 877949 fixes.  I'll get a separate review, because it will be different enough.  Bug 877949 added a lower limit for <10.8, this bug added a higher limit for >=10.8. Without bug 877949, for beta and esr17, the code will just add the higher limit, but will do so for all versions.

Comment 47

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 773300 [details] [diff] [review]
8k-1 limit on the texture sizes on OSX Nvidia combination for >= 10.8.  < 10.8 already had a 4k limit from bug 877949.

[Approval Request Comment]

See comment 41 in particular - the approval got reset by mistake.

Comment 48

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Beta version of the patch

Explicit review, see comment 46.

Comment 49

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: 17ESR version of the patch

Explicit review, see comment 46.

Comment 50

[Milan Sreckovic [:milan] (needinfo for best results)]

Checkin-needed for beta and 17esr.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/35a85a4e1222
https://hg.mozilla.org/releases/mozilla-esr17/rev/e1a74c765ab3

Comment 52

[Ryan VanderMeulen [:RyanVM]]

Backed out of esr17 due to mochitest-1 failures.
https://hg.mozilla.org/releases/mozilla-esr17/rev/271fc0f0166e

https://tbpl.mozilla.org/php/getParsedLog.php?id=25450679&tree=Mozilla-Esr17

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Likewise for beta.
https://hg.mozilla.org/releases/mozilla-beta/rev/158812840d27

https://tbpl.mozilla.org/php/getParsedLog.php?id=25452712&tree=Mozilla-Beta

Comment 54

[Milan Sreckovic [:milan] (needinfo for best results)]

Sorry, my bad. This requires bug 894007, a fix to that test.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

Please don't reopen this unless it's backed out from m-c. The status flags are used to track branch uplifts and it messes with things if the resolution is wrong.

Comment 56

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Milan Sreckovic [:milan] from comment #54)
> Sorry, my bad. This requires bug 894007, a fix to that test.

According to bug 894007 comment 11, that is no longer the case - do we have something to nominate for uplift here? Our second-to-last beta is going to build tomorrow.

Comment 57

[Milan Sreckovic [:milan] (needinfo for best results)]

Sorry, sitting in all day meetings; there is a beta and 17esr patch attached to this bug, if the test failure is no longer an issue, those can be used.

Comment 58

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 777287 [details] [diff] [review]
Beta version of the patch

[Triage Comment]
Let's try landing this to beta and see if we're in the clear.

Comment 59

[Lukas Blakk [:lsblakk] use ?needinfo]

If this lands to FF23 without issues, we'll want to uplift to esr17 this week.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

Why are we expecting these to not bounce this time? I landed the branch-specific patches last time and I don't see any subsequent landings to either branch that would suggest that any test changes were made.

Comment 61

[Lukas Blakk [:lsblakk] use ?needinfo]

On closer look, without a fix for the failing test we can't take this patch as-is on Beta.  My comment 56 was about there no longer being a dependency on bug 894007, not that this patch as written would suddenly work.

Comment 62

[Milan Sreckovic [:milan] (needinfo for best results)]

This may need to ride the train.

Comment 63

[Kelsey Gilbert [:jgilbert]]

To be safe, we can just use 4k on ESR17, since ESR is basically the no-fun branch anyways.

Comment 64

[Kelsey Gilbert [:jgilbert]]

Attachment: 879656.esr17

Switch from 8191 to 4096 to stay POT.

Comment 65

[Guillaume Abadie]

But I've just got the R+ for the WebGL conformance test regression's patch that can handle the case that GLContext return a non power of two value.

Comment 66

[Kelsey Gilbert [:jgilbert]]

(In reply to Guillaume Abadie from comment #65)
> But I've just got the R+ for the WebGL conformance test regression's patch
> that can handle the case that GLContext return a non power of two value.

Oh, my bad. Don't forget to backport that bug everywhere, then.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

Given bug 894007 comment 26, is this wontfix for esr17?

Comment 68

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #67)
> Given bug 894007 comment 26, is this wontfix for esr17?

I'd say so.

Comment 69

[Daniel Veditz [:dveditz]]

lowering to moderate over-all because of limited impact, a fraction of our Mac users.

Comment 70

[u279076]

Ioana, can you please verify that this is now fixed in Firefox 24 and 25?

Comment 71

[Ioana (away)]

I tested this on various versions and got very peculiar results:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0 - 20130814063812
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20100101 Firefox/24.0 - 20130910160258
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0 - 20130917123208
  Couldn't open tilt for the page in comment 0. It either freezes Firefox until I manually close it, or the button for Tilt does nothing, except show visual feedback that I pressed it. I tried to reproduce using other web pages, but I couldn't - all the STR work well and the texture is fine.

Comment 72

[Ioana (away)]

I eventually managed to reproduce this issue on another machine with Mac OS X 10.8.4 and Firefox 23. The texture doesn't contain stuff from other apps, but it's black although it shouldn't be (one of the scenarios specified in comment 0).

Verified as fixed on that machine:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0 - 20131001024718