Closed Bug 643967 Opened 11 years ago Closed 10 years ago
_Clone Script does not preserve principals for nested functions
js_CloneScript that was introduced in the bug 584789 only preserves the principals for the script itself and keeps the principals for all nested functions and their scripts set to null.
Igor, can you explain how bad this is from a security point of view?
Blake, could you comment on implications of null principals in JSScript->principals?
Its a crasher in a bunch of places. I think this is a [dos] only, but Blake is more familiar with this code.
I just realized that we could fix this bug and reduce the size of our serialized scripts by only serializing the principals once. This assumes that all the JSScripts can share a principal and all the sub JSScripts are effectively the same. I have no idea if that's true since I have no idea what principals are, but my browser does start with this patch. I see a 8% drop in the serialized XPIProvider.jsm with this patch. (168612 -> 155092)
Blake, can you please suggest a rating for this bug per https://wiki.mozilla.org/Security_Severity_Ratings
Assignee: igor → mrbkap
Whiteboard: [needs rating from mrbkap]
It's hard to judge here. My initial instinct would be to mark this [sg:crit?].
Whiteboard: [needs rating from mrbkap] → [sg:critical?]
Michael, Blake, is the attached patch good here, does it need a review request? We should get this fixed for 7, and we're getting close to the end of Aurora there.
Comment on attachment 524783 [details] [diff] [review] Like this? Blake says this is an acceptable approach here, and he'll have a closer look at this patch.
This missed 8 :( Blake, review ping?
10 years ago
10 years ago
Whiteboard: [sg:critical?] → [sg:critical]
There's a hunk of this that needs its own bug (the addition to tests.h). Otherwise, this shows the bug as far as I can tell. I'm still working on a simple patch :-/ (my great idea turned out to be harder than I previously thought).
The last hunk is now bug 718733.
I haven't tested this thoroughly enough, but this seems to do the trick. I'll probably ask for review tomorrow.
Comment on attachment 593516 [details] [diff] [review] patch v1 This bug got a little bit more complicated to fix after the introduction of originPrincipals. Before originPrincipals, we could have simply re-serialized the principal as the principal of the current compartment. Now, however, we have to preserve one principal but not the other. So this patch makes us preserve the principal through the clone operation and the does a fixup pass to make the principal correct.
What about adding optional 'newPrincipals' / 'newOriginPrincipals' arguments to js_XDRScript/js_XDRNewFunctionObject? If these were non-null, we could completely skip the principals XDR block and, if decoding, use these as the script's principals/originPrincipals. IIUC, this would be only a few lines change and easy to follow.
(In reply to Luke Wagner [:luke] from comment #16) > What about adding optional 'newPrincipals' / 'newOriginPrincipals' arguments > to js_XDRScript/js_XDRNewFunctionObject? All the nested functions have the same principals as their master script. So this suggest the following approach. We completely remove encoding-decoding of principals from script serialization. Then we add a separated API that assign the given principal and origin to the script and all its bested functions. This API will be used during parsing, cloning and by mozilla component loader in uniform fashion.
(In reply to Igor Bukanov from comment #17) > All the nested functions have the same principals as their master script. So > this suggest the following approach. We completely remove encoding-decoding > of principals from script serialization. Then we add a separated API that > assign the given principal and origin to the script and all its bested > functions. This API will be used during parsing, cloning and by mozilla > component loader in uniform fashion. Note that for a spot fix for this bug we can add fields to XDR with necessary principals. Then during encoding the code would assert that encountered principals match the principals in xdr and during decoding they will be used for initialization.
(In reply to Igor Bukanov from comment #18) Sounds good. It may even speedup overall XDR decoding, depending on the overall speed of principal encoding/decoding.
The patch adds a new method, JSSScript::initPrincipals, that recursively assigns the principals to all the nested scripts. This allows to separate xdr of principals from script encoding/decoding. As a bonus, the principals are now serialized once per top-level script. Changing the parser to use the same method to avoid passing around all the principals is for another bug.
Blake should review, methinks, but nice patch. s/checkAllPrincipas/checkAllPrincipals/
I filed the bug 725576 as a cover for this patch to keep this bug as a way to land the test case when we open it to the public.
There is another bug in our xdr serializer. Our scanner allows one to write //@line <line> "<filename>" to specifythe explicit filename that should be used in error reporting and stored in the script. That can present in the file multiple times. When we create script, we set JSScript::filename to the current value of filename. That implies that different functions in a script can have different filename. However, our xdr serializer does not know about that and stores only the name of the main script. AFAICS this leads only to a harmless assert. Also, this does not affect js_CloneScript since it is used only to clone functions and for function we always parse its whole source before emitting any scripts for nested functions. I also assume that we do not use JSScript::filename in any security checks. On the other hand, as script-embedded filename can be almost arbitrary sequence of bytes up to 1024 bytes in length, do we deal properly with that when using filename, for example, to display the source location in the error reports?
(In reply to Igor Bukanov from comment #23) > There is another bug in our xdr serializer. Our scanner allows one to write > > //@line <line> "<filename>" I think that in Firefox, we currently never enable JSOPTION_ATLINE, so we should be safe from this for now. We might want to fix this bug for embedders, though. That being said, Luke had a plan for js_CloneScript that involved splitting out the core of the script from the objects in order to avoid this crazy serialization/deserialization. Luke, is there a bug filed on that?
Yeah: bug 679940. If you follow the dependency, it enables some other cool stuff.
I gave the principals names for easier debugging.
Attachment #589207 - Attachment is obsolete: true
Yet another problem with CloneScript. It looks like we do not deal properly with nested functions like flat closures as XDR does not allocate function objects with the right allocation kind.
(In reply to Igor Bukanov from comment #27) > Yet another problem with CloneScript. It looks like we do not deal properly > with nested functions like flat closures as XDR does not allocate function > objects with the right allocation kind. I was wrong here - the issue is that JS_XDRFunctionObject was not updated to deal with nested closures while js_CloneFunctionObject properly creates and initializes the clone. As JS_XDRFunctionObject is not exposed to web scripts, this is just an issue for XBL serialization.
I noticed that nsJSContext::CompileEventHandler, https://hg.mozilla.org/mozilla-central/file/f88a05e00f47/dom/base/nsJSEnvironment.cpp#l1802 , compiles its handlers without principals with comments: // Event handlers are always shared, and must be bound before use. // Therefore we never bother compiling with principals. However, this means that nested functions in the event handlers also have null principals. Why is this safe? Blake, could you comment on that?
There is yet another issue with CloneScript. The bug 661903 has moved the script table to the compartment, yet I missed during the review that the js_CloneScript code skip encoding/decoding of JSScript::filename and just set the filename for the decoded script to the original one. That results in JSScript with a pointer to the filename from a different compartment so that filename can be GC-ed.
Igor, is there anything else left to do here? Seems the blocking bugs are both fixed. Also assigning this to you since it seems you took over here.
Assignee: mrbkap → igor
(In reply to Johnny Stenback (:jst, firstname.lastname@example.org) from comment #31) > Igor, is there anything else left to do here? I am waiting an input about comment 29.
Whiteboard: [sg:critical] → [sg:critical] [need mrbkap to reply to comment 29]
Igor: would the "hoist stringFilenameTable" patch in bug 720753 fix this issue?
(In reply to Igor Bukanov from comment #29) > However, this means that nested functions in the event handlers also have > null principals. Why is this safe? This is safe because event handlers are always cloned function objects. Because of this, in order to compute their principals, caps ignores their script (and therefore their script->principals) and instead uses their actual scope chains (parent link) to find their principals.
(In reply to Blake Kaplan (:mrbkap) from comment #34) > This is safe because event handlers are always cloned function objects. But what about a nested function defined in the event handler? In the clone it will still have null principals. If this nested function is optimized, at run time we skip creating closure for it and will use it as is during execution AFAIK.
Whiteboard: [sg:critical] [need mrbkap to reply to comment 29] → [sg:critical]
(In reply to Igor Bukanov from comment #30) > There is yet another issue with CloneScript. The bug 661903 has moved the > script table to the compartment, yet I missed during the review that the > js_CloneScript code skip encoding/decoding of JSScript::filename and just > set the filename for the decoded script to the original one. That results in > JSScript with a pointer to the filename from a different compartment so that > filename can be GC-ed. I inspected that code with Luke and it looks OK to me. IIUC, the script argument to CloneScript and its nested scripts will be in the same compartment, so it's OK for them to share filenames. I built a browser that asserts the filename matches the compartment when they are assigned, and it starts up and does stuff fine. I'm running it on try now.
(In reply to David Mandelin from comment #36) > I built a browser that > asserts the filename matches the compartment when they are assigned, and it > starts up and does stuff fine. I'm running it on try now. I fixed this issue in the bug 725576. So the only remaining issue is the comment 35.
(In reply to Igor Bukanov from comment #37) > (In reply to David Mandelin from comment #36) > > I built a browser that > > asserts the filename matches the compartment when they are assigned, and it > > starts up and does stuff fine. I'm running it on try now. > > I fixed this issue in the bug 725576. So the only remaining issue is the > comment 35. Ah, no wonder it looked right!
Assignee: igor → dmandelin
All that's left is to check with mrbkap on comment 35. But he's on vacation right now, so we'll have to ask when he gets back.
So, I keep meaning to read a bunch of code but I haven't had time to do so. Basically, the answer to comment 35 is that it used to be safe to assume that we wouldn't optimize nested functions in event handlers since there was no way that their global would be the "correct" global and we would be guaranteed to clone the function object, which would be safe. However, reading through the code now, it appears that null closures that are METHODs can appear on the JS stack for a security check without having been cloned, meaning that caps would get confused... So unless I'm confused (which is very possible here!) there is a little work left to do. At the very least, it'd be good to have a testcase showing that this is either safe or not.
What luck, bug 739808 removed methods. So that shouldn't be a problem. However, there is also this CloneFunctionObjectIfNotSingleton (http://mxr.mozilla.org/mozilla-central/source/js/src/jsfuninlines.h#246). This function, it seems, mutates the parent/environment of the compiler-created function in-place. Is that good enough?
Yes. From IRC: 19:12 < bhackett> mrbkap: for functions, native functions will have singleton types, and scripted functions declared in global scope so lambdas will never have singleton types and we'll always create cloned function objects. I think we can resolve this bug, finally.
Ok, FIXED then! Thanks Blake, Luke, David, Igor, and everyone else who chimed in here!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
We noticed that the two patches that resolve this bug span FF13 and FF14, and both seem risky for the ESR. What alternatives do we have?
(In reply to Alex Keybl [:akeybl] from comment #44) > We noticed that the two patches that resolve this bug span FF13 and FF14, > and both seem risky for the ESR. What alternatives do we have? I don't really know that much about the bug itself, so I don't have any brilliant ideas. Bug 739808 should not be that hard to rebase, or alternatively, a relatively small patch (similar to my first one in that bug) could disable the optimization. Not sure about bug 725576.
(In reply to Johnny Stenback (:jst, email@example.com) from comment #43) > Ok, FIXED then! Thanks Blake, Luke, David, Igor, and everyone else who > chimed in here! YAAAAAAAAAYYYYYYYYYY!!!!! Finally off my radar! / / o \|/ | / \ And thanks again from me!
Verified by verifying 725576 & 739808 on trunk with checked in tests. If people think this needs more verification, speak up, but I'm at a loss of what else to do for verification.
Status: RESOLVED → VERIFIED
bug 725576 is a 39K patch and bug 739808 is a 135K patch (including tests). We'll need a smaller combined patch to take this on the ESR. Does it make sense to wait 12 weeks when it'll be the same amount of work to get it fixed in 6? (In reply to David Mandelin from comment #45) > alternatively, a relatively small patch (similar to my first one in > [bug 739808]) could disable the optimization. I only see one patch in that bug, and no comments talking about other patches.
[Triage Comment] We'll track this for the ESR that goes out with Firefox 13 - we'll need to get the combined smaller patch (Dmandelin can you do this?) and then have the backport landed early enough into ESR10 nightlies that we can get some testing to verify this.
(In reply to Lukas Blakk [:lsblakk] from comment #49) > [Triage Comment] > We'll track this for the ESR that goes out with Firefox 13 - we'll need to > get the combined smaller patch (Dmandelin can you do this?) and then have > the backport landed early enough into ESR10 nightlies that we can get some > testing to verify this. Any update on this patch for ESR 13?
or for normal Firefox 13 (beta branch) for that matter.
I moved this from 13+ to 14+ because FF13 is still marked as affected (it doesn't have bug 739808), and we won't take bug 739808 on ESR until it's on Beta.
Both dependent bugs appear to have been fixed in Firefox 13: bug 725576 comment 17 bug 739808 comment 14
Just spoke to Luke and Blake - they do not believe bug 725576 is necessary on the ESR as long as bug 739808 was properly fixed. Additionally, the backport would be very painful. Marking the ESR as fixed here and bug 725576 as wontfix for ESR.
Whiteboard: [sg:critical] [fixed by 725576 & 739808] → [advisory-tracking+][sg:critical] [fixed by 725576 & 739808]
verified per depends bugs tinderbox tests
The bugs that fixed this don't seem to provide any tests, but a test is attached here. mrbkap, does this test still make sense and can we put it in the testsuite? If not, just in-testsuite- this, thanks! :)
It looks like bug 725576 landed the test from this bug.
Flags: needinfo?(mrbkap) → in-testsuite+
Thanks for checking :)
You need to log in before you can comment on or make changes to this bug.