This one is a really complex issue and it's me so I'm really going to try to condense this down. https://mxr.mozilla.org/mozilla-central/source/dom/xml/nsXMLPrettyPrinter.cpp#114 You can start reading there to begin to understand this. Just get a general idea of what is happening as it's not terribly important except for one key detail. That is the fact that it is from c++ code that the prettyprint bindings for xml data are loaded. This involves importing two style sheets into the document, both of which don't show up through document.childNodes, but references to them are accessible through document.styleSheets. These references get the normal native anonymous content protection goodness, but that can be bypassed with access to the XBL execution context. I'm using pieces of my older work from bug 1045034 for that bypass, but I'm fairly close I believe to having another method of achieving the same thing. Now to the interesting parts. Once you have access to these references one who looks will notice that the prototypes accessible from the ownerNode of the style sheet are prototypes which provide access to native methods. Here I'm tracing the __proto__ chain up to the Object prototype and using the native __lookupSetter__ method to bypass restrictions on windows that I shouldn't have access to. Using this over powered __lookupSetter__ function I then get the location setter of a window that is inaccessible to me otherwise. With this location setter it's possible to bypass all restriction when setting the location property of any window except those that are chrome privileged. In the testcase I'm attaching with this I use this location setter on a pdf viewer window to inject javascript into it via a javascript URI. I then have it load a local file because resource:// pages have that ability and still using the same OP location setter I show javascript execution in that window to prove that in fact the contents of the file loaded are fully accessible from content. There, and that's as condensed down as possible for an issue this complicated I believe(for me). I still have to mention that it's also possible to manipulate about:home windows to achieve chrome privilege escalation but without a way to inject script into it, I thought this was more interesting. I hope I didn't kill you guys with all that, and also let me know if this works in windows ok. you OSX guys will just have to trust the word of others.

> I then get the location setter of a window that is inaccessible to me otherwise Can you not just do: Object.getOwnPropertyDescriptor(i2.contentWindow, "location").set to get the same function that you're getting via __lookupSetter__ in the attached testcase?

(In reply to Boris Zbarsky [:bz] from comment #1) > > I then get the location setter of a window that is inaccessible to me otherwise > > Can you not just do: > > Object.getOwnPropertyDescriptor(i2.contentWindow, "location").set > > to get the same function that you're getting via __lookupSetter__ in the > attached testcase? Then the location setter shouldn't allow javascript URIs. It would only be the location setter as can be obtained from the XBL context which AFAIK doesn't allow a javascript URI in a cross domain situation(this allows for XSS even). The prototypes available here from the NAC do not match the prototypes from i.contentWindow or i.contentWindow.wrappedJSObject. I guess you could try that, but I wouldn't expect it to work. What made me even think to try this was that the prototype of the ownerNode in this instance doesn't match i.contentWindow.HTMLLinkElement.prototype or the wrappedJSObject equivalent. The only thing that is true about the ownerNode(from script) is that it is an instanceof i.contentWindow.HTMLLinkElement. Who knows though maybe I way over thought this one.

Ok, bz is right here. I'm fairly sure I tried XSS with access to the XBL context in the past with no success, so I'm guessing this is a regression. I'll dig into it and see what I can come up with on that. It's still very interesting to me that the prototypes accessible from NAC aren't what they're expected to be. Can anyone explain that?

Confirming on Win7 (had to reload to get the popup showing my hosts file). On the error console I get 08:55:50.695 unsafe CPOW usage1 PdfjsChromeUtils.jsm:344:0 08:55:50.696 unsafe CPOW usage1 tabbrowser.xml:542:0 (2) 08:55:50.696 Error: operation not possible on dead CPOW1 tabbrowser.xml:542:0

Cody, this is top-notch work - super clever, 3-part exploit (built on an earlier bug that we haven't fixed, though are working hard to get fixed). Al, I recommend a generous bounty on this one. This is exactly the kind of exploit I expect to see at pwn2own 2016 unless we assign someone to fix up our loading permission story. IIUC, cody starts by using bug 1045034 to get access to both the playPreview scope _and_ its XBL scope running with an nsExpandedPrincipal. Cody, you mentioned above that you might have another way to do that part without using playPreview. Is that the case? if so, please file it as a separate bug and NI me. Next, he uses the playPreview scope to load a file:// URI in an iframe. Cody, is this also part of bug 1045034, or something different? Boris, any estimation of the compat risk of preventing resource:// URIs from loading file:// ? Finally, he exploits a bug in DocShell in which, while trying to avoid inheriting an nsEP from the caller [1], we end up inheriting the principal of the docshell's active document for the triggering principal [2]. This means that, when the nsJSThunk runs async, the security check ends up passing [3]. This is a regression from bug 821809, I think, though the previous state (inheriting the nsEP into a DOMWindow) probably wasn't much better. Boris, what do you think? Should we just mint an nsNullPrincipal at [1] in the nsEP case? it seems like we should basically never be inheriting the principal in the javascript:// uri case, but I'm not sure how to enforce that. [1] https://hg.mozilla.org/mozilla-central/annotate/f5e3bacfb60e/docshell/base/nsDocShell.cpp#l1603 [2] https://hg.mozilla.org/mozilla-central/annotate/f5e3bacfb60e/docshell/base/nsDocShell.cpp#l9740 [3] https://hg.mozilla.org/mozilla-central/annotate/cad68cc74656/dom/jsurl/nsJSProtocolHandler.cpp#l267

I've reduced cody's testcase a bit using SpecialPowers to replace the dependency on bug 1045034. This demonstrates the third part of the exploit, which is the new part discussed above.

(In reply to Bobby Holley (:bholley) from comment #5) > Next, he uses the playPreview scope to load a file:// URI in an iframe. > Cody, is this also part of bug 1045034, or something different? I have to admit here I kind of fell uphill on this one. I only thought to try this because as I said the prototypes involved from the prettyprint bindings aren't what they're expected to be. with styleSheet being a reference to the style sheet here from the XBL scope it should be like this: styleSheet.ownerNode.__proto__ === i.contentWindow.HTMLLinkElement.prototype and to access the prototype as the content of i.contentWindow one should use i.contentWindow.wrappedJSObject.HTMLLinkElement.prototype. I'm sure you can get what I'm saying from that, the only strange part is neither prototype is actually the prototype of the ownerNode. This led me to have the idea that by using the Object prototype accessible via the __proto__ chain, that it may allow one to bypass security restrictions. (In reply to Bobby Holley (:bholley) from comment #5) > Cody, you mentioned above that you might have another way to do that part without using playPreview. > Is that the case? if so, please file it as a separate bug and NI me. I've been very close to this by using the marquee bindings. I've also ran into a most interesting bug that I'll try to get a testcase to show off from script in which by placing certain elements that generate NAC as a child of a marquee you permanently poison a profile. With this bad profile all the methods/fields that have exposeToUntrustedContent set to true, aren't actually accessible from content anymore. It throws no errors and doesn't crash anything therefore is extremely hard to debug. I have concerns the above mentioned bug could possibly work in reverse and have anonymous content influence the way NAC works, so when I get a testcase that reproduces the issue I'll mark it security sensitive. I'll also continue the work I was doing to use the marquee bindings to compromise the XBL scope and follow up as needed.

Thinking about this a little more I may be able to answer Bobby's questions a little more straight forward. I already knew that resource:// URIs could load file:// URIs as I also knew that before pwn2own 2015 that resource:// URIs could load chrome:// URIs. I remember discussing it with Daniel or Al at some point on IRC though they might not remember. The only magic here is the location setter obtained allows javascript URI's in windows that it shouldn't. It also makes me want to meet Mariusz Mlynski even more than after pwn2own 2014 because I swear the guy is living in my head but seems better able to apply the knowledge floating around in there ;) Jokes aside I absolutely have to set down and have a coffee or something with him because we seem to think scarily close in terms of security research. If I actually planned on attending pwn2own this year my focus for the work was going to be the fact that resource:// URIs could load chrome:// URIs and whatever I could build on that.

> any estimation of the compat risk of preventing resource:// URIs from loading file:// No, sadly. Seems pretty risky to me, but we could at least try it on nightly... > while trying to avoid inheriting an nsEP from the caller [1], we end up inheriting the > principal of the docshell's active document We did that more or less on purpose, right? But an alternative here would in fact be to not inherit at all in this case. That would end up not running javascript:, I expect and give data: a nullprincipal when it loads.

Here's just something to look at along the lines of not inheriting the owner. I'm reading up on the proper way to do this using mercurial and hg or even MozReview. This probably needs to be redone anyway because this is wasting a call to IsExpandedPrincipal. I'm thinking two different conditionals representing each call individually instead of the call to IsSystemOrExpanedPrincipal. If this is totally way off or if I'm stepping on anyone's toes just ignore it.

Here is a more proper patch generated with hg. This also doesn't waste a call to IsExpandedPrincipal. Ignore the parent listed as I used the 39 branch source to test this patch as I noticed that accessing anonymous content from bug 1045034 has been stopped in the newest nightly. I can easily generate the same patch from the newest nightly if needed. Which would be a bigger performance hit here, a wasted call to IsExpandedPrincipal, or breaking the whole thing down into two separate conditional statements as I have? This only takes care of the problem of inheriting the owner in the case of nsExpandedPrincipal which pertains to data, and javascript URIs plus the special use cases of about:blank. I'll wait to hear from those more familiar with this for the other part needed. Also bz I hope you don't mind me NI'ing you here.

Fixes a stupid mistake in the comments explaining this change.

Not sure what the needinfo is for, exactly. Are you looking for feedback on the patch? Review? Something else?

I should have directed the question below directly to you I guess. I was looking more for feedback, maybe a review once I properly generate the patch from the newest nightly. I thought you were probably the authority on how this should be handled. I can attempt the second patch to stop resource:// URIs from loading file:// URIs too if need be. I'm just trying to be helpful. (In reply to Cody Crews from comment #11) > Which would be a bigger performance hit here, a wasted call to > IsExpandedPrincipal, or breaking the whole thing down into two separate > conditional statements as I have?

Ah. Two conditional statements, with "else" so the second one doesn't even run if the first one tests true, is better.

Can I get some feedback on this when you see it bz? This patch is generated from changes made to the newest nightly, so possibly a review? I'm going to start looking into the source to see where the resource:// protocol is handled and where changes need to be made to prevent loading file:// URIs. Unless you guys have a better idea that is. Just let me know.

Comment on attachment 8631840 [details] [diff] [review] bug-1178058-part1.patch Review of attachment 8631840 [details] [diff] [review]: ----------------------------------------------------------------- This patch prevents inheriting the owner in the case of nsExpandedPrincipal in nsDocShell::LoadURI as the first part fix of bug 1178058.

I wasn't sure which of you guys should look this over. I believe that this part is solid in terms of accomplishing what we want here. Flagging for review is probably the only productive thing I'll get done today =/

Starting tomorrow I *should* be gone for approximately a week. I never did get to part two, but part one here is a fairly minimal change. Just handle things as if I was here anyway(like previously before I was getting more involved). I'll try to be in touch some during this coming week but don't expect much. I'm sure you guys have it handled either way. Thanks in advance for the hard work guys.

I just wanted to clarify while I had a minute today. I'm currently in the process of moving which is why I am going to have limited access to keep in touch with you guys. I know I move a lot but in the great state of North Carolina it's almost impossible to find a place that's actually everything it's supposed to be throughout a lease. I really wanted to get to part two of this as I felt it would be more challenging than part 1 by far. Should I need info bz to get the review rolling? I'll just do that anyway as I need to start packing things and such. I still have the net right now but wont after this move starts, so also if anything on the payout front happens just tell Chris to push it through to the most current account information on file for me. I can't be sure that I'll even have the technicians out after this move in time to be back but I've prearranged it so that it should happen in the amount of time I've listed. When this is done I should be as active as I have been and hopefully more so as time builds.

Can I get a review on this bz? I flagged bholley on this as well as he was a suggested person but I know you're probably the one that really needs to look at it. If you can after you look it over if nothing is needed(it's a very minimal change) then just make sure Bobby isn't bothered by it too. Also just commit the changes as yourself if needed. Just as long as it gets handled. I'll try to keep an eye on things but I may go ahead and leave ASAP because my new neighbors are driving me crazy ;-)

Cody, no need to needinfo me separately. Especially over the weekend, when I'm not going to be reviewing anyway. ;) I do have a list of review requests waiting for me when I get back on Monday, and it usually takes a bit to get through it all.

Comment on attachment 8631840 [details] [diff] [review] bug-1178058-part1.patch So I thought about this a bit more and I think what we should really do is what Bobby suggested in comment 5: treat the expanded principal case just like we treat the LOAD_FLAGS_DISALLOW_INHERIT_OWNER case. I'll post a patch to that effect in a moment...

(In reply to Boris Zbarsky [:bz] from comment #22) > Cody, no need to needinfo me separately. Especially over the weekend, when > I'm not going to be reviewing anyway. ;) I do have a list of review > requests waiting for me when I get back on Monday, and it usually takes a > bit to get through it all. Yeah sorry about that, I've never had the honor/burden of being flagged for review so I didn't know it would double up your nag emails :P I'm uploading what should be a viable start to part 2 of the fix for this and I guess I'll just flag Bobby for review, but I would definitely like your feedback too.

This patch just checks the sourceScheme in nsScriptSecurityManager::CheckLoadURIWithPrincipal to see if it matches resource and bails if trying to load a file:// URI. Once again ignore the parent listed.

Just thought I would say the only time I can get to anything during this week is late, it's currently 11:48 PM EST here and now I must sleep! The above patch should be a good start, I'm sure we may want to allow for special use cases. I'm also not sure what "But watch out for the view-source stylesheet?" in the comments means. Help me anywhere I'm missing anything here guys. Also if there's activity on this bug and there's no activity from me after 10 PM EST after that then maybe the next day.

Comment on attachment 8633707 [details] [diff] [review] Don't convert an expanded principal into inheriting the current principal. Just pretend like the null principal did the load (like LOAD_FLAGS_DISALLOW_INHERIT_OWNER does) Review of attachment 8633707 [details] [diff] [review]: ----------------------------------------------------------------- Do you think we could also use a null principal in the chrome case? Or maybe just for javascript: URIs? r=me either way. Sorry for the lag.

Comment on attachment 8633878 [details] [diff] [review] bug-1178058-part2.patch Review of attachment 8633878 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I think we should tackle this in a separate bug. I've filed bug 1184387, can you attach it there, and do a try push? ::: caps/nsScriptSecurityManager.cpp @@ +815,5 @@ > nsIProtocolHandler::URI_IS_UI_RESOURCE, > &sourceIsChrome); > NS_ENSURE_SUCCESS(rv, rv); > if (sourceIsChrome) { > + nsresult rv = sourceBaseURI->GetScheme(sourceScheme); Don't we get the source scheme already above? why do we need to get it again? @@ +817,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > if (sourceIsChrome) { > + nsresult rv = sourceBaseURI->GetScheme(sourceScheme); > + if (NS_FAILED(rv)) return rv; > + // If the sourceScheme is resource deny load to Nit: "resource://", and remove the trailing whitespace.

> Do you think we could also use a null principal in the chrome case? Wouldn't that break bookmarklets? Or does the chrome pass in an owner for those? If it wouldn't break bookmarklets, I'm willing to at least try it.

(In reply to Boris Zbarsky [:bz] from comment #30) > > Do you think we could also use a null principal in the chrome case? > > Wouldn't that break bookmarklets? Or does the chrome pass in an owner for > those? I'm not sure - Gijs, do you know?

(In reply to Bobby Holley (:bholley) from comment #31) > (In reply to Boris Zbarsky [:bz] from comment #30) > > > Do you think we could also use a null principal in the chrome case? > > > > Wouldn't that break bookmarklets? Or does the chrome pass in an owner for > > those? > > I'm not sure - Gijs, do you know? Not offhand, I can look either late tonight or (my tz) tomorrow morning.

I've been looking over writing tests for changes such as this, and I think I have it figured out except one issue here. The attempt to execute javascript in another window fails silently. The tests I've looked over involve catching errors to confirm that things are handled properly usually. Any suggestions? I'll be digging deeper into making tests until I hear something.

(In reply to Bobby Holley (:bholley) from comment #31) > (In reply to Boris Zbarsky [:bz] from comment #30) > > > Do you think we could also use a null principal in the chrome case? > > > > Wouldn't that break bookmarklets? Or does the chrome pass in an owner for > > those? > > I'm not sure - Gijs, do you know? So... I'm not sure how complete an answer this is. But clicking a bookmarklet link on the bookmarks toolbar just ends up calling browser (like XBL browser) .loadURI with the js URL and flags being 0 (notably, we don't pass LOAD_FLAGS_DISALLOW_INHERIT_OWNER). I'll go down the rabbithole a bit further and see what I can dig up.

(In reply to Cody Crews (if gone back by 07/21) from comment #34) > I've been looking over writing tests for changes such as this, and I think I > have it figured out except one issue here. The attempt to execute > javascript in another window fails silently. The tests I've looked over > involve catching errors to confirm that things are handled properly usually. > > Any suggestions? I'll be digging deeper into making tests until I hear > something. It's hard to say much about this without knowing how you're writing a test - mochitest? mochitest-browser? Something else? It might make sense to attach your WIP.

(In reply to :Gijs Kruitbosch (away Jul 18 - Aug 2) from comment #35) > (In reply to Bobby Holley (:bholley) from comment #31) > > (In reply to Boris Zbarsky [:bz] from comment #30) > > > > Do you think we could also use a null principal in the chrome case? > > > > > > Wouldn't that break bookmarklets? Or does the chrome pass in an owner for > > > those? > > > > I'm not sure - Gijs, do you know? > > So... I'm not sure how complete an answer this is. But clicking a > bookmarklet link on the bookmarks toolbar just ends up calling browser (like > XBL browser) .loadURI with the js URL and flags being 0 (notably, we don't > pass LOAD_FLAGS_DISALLOW_INHERIT_OWNER). > > I'll go down the rabbithole a bit further and see what I can dig up. Meh. So we call loadURI, and we pass no owner, so we hit this block: if (!owner && !inheritOwner && !ownerIsExplicit) { // See if there's system or chrome JS code running inheritOwner = nsContentUtils::IsCallerChrome(); } and inheritOwner is set to true. I presume this, at least, answers the question? If not, please re-needinfo (but be quick, because I'm leaving tonight and won't be back until August). If you need someone after my 5pm today, maybe ask :ttaubert or someone else on the frontend team.

Yes, that answers the question. So using nullprincipal in the "is system principal" case with explicitly given owner would actually not break bookmarlets, I guess.

(In reply to Cody Crews (if gone back by 07/21) from comment #34) > I've been looking over writing tests for changes such as this, and I think I > have it figured out except one issue here. The attempt to execute > javascript in another window fails silently. The tests I've looked over > involve catching errors to confirm that things are handled properly usually. Hey Cody, Yeah, this is indeed a problem - that's why I had to use setTimeout in my test (see the one attached to this bug).

Comment on attachment 8629241 [details] [diff] [review] Tests. v1 This should get reviewed so that we can check it in later on. Note that I'm building on it in bug 1185015.

(In reply to :Gijs Kruitbosch (away until Aug 3) from comment #36) > (In reply to Cody Crews (if gone back by 07/21) from comment #34) > > I've been looking over writing tests for changes such as this, and I think I > > have it figured out except one issue here. The attempt to execute > > javascript in another window fails silently. The tests I've looked over > > involve catching errors to confirm that things are handled properly usually. > > > > Any suggestions? I'll be digging deeper into making tests until I hear > > something. > > It's hard to say much about this without knowing how you're writing a test - > mochitest? mochitest-browser? Something else? > > It might make sense to attach your WIP. Bobby actually answered this in comment 40. I should have looked over his test more in depth.

Comment on attachment 8629241 [details] [diff] [review] Tests. v1 s|javascript://|javascript:| r=me

Tracked for all the current train releases.

Comment on attachment 8633707 [details] [diff] [review] Don't convert an expanded principal into inheriting the current principal. Just pretend like the null principal did the load (like LOAD_FLAGS_DISALLOW_INHERIT_OWNER does) Approval Request Comment [Feature/regressing bug #]: bug 821809, sorta [User impact if declined]: security bugs [Describe test coverage new/current, TreeHerder]: Automated test in this bug. Not checked in. [Risks and why]: Medium-low risk. It could theoretically break addons, but it's very doubtful. [String/UUID change made/needed]: None

Sounds like this may also affect 39. Is that the case? If so do we need a chemspill for 39?

Assuming also this may affect esr38.

Bobby, in the uplift request risk assessment you mentioned that this could break add-ons. Did you get a chance to test the fix locally with any add-ons installed? This will give me a bit of confidence when approving uplift to Aurora. Thanks!

Bobby, can you also request sec-approval? Thanks.

Comment on attachment 8633707 [details] [diff] [review] Don't convert an expanded principal into inheriting the current principal. Just pretend like the null principal did the load (like LOAD_FLAGS_DISALLOW_INHERIT_OWNER does) [Security approval request comment] How easily could an exploit be constructed based on the patch? Not easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Not really. Which older supported branches are affected by this flaw? All of them. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? They should be identical. How likely is this patch to cause regressions; how much testing does it need? See comment 45.

(In reply to Ritu Kothari (:ritu) from comment #48) > Bobby, in the uplift request risk assessment you mentioned that this could > break add-ons. Did you get a chance to test the fix locally with any add-ons > installed? This will give me a bit of confidence when approving uplift to > Aurora. Thanks! This fix has been in Nightly for almost six weeks now, so presumably we'd have heard about addon bustage from any popular addons. I'm really not worried about addon breakage here. The addon would have to have been written very recently, and basically be explicitly depending on this security flaw.

Comment on attachment 8633707 [details] [diff] [review] Don't convert an expanded principal into inheriting the current principal. Just pretend like the null principal did the load (like LOAD_FLAGS_DISALLOW_INHERIT_OWNER does) Approving for ESR38 and Aurora uplift. This patch has been in Nightly for 3 weeks, seems stable.

Bobby, FF40 RC will be built off of mozilla-release branch. Can you please also request uplift to m-r?

Comment on attachment 8633707 [details] [diff] [review] Don't convert an expanded principal into inheriting the current principal. Just pretend like the null principal did the load (like LOAD_FLAGS_DISALLOW_INHERIT_OWNER does) Beta+, Release+. This patch has been in Nightly for > two weeks so it seems stable. The fix was also verified which is great: https://bugzilla.mozilla.org/show_bug.cgi?id=1191284#c16

I think we shouldn't land this until we know if we are doing a chemspill to 39/ESR branches. I should know soon.

esr38 (default): http://hg.mozilla.org/releases/mozilla-esr38/rev/65c071b3116e esr38 (GECKO3810esr_2015062417_RELBRANCH): http://hg.mozilla.org/releases/mozilla-esr38/rev/4bd38a453c6e release (default): http://hg.mozilla.org/releases/mozilla-release/rev/f147014ff61a release (GECKO390_2015063018_RELBRANCH): http://hg.mozilla.org/releases/mozilla-release/rev/3769169d9bf8 beta: http://hg.mozilla.org/releases/mozilla-beta/rev/f147014ff61a aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/5c3a30a76af9

We're hitting a compile error on mozilla-release, on the relbranch (39.0 based) but not on default (40.0), so something changed between in a cycle. The error is: In file included from /builds/slave/rel-m-rel-l64_bld-000000000000/build/obj-firefox/docshell/base/Unified_cpp_docshell_base0.cpp:56:0: /builds/slave/rel-m-rel-l64_bld-000000000000/build/docshell/base/nsDocShell.cpp: In member function 'virtual nsresult nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, uint32_t, bool)': /builds/slave/rel-m-rel-l64_bld-000000000000/build/docshell/base/nsDocShell.cpp:1610:15: error: 'Create' is not a member of 'nsNullPrincipal' Full log: http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/39.0.3-candidates/build1/logs/release-mozilla-release-linux64_build-bm73-build1-build1.txt.gz

Replace the added line: owner = nsNullPrincipal::Create(); with owner = do_CreateInstance("@mozilla.org/nullprincipal;1"); NS_ENSURE_TRUE(owner, NS_ERROR_FAILURE); Bed time for me.

For what it's worth I cannot reproduce this testcase on ESR-31. In the dev-tools console I get Error: stream must have data" pdf.worker.js:215 assert@resource://pdf.js/build/pdf.worker.js:232:5 init@resource://pdf.js/build/pdf.worker.js:4615:5 PDFDocument@resource://pdf.js/build/pdf.worker.js:4606:7 LocalPdfManager@resource://pdf.js/build/pdf.worker.js:4201:5 getPdfManager@resource://pdf.js/build/pdf.worker.js:36831:11 wphSetupDoc@resource://pdf.js/build/pdf.worker.js:36997:7 messageHandlerComObjOnMessage@resource://pdf.js/build/pdf.worker.js:1115:9 I don't know if the problem can be worked around or if that version of Firefox and/or the PDF reader is missing a crucial feature, but at least the 0-day won't work as written.

r=dveditz on the "patch" in comment 58

Landed the fix as esr38 (default): http://hg.mozilla.org/releases/mozilla-esr38/rev/010cae6de47b esr38 (GECKO3810esr_2015062417_RELBRANCH): http://hg.mozilla.org/releases/mozilla-esr38/rev/63989e580394 release (GECKO390_2015063018_RELBRANCH): http://hg.mozilla.org/releases/mozilla-release/rev/5a41909266e5

According to Comment 45 and Comment 50, the fix pushed for this issue is already covered by automated testing. Bobby, is manual testing also required for this patch? If that is the case, could you please elaborate a bit on how Manual QA can help out here?

(In reply to Andrei Vaida, QA [:avaida] from comment #62) > Bobby, is manual testing also required for this patch? If that is the case, > could you please elaborate a bit on how Manual QA can help out here? There is a test for this ("Tests. v1"), but it was not landed. So somebody needs to run those tests.

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3a02cd6e6ef7 https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/3a02cd6e6ef7 https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/0301111193bb

Yes, verify the mochitest in this patch, as well as cody's original testcase attached here. Note that the testcase is designed to work on Windows and Linux. To make it work for Mac, you'll need to add an appropriate file path for the testcase to display.

Dunno if this adds value, but i can confirm i've tested 38.1.0esr and 39.0 on OpenBSD/amd64, they were affected by a slightly adapted version of the testcase in attachment #8626955 [details] reading /etc/passwd, and 38.1.1esr, 39.0.3 & 40.0rc2 were all immune to the bug.

(In reply to Landry Breuil (:gaston) from comment #66) > Dunno if this adds value, but i can confirm i've tested 38.1.0esr and 39.0 > on OpenBSD/amd64, they were affected by a slightly adapted version of the > testcase in attachment #8626955 [details] reading /etc/passwd, and > 38.1.1esr, 39.0.3 & 40.0rc2 were all immune to the bug. Thank is very helpful, thank you. :-)

(In reply to Cody Crews from comment #7) > … I'll also > continue the work I was doing to use the marquee bindings to compromise the > XBL scope and follow up as needed. Does this mean we could reduce attack surface by re-implementing those HTML elements which are currently implemented using XBL with something possibly safer (e.g. HTML/CSS)? Not an expert here, just wondering :-)

(In reply to Frederik Braun [:freddyb] (on PTO until Sep 4th) from comment #68) > (In reply to Cody Crews from comment #7) > > … I'll also > > continue the work I was doing to use the marquee bindings to compromise the > > XBL scope and follow up as needed. > > Does this mean we could reduce attack surface by re-implementing those HTML > elements which are currently implemented using XBL with something possibly > safer (e.g. HTML/CSS)? Not an expert here, just wondering :-) Those technologies aren't really analogous - WebComponents would be the web-y equivalent to XBL, and I'm not sure we've mapped out the steps to ship WebComponent bindings with the browser. In general, the point of the XBL scope is to hoist in-content XBL code to a place where the content can't see it. In general compromising the XBL scope is not itself a security issue (because it runs with a limited principal), but this bug demonstrates how it can be combined with other exploits.

Sorry that I don't know the process, but: now we've shipped this, should this bug be opened? Gerv

Given the heavy discussion of it, I want to keep this closed until bug 1184387 lands everywhere.