In bug 1036248 we ran into a nasty situation in which chrome code called untrusted code, the untrusted code threw, and then the chrome code had no way to determine what caused the exception because the .stack of the exception was empty. This happened because the chrome stack frames were filtered out from .stack based on the principal in effect when the exception object was constructed. How feasible would it be to have exceptions capture the unfiltered stack and then do the filtering when trying to stringify it? We'd have to change .stack to a getter and recreate the string each get (or cache, along with the compartment that requested it last, or something). We'd also need to store principals in the saved stack data structures. And we'd have the possibility of confused-delegate attacks where an exception in content is handed to chrome and the chrome code gets the .stack and hands it back to content... On the other hand, we'd have much much better debuggability. Thoughts?

If the only need here is for debugging, perhaps we could just have some global (runtime) mode that says "don't filter callstacks"? Otherwise, that sounds like a fair amount of additional complexity.

It's not just debugging via the debugger. It's callstacks dumped in our tinderbox logs on failure, for example. But even if it were just debugging, we wouldn't want to expose the unfiltered callstack to web pages while debugging them anyway.

Oh, I don't mean "debug mode" debugging, I thought we were talking about "bz is debugging FF" and I was thinking of an env var ;) TB logs is another interesting case. I wonder also if the devtools JS console et al would want to show unfiltered stacks also.

IIUC, this is what I'm doing now in |js::SavedStacks|.

(In reply to Nick Fitzgerald [:fitzgen] from comment #4) > IIUC, this is what I'm doing now in |js::SavedStacks|. "this" == saving all frames and keeping principals around to only show frames that the toString's caller should be able to see

Yes, we should definitely do this.

Note that .stack is not standardized, so we can probably get away with switching it to a getter. There are a lot of sites that depend on the _format_ of .stack though, and do all sorts of crazy regexp parsing. So we should avoid changing the web-observable format or behavior for that.

> IIUC, this is what I'm doing now in |js::SavedStacks|. Yes, indeed. So one simple way of implementing this would be to just have Error objects saveCurrentStack and store the result, stringifying it as required. Again, possibly with memoization of the string to help performance. This is basically what I'm going to do in DOMException in bug 857648. So we should perhaps make sure that lands and sticks (and hence that the various SavedStacks issues that uncovers are fixed) before switching Error to SavedStacks. > So we should avoid changing the web-observable format or behavior for that. SavedFrame::toStringMethod produces output in the same exact format as js::ComputeStackString, precisely for that reason.

Actually, I don't think SavedStacks does what we want here, unless I'm totally missing something about how it works. In particular, if I call saveCurrentStack() while in some compartment, I get back a SavedFrame from that compartment. Then when I call SavedFrame::toStringMethod I better be in that same compartment, and that's the principal that will be used for filtering. Basically, we need Xrays for SavedFrame to make this work. And then changing the code bug 857648 to work on the passed-in JSContext instead of just entering the compartment of the saved stack.

(In reply to Boris Zbarsky [:bz] from comment #9) > Actually, I don't think SavedStacks does what we want here, unless I'm > totally missing something about how it works. In particular, if I call > saveCurrentStack() while in some compartment, I get back a SavedFrame from > that compartment. Then when I call SavedFrame::toStringMethod I better be > in that same compartment, and that's the principal that will be used for > filtering. > > Basically, we need Xrays for SavedFrame to make this work. And then > changing the code bug 857648 to work on the passed-in JSContext instead of > just entering the compartment of the saved stack. I'm not sure I follow 100%... Are js/src/jit-test/tests/saved-stacks/principals-{01.js,02.js} incorrect or not enough?

What those tests do is create several globals with different principals, and in each one call saveStack() and then immediately examine the result (frame by frame in -01.js and via toString in -02.js). In particular, the saveStack() call and the examination of the return value happen with the same principal. There's even a comment in -01.js about how we have to inject "extract" into each global to avoid cross-compartment wrappers. What this bug is about is that even if the saveStack() happens in an unprivileged context, if the stack examination is done in a privileged one the entire stack should be visible. That doesn't work right now, as you can see by applying this patch and then running the test.

Note to self: Jason would prefer a resolve hook, not an accessor property, for .stack on Error.

I filed bug 1038238 on converting Error over to SavedFrame, which is somewhat independent of this bug, though a prereq for it.

Comment on attachment 8455429 [details] [diff] [review] Somewhat of a start Argh, wrong bug. :(

<bholley> bz: so, yes, I think we need: <bholley> (1) a getter for Error.stack that first grabs the base SavedFrame, and then (generically, from the caller's compartment) builds the stack string <bholley> (2) Xrays for SavedFrame <bholley> (3) An implementation of the parentProperty getter that doesn't go through CallNonGenericMethod <bholley> (i.e. goes through CheckedUnwrap) <bholley> oh, and everything _else_ needs to be rejiggered to go over CallNonGenericMethod <bholley> because right now these aren't cross-compartment-ready <bholley> ("everything else" as in "the other accessors on SavedFrame") <bholley> bz: does that all sound sensible? <bz> bholley: yeah, seems plausible <bz> bholley: so in DOMException we'd wrap the stack into caller compartment <bz> bholley: possibly producing an Xray <bz> bholley: and then ToString() it? <bz> bholley: which will delegate to the Xrayed toString, etc <bholley> bz: yep <bholley> bz: well, wrap the _exception_ <bholley> bz: since my understanding is that the useful stringification for the SavedStack happens in the .stack getter on Error <bholley> bz: right? <bz> bholley: we have a direct SavedFrame pointer in DOMException <bz> bholley: no Error <bz> bholley: so no <bholley> bz: hm, that's kind of a problem then <bz> bholley: why? <bz> bholley: I mean, DOMException can do whatever Error is doing <bholley> bz: yes, but it just means that the code has to be duplicated <bholley> bz: the ultimate goal is for DOMException to inherit Error.prototype's toString right? <bz> This isn't about .toString <bz> it's about .stack <bholley> bz: yes I know. but toString is also relevant <bz> And Error.prototype.stack is "" right now <bz> Fair <bholley> bz: so I guess we just need to give both Error and DOM .stack getters <bz> Right <bz> DOM has one <bholley> bz: that presumably do the same logic on a possibly-cross-compartment SavedFrame <bz> yep <bz> Taht's what I'm thinkin <bholley> bz: so the stringification lives in a jsfriendapi.h function <bz> You mean will live <bholley> yes <bholley> bz: so who's going to do it? <bz> Ideally fitzgen <bz> from my pov <bz> We just need to give him enough info to do it. <bholley> sounds good. I'll paste this chat into the bug Nick, is this something you're able to work on? Boris and I can sit down with you and go over whichever pieces need extra explanation.

I could do this eventually, but it depends how fast we want it done. I'm just becoming unblocked on building a memory panel for the devtools, and shipping that is my number one goal at this point in time.

This isn't super-urgent yet. Certainly not until after bug 1038238 happens. We've sort of lived with this issue for a while, so a bit more won't kill us.

Ok, I'll take the bug then. So this is my understanding of what's needed in this bug: * Make an xray for SavedFrame * Make the SavedFrame accessors (or a second set of accessors?) use CheckedUnwrap * Add a jsfriendapi method for stringifying stacks Please correct me if I'm wrong on anything. If you have pointers to code I can crib from, more details, etc that would be very appreciated as well :)

I realized that implementing Xrays to SavedFrame would actually be kind of a pain, because the current Xray machinery is all based off of JSProtoKeys (keys for standard classes), which we don't have for SavedFrame. We could maybe add one, but it seems kind of like trying to point a square peg into a round hole. So how about: * Move the principal-filtering logic from parentProperty into getParent, and make getParent take an explicit principal to use for filtering. * Make the jsfriendapi function for stringifying stacks take a SavedFrame*, and a principal to use for the filtering. * Make Error.stack into a getter that is aware of the fact that |this| might be a proxy. Instead of operating over CallNonGenericMethod, it saves the current principal, manually unwraps and enters the compartment of the SavedFrame, and operates on it (using the jsfriendapi function for stringifying stacks), with the saved principal. Does that all make sense? Thoughts Boris?

> Make the jsfriendapi function for stringifying stacks take a SavedFrame* I don't believe we expose SavedFrame* outside the JS engine. We could, I guess, but why is that needed here? The callee could just verify that it's a SavedFrame* we have. > * Make Error.stack into a getter that is aware of the fact that |this| might be a proxy. You mean mStack might be a proxy, right? This plan sounds fine to me assuming the JS folks are willing to make Error.stack an accessor property.

(In reply to Boris Zbarsky [:bz] from comment #21) > > Make the jsfriendapi function for stringifying stacks take a SavedFrame* > > I don't believe we expose SavedFrame* outside the JS engine. We could, I > guess, but why is that needed here? Because you said above: <bz> bholley: we have a direct SavedFrame pointer in DOMException Did I misunderstand you somehow? > > * Make Error.stack into a getter that is aware of the fact that |this| might be a proxy. > > You mean mStack might be a proxy, right? No, |this|. mStack would always be same-compartment with the error. We just need to make the .stack getter handle a cross-compartment |this| without using CallNonGenericMethod. Once we do that, Xrays-to-Error will handle the rest.

We have a JSObject*. SavedFrame inherits from JSObject, but the public API for working with these things is based on JSObject*. So far. Note bug 1031152. > No, |this|. Oh, as in "this" might be an Xray to the actual Error? That makes sense.

(In reply to Boris Zbarsky [:bz] from comment #23) > We have a JSObject*. SavedFrame inherits from JSObject, but the public API > for working with these things is based on JSObject*. So far. Note bug > 1031152. Makes sense. > > No, |this|. > > Oh, as in "this" might be an Xray to the actual Error? That makes sense. Exactly. Xrays basically work by JSFunction-izing the JSNatives in the Xray scope, and auditing all of them to make sure that they're either (a) generic or (b) handle wrappers. We usually do (b) via CallNonGenericMethod (which invokes the nativeCall trap, effectively forwarding the call across the compartment boundary), but that won't work here - so we'll need to do something manual.

(In reply to Nick Fitzgerald [:fitzgen] from comment #19) > Ok, I'll take the bug then. > > So this is my understanding of what's needed in this bug: > > * Make an xray for SavedFrame > * Make the SavedFrame accessors (or a second set of accessors?) use > CheckedUnwrap > * Add a jsfriendapi method for stringifying stacks This has all been done in bug 1117242 and bug 1031152. What is still needed here?

Well, fixing bug 1038238, right?

And I think we are done here now that bug 1038232 is fixed.

(In reply to Nick Fitzgerald [:fitzgen] from comment #27) > And I think we are done here now that bug 1038232 is fixed. Err bug 1038238