Freeze ParallelArray objects; rewrap .shape property

RESOLVED FIXED in Firefox 17

Status

()

--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: shu, Assigned: shu)

Tracking

({regression, sec-critical})

unspecified
mozilla18
regression, sec-critical
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox16 unaffected, firefox17+ fixed, firefox18 fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [qa-][adv-track-main17-])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
ParallelArray objects should be frozen in the Object.isFrozen sense.

Further, the .shape property on objects is currently writable by users and needs to be rewrapped every access. This is a glaring security hole, marking s-s.
(Assignee)

Updated

6 years ago
Assignee: general → shu
(Assignee)

Updated

6 years ago
status-firefox17: --- → affected
status-firefox18: --- → affected
(Assignee)

Comment 1

6 years ago
Created attachment 659586 [details] [diff] [review]
fix and testcases

Until Waldo implements something to deal with all properties uniformly between native/non-native objects, this code is a bit hacky to duplicate behavior of frozen objects.

Makes .length and .shape "own" properties instead of dangling them as getters off the prototype.

Copy the .shape array everytime it's accessed.
Attachment #659586 - Flags: review?(dmandelin)
(Assignee)

Updated

6 years ago
Flags: in-testsuite+
(Assignee)

Comment 2

6 years ago
Created attachment 659612 [details] [diff] [review]
fix and testcase v2

Forgot to reuse the type of copied .shape array.

dmandelin, would you be willing to review this or this freeze semantics kind of stuff is better off for Waldo?
Attachment #659586 - Attachment is obsolete: true
Attachment #659586 - Flags: review?(dmandelin)
(In reply to Shu-yu Guo [:shu] from comment #2)
> Created attachment 659612 [details] [diff] [review]
> fix and testcase v2
> 
> Forgot to reuse the type of copied .shape array.
> 
> dmandelin, would you be willing to review this or this freeze semantics kind
> of stuff is better off for Waldo?

Yes, Jeff would be a better reviewer.
(Assignee)

Updated

6 years ago
Attachment #659612 - Flags: review?(jwalden+bmo)
Keywords: sec-critical
status-firefox-esr10: --- → unaffected
status-firefox16: --- → unaffected

Comment 4

6 years ago
Comment on attachment 659612 [details] [diff] [review]
fix and testcase v2

Review of attachment 659612 [details] [diff] [review]:
-----------------------------------------------------------------

I know nothing at all about ParallelArray semantics, so I'm making some assumptions about what's being implemented here, and how it fits in with the rest of the implementation, and about the semantics of the existing implementation.  It looks reasonable from a pure-ECMAScript-semantics angle, but if you want any double-checking of ParallelArray-centric semantics, you'll need to find someone else for it.

::: js/src/builtin/ParallelArray.cpp
@@ +214,5 @@
>  
> +static JSBool
> +EmptyDimensions(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    JSObject *empty = NewDenseArrayWithType(cx, 1);

Use a root here, please.

@@ +1772,5 @@
>      }
>  
> +    if (name == cx->runtime->atomState.shapeAtom) {
> +        RootedObject dimArray(cx, as(obj)->dimensionArray());
> +        JSObject *copy = NewDenseCopiedArray(cx, dimArray->getDenseArrayInitializedLength(),

Root this too.

@@ +1812,5 @@
> +        return true;
> +    }
> +
> +    vp.setUndefined();
> +    return true;

*present = false; here.  I'm not sure if it's part of the contract that *present has unspecified value initially, but really we should just assume that -- there's no reason to tempt fate on this, especially since the property/elements change should make this hook go away.

@@ +1958,5 @@
> +    if (IdIsLengthOrShapeAtom(cx, id) || IdIsInBoundsIndex(cx, obj, id)) {
> +        if (strict)
> +            return obj->reportNotConfigurable(cx, id);
> +        if (cx->hasStrictOption())
> +            return obj->reportNotConfigurable(cx, id, JSREPORT_STRICT | JSREPORT_WARNING);

This doesn't look right.  If strict you correctly throw (although I dislike the implication of throwing-ness, optional arguments controlling behavior so significantly here, sigh).  If there's only a strict option, tho, you shouldn't be stopping execution (at least not if the werror option is unset).  Check for !reportNotConfigurable and return false in that case, but if it doesn't fail, you should fall through here.

@@ +2000,5 @@
>                                 AutoIdVector *props)
>  {
>      RootedParallelArrayObject source(cx, as(obj));
>  
> +    if (flags & JSITER_HIDDEN &&

Throw some parentheses around this bitwise-and, please.
Attachment #659612 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 5

6 years ago
Created attachment 663465 [details] [diff] [review]
fix and testcase v3

Reverted decision to make .shape and .length magical [own] properties.

Carrying the r+ as frozen semantics are not changed, and unfortunately I myself am the only person who can review ParallelArray semantics changes.
Attachment #659612 - Attachment is obsolete: true
Attachment #663465 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3b92cb5e3d65
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox18: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Assignee)

Comment 8

6 years ago
Comment on attachment 663465 [details] [diff] [review]
fix and testcase v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 778559
User impact if declined: Crash/read of uninitialized memory
Testing completed (on m-c, etc.): locally and on m-c
Risk to taking this patch (and alternatives if risky): None, localized to a non-publicized and unused-in-the-wild API
String or UUID changes made by this patch:
Attachment #663465 - Flags: approval-mozilla-aurora?

Updated

6 years ago
tracking-firefox17: --- → +
Keywords: regression

Updated

6 years ago
Attachment #663465 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
despite lack of sec-approval+ since this landed on m-c dveditz agreed there was no need to await a landing -- however:

$ hg transplant -s http://hg.mozilla.org/mozilla-central 3b92cb5e3d65
searching for changes
applying 3b92cb5e3d65
patching file js/src/builtin/ParallelArray.cpp
Hunk #1 succeeded at 227 with fuzz 1 (offset 0 lines).
Hunk #2 FAILED at 826
Hunk #3 succeeded at 1054 with fuzz 2 (offset 11 lines).
Hunk #4 FAILED at 1445
Hunk #5 FAILED at 1522
Hunk #6 FAILED at 1604
Hunk #7 FAILED at 1652
Hunk #8 FAILED at 1664
Hunk #9 FAILED at 1673
Hunk #10 FAILED at 1682
Hunk #11 FAILED at 1711
Hunk #13 FAILED at 1767
Hunk #14 FAILED at 1811
Hunk #15 FAILED at 1836
Hunk #16 FAILED at 1902
13 out of 16 hunks FAILED -- saving rejects to file js/src/builtin/ParallelArray.cpp.rej
patching file js/src/builtin/ParallelArray.h
Hunk #1 FAILED at 378
1 out of 1 hunks FAILED -- saving rejects to file js/src/builtin/ParallelArray.h.rej
patching file js/src/jit-test/tests/parallelarray/length-3.js
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file js/src/jit-test/tests/parallelarray/length-3.js.rej
patching file js/src/jit-test/tests/parallelarray/shape-2.js
Hunk #1 FAILED at 6
1 out of 1 hunks FAILED -- saving rejects to file js/src/jit-test/tests/parallelarray/shape-2.js.rej
file js/src/jit-test/tests/parallelarray/shape-4.js already exists
1 out of 1 hunks FAILED -- saving rejects to file js/src/jit-test/tests/parallelarray/shape-4.js.rej
file js/src/jit-test/tests/parallelarray/surfaces-3.js already exists
1 out of 1 hunks FAILED -- saving rejects to file js/src/jit-test/tests/parallelarray/surfaces-3.js.rej
patching file js/src/js.msg
Hunk #1 FAILED at 353
1 out of 1 hunks FAILED -- saving rejects to file js/src/js.msg.rej
patch failed to apply
abort: Fix up the merge and run hg transplant --continue

I'm not about to try untangling the .rej's right now.
(Assignee)

Comment 10

6 years ago
I had landed this already; it slipped my mind to link the commit:

https://hg.mozilla.org/releases/mozilla-aurora/rev/106950

But was I supposed to get sec-approval+? I've been only asking for a? and landing patches for s-s ParallelArray patches that needed to go to aurora.
(Assignee)

Updated

6 years ago
status-firefox17: affected → fixed
Whiteboard: [qa-]
Whiteboard: [qa-] → [qa-][adv-track-main17-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.