1.0.x split-window alternative

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
DOM
P2
major
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: dveditz, Assigned: mrbkap)

Tracking

({fixed1.7.13})

1.8 Branch
mozilla1.9alpha1
fixed1.7.13
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high] xss)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Updated

12 years ago
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Whiteboard: [sg:high] xss
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
(Assignee)

Comment 2

12 years ago
(please excuse the misleading TM field that's making up for a lack of good todo-list type bugzilla queries).
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha

Updated

12 years ago
Summary: 1.0x split-window alternative → 1.0.x split-window alternative
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

12 years ago
Created attachment 203478 [details] [diff] [review]
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.
(Assignee)

Comment 4

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

Comment 5

12 years ago
Created attachment 203620 [details] [diff] [review]
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...
Attachment #203478 - Attachment is obsolete: true
(Assignee)

Comment 6

12 years ago
Created attachment 203851 [details] [diff] [review]
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.
Attachment #203620 - Attachment is obsolete: true
Attachment #203851 - Flags: review?(brendan)
Comment on attachment 203851 [details] [diff] [review]
Potential patch

I talked blake into plan A ;-).

/be
Attachment #203851 - Flags: review?(brendan) → review-
(Assignee)

Comment 8

12 years ago
Created attachment 203875 [details] [diff] [review]
Smaller patch alternative

This works and also fixes all of the security bugs hanging off of bug 296639.
Attachment #203851 - Attachment is obsolete: true
Attachment #203875 - Flags: review?(brendan)
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
(Assignee)

Comment 10

12 years ago
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 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
Attachment #203875 - Flags: review?(brendan) → review+
(Assignee)

Comment 12

12 years ago
Created attachment 203963 [details] [diff] [review]
Updated to comments

Brendan, do you want to give my comments one last look-over?
Attachment #203963 - Flags: review?(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
Attachment #203963 - Flags: review?(brendan) → review+
(Assignee)

Comment 14

12 years ago
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.
Attachment #203963 - Flags: approval1.7.13?
Attachment #203963 - Flags: approval-aviary1.0.8?
(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
(Reporter)

Comment 16

11 years ago
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.
Attachment #203963 - Flags: approval1.7.13?
Attachment #203963 - Flags: approval1.7.13+
Attachment #203963 - Flags: approval-aviary1.0.8?
Attachment #203963 - Flags: approval-aviary1.0.8+
QA will need to spend some good cycles testing this. Lets make sure we take a good look at this one.
(Assignee)

Comment 18

11 years ago
Checked into branches.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed-aviary1.0.8, fixed1.7.13
Resolution: --- → FIXED
Depends on: 326279
(Reporter)

Updated

11 years ago
Blocks: 327194
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.

(Reporter)

Updated

11 years ago
No longer blocks: 327194

Comment 20

11 years ago
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)
Keywords: fixed-aviary1.0.8 → verified-aviary1.0.8

Updated

11 years ago
Flags: testcase-
(Reporter)

Updated

11 years ago
Blocks: 310664
(Reporter)

Updated

11 years ago
Group: security
You need to log in before you can comment on or make changes to this bug.