643967 - js_CloneScript does not preserve principals for nested functions

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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.

Comment 1

[Johnny Stenback (:jst)]

Igor, can you explain how bad this is from a security point of view?

Comment 2

[Igor Bukanov]

Blake, could you comment on implications of null principals in JSScript->principals?

Comment 3

[Andreas Gal :gal]

Its a crasher in a bunch of places. I think this is a [dos] only, but Blake is more familiar with this code.

Comment 4

[Michael Wu [:mwu]]

Attachment: Like this?

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)

Comment 5

[Johnny Stenback (:jst)]

Blake, can you please suggest a rating for this bug per https://wiki.mozilla.org/Security_Severity_Ratings

Comment 6

[Johnny Stenback (:jst)]

Blake, ping?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

It's hard to judge here. My initial instinct would be to mark this [sg:crit?].

Comment 8

[Johnny Stenback (:jst)]

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 9

[Johnny Stenback (:jst)]

Blake, ping?

Comment 10

[Johnny Stenback (:jst)]

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.

Comment 11

[Johnny Stenback (:jst)]

This missed 8 :(

Blake, review ping?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: testcase

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).

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

The last hunk is now bug 718733.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

I haven't tested this thoroughly enough, but this seems to do the trick. I'll probably ask for review tomorrow.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 16

[Luke Wagner [:luke]]

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.

Comment 17

[Igor Bukanov]

(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.

Comment 18

[Igor Bukanov]

(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.

Comment 19

[Luke Wagner [:luke]]

(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.

Comment 20

[Igor Bukanov]

Attachment: separate principals from scripts

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.

Comment 21

[Luke Wagner [:luke]]

Blake should review, methinks, but nice patch.  s/checkAllPrincipas/checkAllPrincipals/

Comment 22

[Igor Bukanov]

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.

Comment 23

[Igor Bukanov]

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?

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

(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?

Comment 25

[Luke Wagner [:luke]]

Yeah: bug 679940.  If you follow the dependency, it enables some other cool stuff.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: testcase

I gave the principals names for easier debugging.

Comment 27

[Igor Bukanov]

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.

Comment 28

[Igor Bukanov]

(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.

Comment 29

[Igor Bukanov]

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?

Comment 30

[Igor Bukanov]

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.

Comment 31

[Johnny Stenback (:jst)]

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.

Comment 32

[Igor Bukanov]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #31)
> Igor, is there anything else left to do here? 

I am waiting an input about comment 29.

Comment 33

[Luke Wagner [:luke]]

Igor: would the "hoist stringFilenameTable" patch in bug 720753 fix this issue?

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

(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.

Comment 35

[Igor Bukanov]

(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.

Comment 36

[David Mandelin [:dmandelin]]

(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.

Comment 37

[Igor Bukanov]

(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.

Comment 38

[David Mandelin [:dmandelin]]

(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!

Comment 39

[David Mandelin [: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.

Comment 40

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 41

[Luke Wagner [:luke]]

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?

Comment 42

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 43

[Johnny Stenback (:jst)]

Ok, FIXED then! Thanks Blake, Luke, David, Igor, and everyone else who chimed in here!

Comment 44

[Alex Keybl [:akeybl]]

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?

Comment 45

[David Mandelin [:dmandelin]]

(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.

Comment 46

[David Mandelin [:dmandelin]]

(In reply to Johnny Stenback (:jst, [e-mail]) 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!

Comment 47

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 48

[Daniel Veditz [:dveditz]]

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.

Comment 49

[Lukas Blakk [:lsblakk] use ?needinfo]

[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.

Comment 50

[Al Billings [:abillings - ex-MoCo]]

(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?

Comment 51

[Al Billings [:abillings - ex-MoCo]]

or for normal Firefox 13 (beta branch) for that matter.

Comment 52

[Alex Keybl [:akeybl]]

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.

Comment 53

[Daniel Veditz [:dveditz]]

Both dependent bugs appear to have been fixed in Firefox 13:
  bug 725576 comment 17
  bug 739808 comment 14

Comment 54

[Alex Keybl [:akeybl]]

Flag confusion - back to 13+

Comment 55

[Alex Keybl [:akeybl]]

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.

Comment 56

[Tracy Walker [:tracy]]

verified per depends bugs tinderbox tests

Comment 57

[Christian Holler (:decoder)]

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! :)

Comment 58

[Blake Kaplan (:mrbkap) (inactive)]

It looks like bug 725576 landed the test from this bug.

Comment 59

[Christian Holler (:decoder)]

Thanks for checking :)