1203321 - The startup cache can be manipulated to confuse XBL bindings

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Cody Crews]

Attachment: marqueeBindingPollution-testcase-new.html

I'm attaching a testcase which reproduces this issue upon a restart.  I'm currently not sure exactly which parts of this testcase are necessary to see the problem and will be working on a reduced testcase.

To see the potential problem load the attached testcase on a clean profile, open the web console to see the XBL provided content prototype, then restart firefox again loading the testcase and opening the web console to see the XBL prototype lacking all the exposed properties that it had before.

With this testcase the marquee bindings are cached in an improper way which leaves them in a state where all fields/methods that have exposeToUntrustedContent set to true actually aren't exposed.

I'll label this sec-audit as I believe it needs more eyes on it.  If exposeToUntrustedContent can be lost for fields/methods that have it set, it would be very bad if the opposite could occur.

I've filed this in XBL as of now, but I believe this may actually be a problem with caching as I've read through most of the XBL code at this point and I don't see anything that would make this possible.

Comment 1

[Benjamin Smedberg]

Is this a testcase intended to run with content privileges or in a privileged context?

If normal web content, why is that allowed to link to chrome://browser/content/feeds/subscribe.css ?

Comment 2

[Cody Crews]

That's been possible for some time, and most of the things that could be done with it have been mitigated by the xbl attributes bindToUntrustedContent, and exposeToUntrustedContent.  These two attributes are supposed to control which bindings content can trigger and which fields/methods the binding then exposes to content.

To answer the original question, this should be ran as content.  Only stylesheets can load chrome:// URIs and they can only load certain ones even then.  As to why this is possible completely from content I'm not sure.  That could pose a different security problem if they didn't deny access to rules if the URI they load isn't one content can access, but they do.

This bug is about the fact that after viewing this testcase and restarting, the marquee bindings which are supposed to expose quite a few things to content, expose nothing.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Bobby, do you have time to look into what's going on here?

Comment 4

[Cody Crews]

I'll wait and see what Bobby says, and while waiting I'll start tearing through the caching code.

If Bobby doesn't have the time I'll take this one and figure it out.  I know it has to do with caching at this point because to fix the issue all you have to do is get rid of the startup cache.

I thought I would have this figured out by the time I read through the XBL code, but it took me a while to fully visualize how all the XBL machinery is working, and only after I finished with that did I realize that removing the startup cache fixes the issue.

Now to start reading through all the caching code, oh yeah more fun for me ;-)

Would it be more beneficial to move this to whatever section the startup cache is under seeing as whoever is over it might have an idea as to where to start looking?

Comment 5

[Boris Zbarsky [:bzbarsky]]

bsmedberg is already cced.

Comment 6

[Cody Crews]

Ah, I still don't know who owns what.  Just wanting more eyes on it, it's a very strange issue.

Comment 7

[Benjamin Smedberg]

Unless you have evidence showing otherwise, it's unlikely that this is a bug in the startup cache itself: it's a pretty straightforward id-string -> buffer keyvalue store. I'm going to assume that the XBL code is misusing the startup cache in some way causing this bug.

Comment 8

[Cody Crews]

What about xul caching?  Seems the involvement of xul elements is needed here.  I haven't had the time just yet to go back over this, but I will soon.

Comment 9

[Cody Crews]

I'm back on this as of today, hopefully I can track down the cause but right now I'm on windows and debugging on windows is like meh.

Will see what I can come up with though.

Comment 10

[Bobby Holley (:bholley)]

Sorry I haven't had the time to look at this yet - was on PTO, now I'm trying to get caught up on reviews and sort out some ServiceWorker stuff before the branch next week. Hopefully I can look at it soon.

Comment 11

[Cody Crews]

I have confirmed that setting the pref nglayout.debug.disable_xul_cache to true prevents this issue.

I believe the xul cache is to blame here, now to search through the source for any reference to nglayout.debug.disable_xul_cache.

Comment 12

[Cody Crews]

The above preference only triggers a flush of the xul cache when set.  I've chased my tail around in circles here.

Soon I'll have a *nix system setup to properly debug which how I prefer it.  Until then this one has me stumped.  I'll keep looking until Bobby can weigh in though.

Comment 13

[Bobby Holley (:bholley)]

Hey Cody,

this doesn't seem to reproduce for me on mac. I load the testcase once (in a non-e10s window), and get the following in the web console:

XBL prototype JSClass { scrollAmount: Getter, scrollDelay: Getter, trueSpeed: Getter, direction: Getter, behavior: Getter, loop: Getter, onstart: Getter, onfinish: Getter, onbounce: Getter, height: Getter, 3 more… }

I then exit the browser, and repeat the process, ending up with the same result. Any idea what's going on? Am I doing it wrong? Is this windows-specific?

Comment 14

[Cody Crews]

I am sadly on windows right now for a short time.  I did reproduce this issue first in Linux so I'm sure it's not only windows specific.  I am just this second digging through the newest nightly looking for some new coolness to target.

I'll load up the testcase with the newest nightly and see if somehow this issue has been worked out already.  That would be very nice because trying to visualize the code to figure out where this went wrong was a headache and getting me nowhere.

I'll post back the results of a nightly test probably within an hour.

Comment 15

[Cody Crews]

I can confirm that in the newest nightly this issue no longer exists.  I assume something to do with the magic of e10s prevents this.

I'm closing this out as it showed no obvious security impact the way I presented it.  As long as e10s is going to be the default here soon only those opting out of it by pref would possibly be vulnerable to anything that could come from this.

I'm marking it fixed but maybe we should still run down the changes between the current release and nightly to see if this is just e10s or if there was any change to the way xul caching works.

Comment 16

[Cody Crews]

Just saw where you said non-e10s hmm.  So yeah that should cover it completely.  I might run down the changes to see what I was missing.  Sorry I missed you saying that I was in the middle of something else extremely interesting and dropped it to check this in a hurry.

That's good enough for me.

Comment 17

[Bobby Holley (:bholley)]

Sounds good - thanks for the bug report cody!