Closed
Bug 754202
Opened 12 years ago
Closed 12 years ago
Pull principals directly off the context/object compartment
Categories
(Core :: Security: CAPS, defect)
Core
Security: CAPS
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: bholley, Assigned: bholley)
References
(Blocks 1 open bug)
Details
Attachments
(11 files, 3 obsolete files)
1.67 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
6.21 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
3.53 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
25.30 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
7.10 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
5.08 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
1.27 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
1.20 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
942 bytes,
patch
|
mrbkap
:
review+
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
1.09 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
9.10 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
This is one of the big things that CPG enables, but we still have to actually do it. This will involve ripping out a lot of crappy code. :-)
Assignee | ||
Updated•12 years ago
|
Summary: Pull principals directly off the context/caller compartment → Pull principals directly off the context/object compartment
Assignee | ||
Comment 1•12 years ago
|
||
IonMonkey will break gecko security without this.
Blocks: IonMonkey
Assignee | ||
Comment 2•12 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=83c4e102d93d
Comment 3•12 years ago
|
||
Make old_doGetObjectPrincipal DEBUG-only, please?
Assignee | ||
Comment 4•12 years ago
|
||
This isn't something we actually support, and were using enablePrivilege to do it before. When we switch this to SpecialPowers wrapping, the sender origin becomes a chrome URL (not very interesting) and the source window becomes null (which we do explicitly in nsGlobalWindow for chrome callers). So those tests stop being useful.
Attachment #625948 -
Flags: review?(mrbkap)
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #625950 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 6•12 years ago
|
||
It would be nice to check these principals against the principals acquired using the old mechanism. Unfortunately, they often differ. Because CAPS uses JS stack frames, any time we enter a compartment and do an operation (even throwing an Access-Denied exception) without running any JS code, we'll end up with a different principal. Our security story is pretty darn tied to compartments at this point, so let's just pull the trigger.
Attachment #625951 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•12 years ago
|
||
Comment on attachment 625948 [details] [diff] [review] Part 1 - Fix content->chrome postMessage mochitest. v1 I decided midstream to flag bz for these reviews, since I've already got a lot of things in Blake's review queue. Blake already signed off on the idea here.
Attachment #625948 -
Flags: review?(mrbkap) → review?(bzbarsky)
Comment 8•12 years ago
|
||
Comment on attachment 625948 [details] [diff] [review] Part 1 - Fix content->chrome postMessage mochitest. v1 >+++ b/dom/tests/mochitest/whatwg/test_postMessage_chrome.html >- is(evt.origin, "http://example.org", "content response event has wrong URI"); >- is(evt.source, window.frames.contentDomain, >- "wrong source for same-domain message!"); Uh... why?
Comment 9•12 years ago
|
||
Comment on attachment 625950 [details] [diff] [review] Part 2 - Pull object principals directly off the compartment, and assert that behavior doesn't change. v2 So a question. Do the callers of doGetObjectPrincipal ever have cross-compartment proxies in obj? And if so, do they want the principals of the proxy or of the underlying object? I guess the former, since the proxy will do its own security checks? If so, r=me. Looking forward to all that code (including the code for the xpconnect short-circuit) going away in a followup!
Attachment #625950 -
Flags: review?(bzbarsky) → review+
Comment 10•12 years ago
|
||
Comment on attachment 625951 [details] [diff] [review] Part 3 - Pull subject principals directly off the compartment. v1 Can you explain to me why this doesn't break nsScriptSecurityManager::PushContextPrincipal? Also, falling back to system when not in a compartment is a bit odd; the old code fell back to the results of GetScriptContextPrincipalFromJSContext() when there was nothing on the stack, no? So at first glance, this doesn't look right....
Attachment #625951 -
Flags: review?(bzbarsky) → review-
Comment 11•12 years ago
|
||
Oh, and one more thing. Do we update the compartment of the JSContext when we set up a new inner window? If not, then this code is wrong in that respect too: the old code would get the updated principal off the window directly in the "no script running" situation, so would get the updated principal.
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #8) > Uh... why? See comment 4.
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #9) > So a question. Do the callers of doGetObjectPrincipal ever have > cross-compartment proxies in obj? And if so, do they want the principals of > the proxy or of the underlying object? I guess the former, since the proxy > will do its own security checks? If they did, they'd get the principal of the proxy. There's no special wrapper handling in doGetObjectPrincipal AFAICT, which means that we'll just grab the wrapper's parent, which is the global of the wrapper (not the wrappee). > If so, r=me. Looking forward to all that code (including the code for the > xpconnect short-circuit) going away in a followup! Me too!
Comment 14•12 years ago
|
||
Hrm. That seems ... highly suboptimal. Did you look into why we have this test? Is anything relying on the old behavior?
Comment 15•12 years ago
|
||
Comment 14 was in response to comment 12...
Assignee | ||
Comment 16•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #10) > Can you explain to me why this doesn't break > nsScriptSecurityManager::PushContextPrincipal? It does, but we don't need it anymore. Attaching patches to rip it out momentarily. > Also, falling back to system when not in a compartment is a bit odd; the old > code fell back to the results of GetScriptContextPrincipalFromJSContext() > when there was nothing on the stack, no? Good call. I'll fix that. > Oh, and one more thing. Do we update the compartment of the JSContext when > we set up a new inner window? If not, then this code is wrong in that > respect too: the old code would get the updated principal off the window > directly in the "no script running" situation, so would get the updated > principal. I'm not sure. Do you know where that code would be, or what it should look like if it doesn't exist?
Comment 17•12 years ago
|
||
> Attaching patches to rip it out momentarily. I hope with some asserts about the principal already being correct! Please have smaug review the result? > Do you know where that code would be It would need to be in nsGlobalWindow::SetNewDocument or some callee. Probably right around where we move the outer window into a different compartment. As for what it should look like, I'm not sure; I don't know what APIs jseng exposes to just set the compartment of a JSContext and whatnot.. or even whether it's the right thing to do there.
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #17) > I hope with some asserts about the principal already being correct! Please > have smaug review the result? Yep. :-) > > > Do you know where that code would be > > It would need to be in nsGlobalWindow::SetNewDocument or some callee. > Probably right around where we move the outer window into a different > compartment. As for what it should look like, I'm not sure; I don't know > what APIs jseng exposes to just set the compartment of a JSContext and > whatnot.. or even whether it's the right thing to do there. Hm, maybe we should ask Blake. Blake, if there's no JS code on the stack, should we return system principal, or fall back to the GetScriptContextPrincipalFromJSContext stuff? If the latter, what do you think about bz's concern in comment 11?
Comment 19•12 years ago
|
||
1) Note that "no JS code on the stack" is not the same as "null context compartment". The compartment can be non-null even if there's no JS on the stack. 2) At least event dispatch relies on pushing the JSContext (with no code on it!) to push the right principals, iirc.
Comment 20•12 years ago
|
||
Comment on attachment 625948 [details] [diff] [review] Part 1 - Fix content->chrome postMessage mochitest. v1 r=me
Attachment #625948 -
Flags: review?(bzbarsky) → review+
Updated•12 years ago
|
Blocks: CVE-2012-1967
Comment 21•12 years ago
|
||
Just a note that I was hitting a crash when doing a postMessage, after the assertion "Assertion failure: principals == JS_GetCompartmentPrincipals((js::GetContextComp artment(cx))), at c:/Users/imelven/src/mozilla/caps/src/nsScriptSecurityManager. cpp:208" while working on iframe sandbox (bug 341604) that these patches fixed :)
Assignee | ||
Comment 22•12 years ago
|
||
FWIW the newer patches for this are here: https://github.com/bholley/mozilla-central/commits/decaps Haven't worked on it in a few weeks though, been busy :-(
Assignee | ||
Comment 23•12 years ago
|
||
Attachment #625951 -
Attachment is obsolete: true
Attachment #630200 -
Flags: review?(mrbkap)
Assignee | ||
Comment 24•12 years ago
|
||
Attachment #630201 -
Flags: review?(mrbkap)
Assignee | ||
Comment 25•12 years ago
|
||
Each one of these uses grabs the principal off of an object for pushing, but also enters the compartment of that object. So we shouldn't need this anymore. Can I get a 'hell yeah'?
Attachment #630202 -
Flags: review?(mrbkap)
Assignee | ||
Comment 26•12 years ago
|
||
Attachment #630203 -
Flags: review?(mrbkap)
Assignee | ||
Comment 27•12 years ago
|
||
No more principal pushing!
Attachment #630204 -
Flags: review?(mrbkap)
Comment 28•12 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #25) \o/ \o/ \o/ \o/ \o/ hell yeah \o/ \o/ \o/ \o/ \o/
Assignee | ||
Comment 29•12 years ago
|
||
Attachment #630200 -
Attachment is obsolete: true
Attachment #630200 -
Flags: review?(mrbkap)
Attachment #630223 -
Flags: review?(mrbkap)
Assignee | ||
Comment 30•12 years ago
|
||
mrbkap said on IRC that we should just assert that js::GetContextCompartment never returns null.
Assignee | ||
Comment 31•12 years ago
|
||
Pushed the updated stack here to try: https://tbpl.mozilla.org/?tree=Try&rev=4a4414893cb1
Assignee | ||
Comment 32•12 years ago
|
||
Looks green :-)
Comment 33•12 years ago
|
||
Comment on attachment 630201 [details] [diff] [review] Part 4 - Disallow calling EvaluateString{,WithValue} with a principal that doesn't match the global. v1 Review of attachment 630201 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsJSEnvironment.cpp @@ +1200,5 @@ > + return NS_ERROR_FAILURE; > +#ifdef DEBUG > + bool equal = false; > + principal->Equals(aPrincipal, &equal); > + MOZ_ASSERT(equal); Do we want to assert that aScopeObject's principals is the same as principal here?
Attachment #630201 -
Flags: review?(mrbkap) → review+
Comment 34•12 years ago
|
||
Comment on attachment 630202 [details] [diff] [review] Part 5 - Remove context pushing/popping API. v1 Review of attachment 630202 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/src/json/nsJSON.cpp @@ +185,4 @@ > } > > nsJSONWriter writer; > JSBool ok = JS_Stringify(cx, value, NULL, JSVAL_NULL, Looks like |ok| can go away now.
Attachment #630202 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #630203 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #630204 -
Flags: review?(mrbkap) → review+
Comment 35•12 years ago
|
||
Comment on attachment 630223 [details] [diff] [review] Part 3 - Pull subject principals directly off the compartment. v3 Review of attachment 630223 [details] [diff] [review]: ----------------------------------------------------------------- This is *hot*.
Attachment #630223 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 36•12 years ago
|
||
Addressed review comments. Gave this one last linux-debug-only push to try in case the asserts added from comment 33 fire: https://tbpl.mozilla.org/?tree=Try&rev=54566a8bd245
Assignee | ||
Comment 37•12 years ago
|
||
Looks green. Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/9447d46db02c http://hg.mozilla.org/integration/mozilla-inbound/rev/c78c96dc690f http://hg.mozilla.org/integration/mozilla-inbound/rev/a3c2e8b5f8a6 http://hg.mozilla.org/integration/mozilla-inbound/rev/b2dd0faca988 http://hg.mozilla.org/integration/mozilla-inbound/rev/dedd3f5ec475 http://hg.mozilla.org/integration/mozilla-inbound/rev/8d75c440e1b0 http://hg.mozilla.org/integration/mozilla-inbound/rev/fedb160013c0
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → bobbyholley+bmo
Target Milestone: --- → mozilla16
Comment 38•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fedb160013c0 https://hg.mozilla.org/mozilla-central/rev/8d75c440e1b0 https://hg.mozilla.org/mozilla-central/rev/dedd3f5ec475 https://hg.mozilla.org/mozilla-central/rev/b2dd0faca988 https://hg.mozilla.org/mozilla-central/rev/a3c2e8b5f8a6 https://hg.mozilla.org/mozilla-central/rev/c78c96dc690f https://hg.mozilla.org/mozilla-central/rev/9447d46db02c (Merged by Ed Morley)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 762578
Assignee | ||
Comment 39•12 years ago
|
||
Backed out patches 3-7 for various regressions: http://hg.mozilla.org/mozilla-central/rev/90107a2a0c64
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 40•12 years ago
|
||
Backout was backed out. https://hg.mozilla.org/mozilla-central/rev/17a91ff5dfd7 I guess technically this should be turned fixed, but I'll leave it alone for now...
I redid the backout, but backing out all patches rather than just patches 3-7: https://hg.mozilla.org/mozilla-central/rev/8d57c055f951
Assignee | ||
Comment 42•12 years ago
|
||
So, it looks like we have to remove the assert here because of (drumroll) UniversalXPConnect. When UniversalXPConnect is enabled, sometimes we run into situations where we create chrome objects with a content NodePrincipal. layout/base/tests/test_bug458898.html is an example of where this happens.
Attachment #625950 -
Attachment is obsolete: true
Attachment #632263 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 43•12 years ago
|
||
This fixes the regression from bug 762920.
Attachment #632264 -
Flags: review?(mrbkap)
Assignee | ||
Comment 44•12 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=045e28393ec5
Comment 45•12 years ago
|
||
> When UniversalXPConnect is enabled, sometimes we run into situations where we create
> chrome objects with a content NodePrincipal.
That sounds incredibly broken. Which exact objects in that testcase are "chrome objects with a content NodePrincipal()"?
I'm not happy at all with removing that assert, esp, because in this case it is in fact catching what totally looks like a bug to me.
Assignee | ||
Comment 46•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #45) > > When UniversalXPConnect is enabled, sometimes we run into situations where we create > > chrome objects with a content NodePrincipal. > > That sounds incredibly broken. Which exact objects in that testcase are > "chrome objects with a content NodePrincipal()"? > > I'm not happy at all with removing that assert, esp, because in this case it > is in fact catching what totally looks like a bug to me. The document here: https://mxr.mozilla.org/mozilla-central/source/layout/base/tests/test_bug458898.html?force=1#23
Assignee | ||
Comment 47•12 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #46) > The document here: > https://mxr.mozilla.org/mozilla-central/source/layout/base/tests/ > test_bug458898.html?force=1#23 Where does the principal come from for data urls?
Comment 48•12 years ago
|
||
> Where does the principal come from for data urls?
It depends. Typically it's inherited from somewhere, but in some cases we cut off inheritance and give them a nsNullPrincipal (e.g data: being loaded in a type="content" iframe by chrome code).
The JSObject for a Document with a Window should totally be in the compartment of said Window and should have the same principal as the Window and Document involved.
Comment 49•12 years ago
|
||
If you mean the specific code, there's a bunch of stuff about inheriting principals and whatnot in the docshell LoadURI/InternalLoad code, iirc.
Comment 50•12 years ago
|
||
Comment on attachment 632264 [details] [diff] [review] Part 8 - Check principal in IsCapabilityEnabled when there's no code on the stack. v1 Review of attachment 632264 [details] [diff] [review]: ----------------------------------------------------------------- Funnily enough, jst wrote a patch with basically the same semantics over in bug 235457 8 years ago! At the time, he was defeated by the fact that XPConnect did security checks with no code on the stack, calling out into chrome code on the safe JS context. That's been fixed for a while now.
Attachment #632264 -
Flags: review?(mrbkap) → review+
Comment 51•12 years ago
|
||
Comment on attachment 632263 [details] [diff] [review] Part 2 - Pull object principals directly off the compartment. v4 Per discussion.
Attachment #632263 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 52•12 years ago
|
||
(In reply to Blake Kaplan (:mrbkap) from comment #50) > Funnily enough, jst wrote a patch with basically the same semantics over in > bug 235457 8 years ago! At the time, he was defeated by the fact that > XPConnect did security checks with no code on the stack, calling out into > chrome code on the safe JS context. That's been fixed for a while now. So, it looks like this patch isn't quite out of the clear. It breaks some worker XHR tests. I debugged it, and found the following stacks which used to succeed and now fail: http://pastebin.mozilla.org/1662404 The subject principal at the time is the Null principal. Smaug, how is this stuff supposed to work? Do we need / need to get rid of a cx push somewhere?
Comment 53•12 years ago
|
||
Why is subject principal null. Shouldn't it be system.
Updated•12 years ago
|
Blocks: framesandbox
Assignee | ||
Comment 54•12 years ago
|
||
This fixes the XHR timeouts with Part 8 applied. 1-liner, exactly what we talked about on IRC, blake. Applies on top of bug 765168.
Attachment #633471 -
Flags: review?(mrbkap)
Assignee | ||
Updated•12 years ago
|
Attachment #625950 -
Attachment is obsolete: false
Assignee | ||
Updated•12 years ago
|
Attachment #632263 -
Attachment is obsolete: true
Assignee | ||
Comment 55•12 years ago
|
||
Gave this another try push: https://tbpl.mozilla.org/?tree=Try&rev=c203f65376fd
Comment 56•12 years ago
|
||
Comment on attachment 633471 [details] [diff] [review] Part 7.1 - Scope the push of the safe js context such that it doesn't include the call to XHR::Send. v1 I think this is right, but I'd like bent to look at this as well to double check.
Attachment #633471 -
Flags: review?(mrbkap)
Attachment #633471 -
Flags: review?(bent.mozilla)
Attachment #633471 -
Flags: review+
Assignee | ||
Comment 57•12 years ago
|
||
Last try push was borked. trying again: https://tbpl.mozilla.org/?tree=Try&rev=b004a98d59b5
Updated•12 years ago
|
Attachment #633471 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Comment 58•12 years ago
|
||
There were some linux-only mochitest-ally failures on the last push. I'm hoping they were from something underneath my patches. Pushing again to find out: https://tbpl.mozilla.org/?tree=Try&rev=46fe539c2d74
Assignee | ||
Comment 59•12 years ago
|
||
ok, a11y bustage looks real. Firing up a linux vm to investigate.
Assignee | ||
Comment 60•12 years ago
|
||
So, the test is failing when it tries to send a key event. We fail the following universalxpconnect check were we used to succeed: http://pastebin.mozilla.org/1668825 Basically, some privileged code calls nsDOMWindowUtils::SendKeyEvent on a content window. Along the way, we somehow end up pushing a context and entering a compartment such that we're no longer privileged, and fail the security check at nsHTMLInputElement::SetUserInput: http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp#1080
Assignee | ||
Comment 61•12 years ago
|
||
Also, the relevant JS stack is here: http://pastebin.mozilla.org/1668826
Assignee | ||
Comment 62•12 years ago
|
||
Ok, so here's the issue. When we dispatch events, we push the context from the event target because we might call content event handlers. But in the failing test, we don't - we just call C++. And that C++ now fails, because it thinks its running with content privileges. We used to do the right thing here because CAPS would grant UniversalXPConnect if there was no JS code on the callstack. But I plugged that little loophole in patch 8. It seems to me that the correct solution here is to push the event target principal only if we're running a scripted event handler. We could do this in the event dispatch code, or in XPCWrappedJSClass. The latter would skip a QI (since we could just directly check mIID), which is probably faster. But it's also a bit less clean. Smaug, Blake - can you guys weigh in? This is a pretty important patch to land for a number of reasons, and we're so close here.
Comment 63•12 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #62) > It seems to me that the correct solution here is to push the event target > principal only if we're running a scripted event handler. Push principal? I thought principal pushing is being removed. > We could do this > in the event dispatch code, or in XPCWrappedJSClass. The latter would skip a > QI (since we could just directly check mIID), which is probably faster. But > it's also a bit less clean. We would need the same thing also elsewhere, so it wouldn't make sense to hack event handling code only. All the callbacks should be handled the same way. (But note, onfoo event handlers are actually nsIJSEventListener objects)
Assignee | ||
Comment 64•12 years ago
|
||
Actually though, do we even need to be doing this at all? If we're running content code, we'll have to enter the content compartment (we'll get a compartment mismatch assertion if we don't). If we're pulling the subject principal directly off the context compartment, then we should be fine, no? Or is the cx pushing necessary for another reason?
Assignee | ||
Comment 65•12 years ago
|
||
Sounds like we really just need to kill context pushing here to make this work. I'm going to start on that over in bug 767938. CCing dvander and dmandelin to make sure they're aware of the situation here. Regardless of what we do about enablePrivilege, I'm pretty sure this bug does block IonMonkey, since browser security doesn't work without it. I didn't bring it up in our previous discussion because this had already landed at the time, and then got backed out. :-(
Comment 66•12 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #65) If IM never crosses globals in jit code (and thus always reports the correct "current global"), what is the problem?
Assignee | ||
Comment 67•12 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #66) > (In reply to Bobby Holley (:bholley) from comment #65) > If IM never crosses globals in jit code (and thus always reports the correct > "current global"), what is the problem? Is there always guaranteed to be at least one JSStackFrame for every compartment entered?
Comment 68•12 years ago
|
||
Yes: IM always enters jit code with the entry function's StackFrame pushed. From that initial stack frame, all calls stack within the same global. Any cross-compartment call will do the normal C++ CrossCompartmentWrapper::call path, which will push at least one more StackFrame before entering jit code again. The only way this could break is if two frames in the same global weren't equivalent from a security POV. (Frame annotations were one such case but, as you know, we have another hack to deal with that ;) Regardless, removing context-pushing and bug 767938 sound like most excellent things to do. IIUC, this would get us a long way toward removing JSContext (or, with super-snappy, having one per thread).
Assignee | ||
Comment 69•12 years ago
|
||
I've got patches to fix up event dispatch to only push a context when script is actually being entered. This is green on try, save one failure on android M-3: https://tbpl.mozilla.org/?tree=Try&rev=628da1b54855 Now I need to figure out how to look into that...
Assignee | ||
Comment 70•12 years ago
|
||
It's not used for anything substantial, and it can be (or always is?) called with no JS on the stack.
Attachment #637446 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 71•12 years ago
|
||
Smaug pointed out that I need to pop the context if I don't repush, since we're in a loop. Gave this another push to try. Crossing fingers: https://tbpl.mozilla.org/?tree=Try&rev=dd8b165df73c
Assignee | ||
Comment 72•12 years ago
|
||
Huzzah - It worked! Uploading the last patch, and flagging smaug for review.
Assignee | ||
Comment 73•12 years ago
|
||
Attachment #637487 -
Flags: review?(bugs)
Updated•12 years ago
|
Attachment #637446 -
Flags: review?(bent.mozilla) → review+
Comment 74•12 years ago
|
||
Comment on attachment 637487 [details] [diff] [review] Part 7.3 - Only push the context of the event target if the listener is scripted. v2 > // compiled the event handler itself go ahead and compile it >- if ((ls.mFlags & NS_PRIV_EVENT_FLAG_SCRIPT) && ls.mHandlerIsString) { >+ if ((ls.mListenerType == LISTENER_TYPE_JSEVENTLISTENER) && >+ ls.mHandlerIsString) >+ { { should be in the previous line >+typedef enum { { should be on its own line >+ LISTENER_TYPE_NATIVE = 0, >+ LISTENER_TYPE_JSEVENTLISTENER, >+ LISTENER_TYPE_WRAPPEDJS, >+ LISTENER_TYPE_COUNT Enum values should be in form eValueName > struct nsListenerStruct > { > nsRefPtr<nsIDOMEventListener> mListener; > PRUint32 mEventType; > nsCOMPtr<nsIAtom> mTypeAtom; > PRUint16 mFlags; > bool mHandlerIsString; >- bool mWrappedJS; >+ nsListenerType mListenerType; Is it guaranteed that PRUint16 + bool + nsListenerType <= 4 bytes? We don't want to increase the size of nsListenerStruct unless really needed. >+ // We're about to create a new nsJSEventListener, which means that we're >+ // responsible for pushing the context of the event target. See the similar >+ // comment in nsEventManagerListener.cpp. >+ nsCxPusher pusher; >+ if (!pusher.Push(aTarget)) >+ return NS_ERROR_FAILURE; NS_ENSURE_STATE(pusher.Push(aTarget)); Those addressed, r=me
Attachment #637487 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 75•12 years ago
|
||
Landed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/cf09f7b139da http://hg.mozilla.org/integration/mozilla-inbound/rev/3c9880d4eb15 http://hg.mozilla.org/integration/mozilla-inbound/rev/ab626a14a2f0 http://hg.mozilla.org/integration/mozilla-inbound/rev/53ccd3f1972d http://hg.mozilla.org/integration/mozilla-inbound/rev/bafdc2c0afc5 http://hg.mozilla.org/integration/mozilla-inbound/rev/34eb5691b71c http://hg.mozilla.org/integration/mozilla-inbound/rev/6d94014e416d http://hg.mozilla.org/integration/mozilla-inbound/rev/b647b29c8326 http://hg.mozilla.org/integration/mozilla-inbound/rev/6f70fe755b10 http://hg.mozilla.org/integration/mozilla-inbound/rev/93f05c65f3f1 http://hg.mozilla.org/integration/mozilla-inbound/rev/77f3f1009869
Comment 76•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/77f3f1009869 https://hg.mozilla.org/mozilla-central/rev/93f05c65f3f1 https://hg.mozilla.org/mozilla-central/rev/6f70fe755b10 https://hg.mozilla.org/mozilla-central/rev/b647b29c8326 https://hg.mozilla.org/mozilla-central/rev/6d94014e416d https://hg.mozilla.org/mozilla-central/rev/34eb5691b71c https://hg.mozilla.org/mozilla-central/rev/bafdc2c0afc5 https://hg.mozilla.org/mozilla-central/rev/53ccd3f1972d https://hg.mozilla.org/mozilla-central/rev/ab626a14a2f0 https://hg.mozilla.org/mozilla-central/rev/3c9880d4eb15 https://hg.mozilla.org/mozilla-central/rev/cf09f7b139da
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 77•12 years ago
|
||
Comment on attachment 630201 [details] [diff] [review] Part 4 - Disallow calling EvaluateString{,WithValue} with a principal that doesn't match the global. v1 So, can we remove those principal arguments at some point?
Assignee | ||
Comment 78•12 years ago
|
||
(In reply to :Ms2ger from comment #77) > So, can we remove those principal arguments at some point? Yep. Feel free to file a bug (and also one for removing the vestigial doGetObjectPrincipal_old stuff). We probably want to leave it the way it is for 6 weeks or so, and then rip it out.
Updated•12 years ago
|
Depends on: CVE-2012-4205
Assignee | ||
Updated•12 years ago
|
No longer depends on: CVE-2012-4205
Updated•12 years ago
|
Depends on: CVE-2012-3991
Updated•12 years ago
|
Depends on: CVE-2012-4195
Updated•12 years ago
|
Blocks: CVE-2012-3991
No longer depends on: CVE-2012-3991
Updated•12 years ago
|
Depends on: CVE-2012-4192
You need to log in
before you can comment on or make changes to this bug.
Description
•