316589 - 1.0.x split-window alternative

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Daniel Veditz [:dveditz]]

the full split-windows changes are bigger and scarier than we're comfortable backporting to the 1.0x branch. Brendan and Blake have tossed around some ideas that might stop the XSS holes without requiring the full architectural changes. This is where that alternative will go.

Comment 1

[Brendan Eich [:brendan]]

Note the idea for 1.0.8 will impose a performance penalty, and it won't fix all the non-security bugs that split windows fixed.  Just wanted to clarify that there is a reason we did split windows.

/be

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

(please excuse the misleading TM field that's making up for a lack of good todo-list type bugzilla queries).

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: rough patch

This runs and works, but I need to work on the xpconnect changes a little, and to verify that the build problems I saw were the result of my builds, and not of something that I broke.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Note to self: this patch isn't gc safe (it leaves the principal object dangling).

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: updated rough patch

This is closer, still on the to-do:
* We currently crash on the testcase from bug 296514 because we try to create the principals object with a parent from google, but with the last stack frame (with principals) being from the attachment page and XBL can't cope. I'm going to fix this by pushing a fake stack frame on the context with the principals that I want, forcing creation of the object.
* There are some NS_ERRORs and JS_ASSERT(0)s that I'm using for testing; they need to go.
* I badly named my new function and its uses: 'principals' should be the singular 'principal'.
* Test, test, test...

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Potential patch

This patch fixes all of the security dependencies on bug 296639 and seems to not totally break the world. I don't know of any outstanding issues with it.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 203851 [details] [diff] [review]
Potential patch

I talked blake into plan A ;-).

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Smaller patch alternative

This works and also fixes all of the security bugs hanging off of bug 296639.

Comment 9

[Mike Shaver (:shaver emeritus)]

Comment on attachment 203875 [details] [diff] [review]
Smaller patch alternative


> /*
>  * Reserve two slots in all function objects for XPConnect.  Note that this
>  * does not bloat every instance, only those on which reserved slots are set,
>  * and those on which ad-hoc properties are defined.
>  */
> JSClass js_FunctionClass = {
>     js_Function_str,
>-    JSCLASS_HAS_PRIVATE | JSCLASS_NEW_RESOLVE | JSCLASS_HAS_RESERVED_SLOTS(2),
>+    JSCLASS_HAS_PRIVATE | JSCLASS_NEW_RESOLVE | JSCLASS_HAS_RESERVED_SLOTS(3),

This comment is not so much true any more.  It'd be nice if it also included a link
or other reference to where those slots are used.

Mike

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

I've changed that comment to be:

/*
 * Reserve three slots in all function objects. The first two are used by
 * XPConnect to remember information about what interface and member function a
 * particular cloned function represents.  Note that this does not bloat every
 * instance, only those on which reserved slots are set, and those on which
 * ad-hoc properties are defined.  The third slot is used by
 * js_CloneFunctionObject to remember a lexically scoped function's principal.
 * See the uses of JS_GetReservedSlot in xpcwrappednativeinfo.cpp and
 * XPCDispObject.cpp
 */

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 203875 [details] [diff] [review]
Smaller patch alternative

Regarding the revised comment for:

> /*
>  * Reserve two slots in all function objects for XPConnect.  Note that this
>  * does not bloat every instance, only those on which reserved slots are set,
>  * and those on which ad-hoc properties are defined.
>  */
> JSClass js_FunctionClass = {

viz,

/*
 * Reserve three slots in all function objects. The first two are used by
 * XPConnect to remember information about what interface and member function a
 * particular cloned function represents.  Note that this does not bloat every
 * instance, only those on which reserved slots are set, and those on which
 * ad-hoc properties are defined.  The third slot is used by
 * js_CloneFunctionObject to remember a lexically scoped function's principal.
 * See the uses of JS_GetReservedSlot in xpcwrappednativeinfo.cpp and
 * XPCDispObject.cpp
 */

How about putting the Note after all the sentences to which it applies?

/*
 * Reserve three slots in all function objects. The first two are used by
 * XPConnect to remember information about what interface and member function a
 * particular cloned function represents.  The third slot is used by
 * js_CloneFunctionObject to remember a lexically scoped function's principal.
 * See the uses of JS_GetReservedSlot in xpcwrappednativeinfo.cpp and
 * XPCDispObject.cpp.
 *
 * Note that these reservations do not bloat every instance, only those on which
 * reserved slots are set, and those on which ad-hoc properties are defined.
 */

Here and later, "lexically scoped" is used to denote certain functions, but all JS functions are lexically (or statically) scoped, not dynamically scoped.  Suggest "cloned function object" instead, with some words on why we need those (for when the compile-time lexical scope does not match a given declaration's run-time lexical scope; examples include nested functions, function expressions, "closures" a.k.a. function statements, and brutal sharing).

>         else if (JS_GetFunctionObject(fun) != obj)
>         {
>-            // Function is a clone, its prototype was precompiled from
>-            // brutally shared chrome. For this case only, get the
>-            // principals from the clone's scope since there's no
>-            // reliable principals compiled into the function.
>-            return doGetObjectPrincipal(cx, obj, result);
>+            // Function is a clone. In some cases (such as a lexically
>+            // scoped function in a page)

See above, reword.

>+                                          the JS engine sticks the
>+            // function's principals in the third slot (0 based).
>+            // If it doesn't, then we're probably looking at a function
>+            // that was not in a script page (such as a function in
>+            // the component manager),

"in a JS component" or "in a JS-implemented XPCOM component"

>+                                       and we should fall back on getting
>+            // the principals from its scope.
>+            jsval v;
>+            if (!JS_GetReservedSlot(cx, obj, 2, &v)

Error has been thrown as exception, or reported as OOM.  This should fail the current GetFramePrincipal call and propagate failure out of CAPS code.

                                                      || JSVAL_IS_VOID(v)) {
>+                return doGetObjectPrincipal(cx, obj, result);
>+            }
>+
>+            nsJSPrincipals *prin =
>+                NS_STATIC_CAST(nsJSPrincipals *, JSVAL_TO_PRIVATE(v));

Is this right?  Don't you have to cast the private to JSPrincipals * first, or does that start at the front of nsJSPrincipals?  Ah, it's just struct single inheritance -- still, what's the best practice here, to future-proof or brainprint-relief this?

Note violations of CAPS prevailing brace style.

r=me with some fixes to these minor matters.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

Brendan, do you want to give my comments one last look-over?

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 203963 [details] [diff] [review]
Updated to comments

Looks fine, just one suggestion: function expression rather than function statement in the first big comment's parenthetical example, to match the caps comment and pick the most common example.

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 203963 [details] [diff] [review]
Updated to comments

Requesting approval for the branches. This small patch fixes all known splitwindow security holes (at a memory footprint cost). It is not needed on the trunk or the 1.8 branch because of splitwindow.

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #14)
> (From update of attachment 203963 [details] [diff] [review] [edit])
> Requesting approval for the branches. This small patch fixes all known
> splitwindow security holes (at a memory footprint cost)

And a small cycle hit.

This shuld go in after having some testerboxes up on the branch.  Do we have enough such tinderboxes already running?

/be

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 203963 [details] [diff] [review]
Updated to comments

a=dveditz for drivers, please add fixed-aviary1.0.8 and fixed1.7.13 keywords when checked in.

Comment 17

[Marcia Knous [:marcia]]

QA will need to spend some good cycles testing this. Lets make sure we take a good look at this one.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Checked into branches.

Comment 19

[Marcia Knous [:marcia]]

Using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060216 Firefox/1.0.8, I went back to the original bug (Bug 296639) and tried out some of the various sites that were causing crashes in the original bug, including:
(1) https://bugzilla.mozilla.org/attachment.cgi?id=112654

Also tried some quick shift reloads. I did not crash or experience any issues at these sites. Will try some additional testing on this and report back.

Comment 20

[Jay Patel [:jay]]

v.fixed on 1.0.1 Aviary branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060220 Firefox/1.0.8, using testcases from various other bugs fixed by original split window patch on other trunk/branch:

bug 304181 - pass, no js errors with reload, back, or forward
bug 311024 - pass, xss testcase blocks attack
bug 298315 - pass (already verified by me on 2/13)
bug 253951 - fail (decided it was too scary to try to fix this on 1.7/Aviary branch)
bug 311892 - pass (already verified by me on 2/15)
bug 311619 - pass (verified by me on 2/20)
bug 296514 - pass (verified by me on 2/20)
bug 327194 - pass (verified by me on 2/20)