The default bug view has changed. See this FAQ.
Bug 879656 (CVE-2013-1729)

Texture in Inspector's 3D View showing parts of the OS and other applications

VERIFIED FIXED in Firefox 24

Status

()

Core
Canvas: WebGL
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: vporof, Assigned: milan)

Tracking

({csectype-disclosure, sec-moderate, sec-vector})

unspecified
mozilla25
x86
Mac OS X
csectype-disclosure, sec-moderate, sec-vector
Points:
---

Firefox Tracking Flags

(firefox22 wontfix, firefox23+ wontfix, firefox24+ fixed, firefox25+ verified, firefox-esr1724+ wontfix, b2g18 unaffected)

Details

(Whiteboard: [adv-main24+])

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 758407 [details]
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)
Keywords: sec-high
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?
Also, what precise mac os 10.8.x version?
(Reporter)

Comment 3

4 years ago
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.
(Reporter)

Comment 4

4 years ago
Created attachment 758518 [details]
screenshot 2

Another screenshot, this time with the code in my Sublime Text :)
Sounds familiar:
https://bugzilla.mozilla.org/show_bug.cgi?id=737182
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.
(Reporter)

Comment 7

4 years ago
(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.
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.
(Reporter)

Comment 9

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

That kinda' sucks :(
(Assignee)

Comment 10

4 years ago
Created attachment 762268 [details] [diff] [review]
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?
Flags: needinfo?(vporof)
(Reporter)

Comment 11

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

This seems to do the trick.
Flags: needinfo?(vporof)
(Assignee)

Comment 12

4 years ago
Created attachment 762816 [details] [diff] [review]
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).
Flags: needinfo?(vporof)
(Assignee)

Comment 13

4 years ago
Of course I meant 16k / sqrt(2) :)
(Reporter)

Comment 14

4 years ago
(In reply to Milan Sreckovic [:milan]

Yeah, I was meaning to play with the values myself. Will post my findings asap.
Flags: needinfo?(vporof)
(Assignee)

Comment 15

4 years ago
Once we have the values, we may find the fix to bug 877949 simplified.
(Reporter)

Comment 16

4 years ago
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.
(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
(Reporter)

Comment 18

4 years ago
(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.
(Assignee)

Comment 19

4 years ago
Created 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.

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.
Attachment #762268 - Attachment is obsolete: true
Attachment #762816 - Attachment is obsolete: true
Attachment #772769 - Flags: review?(jgilbert)
(Assignee)

Updated

4 years ago
Assignee: nobody → milan
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.
Attachment #772769 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 21

4 years ago
Created 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.

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.
Attachment #772769 - Attachment is obsolete: true
Attachment #773300 - Flags: sec-approval?
Attachment #773300 - Flags: review+
Is 22 affected by this? How about ESR 17?
(In reply to Al Billings [:abillings] from comment #22)
> Is 22 affected by this? How about ESR 17?

This should affect all branches.
tracking-b2g18: --- → ?
tracking-firefox22: --- → ?
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?
tracking-firefox-esr17: --- → ?
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.
status-b2g18: --- → affected
status-firefox22: --- → wontfix
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox25: --- → affected
status-firefox-esr17: --- → affected
tracking-firefox22: ? → ---
tracking-firefox23: ? → +
tracking-firefox24: ? → +
tracking-firefox25: ? → +
tracking-firefox-esr17: ? → -
Attachment #773300 - Flags: sec-approval? → sec-approval+
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.
Attachment #773300 - Flags: approval-mozilla-esr17?
Attachment #773300 - Flags: approval-mozilla-beta?
Attachment #773300 - Flags: approval-mozilla-b2g18?
Attachment #773300 - Flags: approval-mozilla-aurora?
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?
wontfix/unaffacted?
Calling it "unaffected" for b2g.
status-b2g18: affected → unaffected
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.
Attachment #773300 - Flags: approval-mozilla-esr17?
Attachment #773300 - Flags: approval-mozilla-esr17+
Attachment #773300 - Flags: approval-mozilla-beta?
Attachment #773300 - Flags: approval-mozilla-beta+
Attachment #773300 - Flags: approval-mozilla-b2g18?
Attachment #773300 - Flags: approval-mozilla-aurora?
Attachment #773300 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 30

4 years ago
Checkin-needed for the trunk.  I will post the patches for the other three separately.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9c559b68ead
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c9c559b68ead
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox25: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Updated

4 years ago
Depends on: 894007
https://hg.mozilla.org/releases/mozilla-aurora/rev/bf0be86bcb77
status-firefox24: affected → fixed
Is the plan to uplift bug 877949 to beta/esr17 since this bug obviously depends heavily on it?
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.
Attachment #773300 - Flags: approval-mozilla-esr17+
Attachment #773300 - Flags: approval-mozilla-beta+
Attachment #773300 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 36

4 years ago
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.
(Assignee)

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 37

4 years ago
(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?
Flags: needinfo?(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'.
Flags: needinfo?(jgilbert) → needinfo?(ryanvm)
(Assignee)

Comment 39

4 years ago
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".
(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.
(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 :-(
Flags: needinfo?(ryanvm)
(Assignee)

Updated

4 years ago
Attachment #773300 - Flags: superreview?(jmuizelaar)
(Assignee)

Comment 42

4 years ago
(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?
Attachment #773300 - Flags: superreview?(jmuizelaar) → superreview+
(Assignee)

Comment 43

4 years ago
(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.
Only release drivers can.
BTW, I'll note comment 34 again as blocking any uplifts to beta/esr17.
(Assignee)

Comment 46

4 years ago
(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.
(Assignee)

Comment 47

4 years ago
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.
Attachment #773300 - Flags: approval-mozilla-esr17?
Attachment #773300 - Flags: approval-mozilla-beta?
Attachment #773300 - Flags: approval-mozilla-aurora?
Attachment #773300 - Flags: approval-mozilla-esr17?
Attachment #773300 - Flags: approval-mozilla-esr17+
Attachment #773300 - Flags: approval-mozilla-beta?
Attachment #773300 - Flags: approval-mozilla-beta+
Attachment #773300 - Flags: approval-mozilla-aurora?
Attachment #773300 - Flags: approval-mozilla-aurora+
tracking-b2g18: ? → ---
tracking-firefox-esr17: - → 23+
(Assignee)

Comment 48

4 years ago
Created attachment 777287 [details] [diff] [review]
Beta version of the patch

Explicit review, see comment 46.
Attachment #777287 - Flags: review?(jgilbert)
(Assignee)

Comment 49

4 years ago
Created attachment 777288 [details] [diff] [review]
17ESR version of the patch

Explicit review, see comment 46.
Attachment #777288 - Flags: review?(jgilbert)
Attachment #777287 - Flags: review?(jgilbert) → review+
Attachment #777288 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 50

4 years ago
Checkin-needed for beta and 17esr.
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-beta/rev/35a85a4e1222
https://hg.mozilla.org/releases/mozilla-esr17/rev/e1a74c765ab3
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
status-firefox23: affected → fixed
status-firefox-esr17: affected → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
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
status-firefox-esr17: fixed → affected
Likewise for beta.
https://hg.mozilla.org/releases/mozilla-beta/rev/158812840d27

https://tbpl.mozilla.org/php/getParsedLog.php?id=25452712&tree=Mozilla-Beta
status-firefox23: fixed → affected
(Assignee)

Comment 54

4 years ago
Sorry, my bad. This requires bug 894007, a fix to that test.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED

Updated

4 years ago
tracking-firefox-esr17: 23+ → 24+
(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.
Flags: needinfo?(milan)
(Assignee)

Comment 57

4 years ago
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.
Flags: needinfo?(milan)
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.
Attachment #777287 - Flags: approval-mozilla-beta+
Keywords: checkin-needed
If this lands to FF23 without issues, we'll want to uplift to esr17 this week.
tracking-firefox-esr17: 24+ → ?
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.
Attachment #777287 - Flags: approval-mozilla-beta+
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.
status-firefox23: affected → wontfix
tracking-firefox-esr17: ? → 24+
Keywords: checkin-needed
(Assignee)

Comment 62

4 years ago
This may need to ride the train.
To be safe, we can just use 4k on ESR17, since ESR is basically the no-fun branch anyways.
Created attachment 782888 [details] [diff] [review]
879656.esr17

Switch from 8191 to 4096 to stay POT.
Attachment #777288 - Attachment is obsolete: true
Attachment #782888 - Flags: review?(milan)

Comment 65

4 years ago
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.
(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.
Attachment #777288 - Attachment is obsolete: false
Attachment #782888 - Attachment is obsolete: true
Attachment #782888 - Flags: review?(milan)
Given bug 894007 comment 26, is this wontfix for esr17?
(Assignee)

Comment 68

4 years ago
(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.
status-firefox-esr17: affected → wontfix
Whiteboard: [adv-main24+]
Alias: CVE-2013-1729
Keywords: csec-disclosure, sec-vector
lowering to moderate over-all because of limited impact, a fraction of our Mac users.
Keywords: sec-high → sec-moderate
Ioana, can you please verify that this is now fixed in Firefox 24 and 25?
Keywords: verifyme

Comment 71

4 years ago
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

4 years ago
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
Status: RESOLVED → VERIFIED
status-firefox25: fixed → verified
Keywords: verifyme
Group: core-security
You need to log in before you can comment on or make changes to this bug.