Closed Bug 1312948 Opened 5 years ago Closed 5 years ago

I can somehow get Array.prototype into a non-freezable state

Categories

(Core :: JavaScript Engine, defect)

51 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- unaffected
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: erights, Assigned: emilio)

References

()

Details

(Keywords: regression)

Attachments

(6 files)

When visiting https://rawgit.com/tvcutsem/es-lab/master/src/ses/contract.html using FF49-release, FF48-developer-edition, or earlier this bug does not occur, as shown on the attached screenshots ff49-release-good and ff48-dev-good.

But when visiting it with FF51 developer edition, by the time this page tries to freeze Array.prototype, it fails to do so in a peculiar way, causing the symptom shown in the ff51-dev-bad screenshot. I have not yet isolated it to something small. 

However what I have found out is that Object.freeze(Array.prototype) (with the original Object.freeze) reports success but leaves Array.prototype.length in the state {value: 0, writable: true, configurable: false}. An Object.defineProperty(Array.prototype, 'length', {writable: false}) (with the original Object.defineProperty) also leaves it in the state {writable: true, ...}. Object.isFrozen(Array.prototype) will correctly give false after these operations fail to lock down the .length property.
Summary: I get somehow get Array.prototype into a non-freezable state → I can somehow get Array.prototype into a non-freezable state
Hmm... so reverting https://hg.mozilla.org/integration/autoland/rev/9cf4fb2f956a fixes it, but it's not totally clear to me why after inspecting the callers, so I'll dig more before proposing that as a fix.
It's also not clear to me what test are they performing yet, so I'll try to reduce that first.
Seems like Object.isFrozen(Array.prototype) returns false even though it's actually frozen and isn't extensible, so IMO this is not a security bug.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> Seems like Object.isFrozen(Array.prototype) returns false even though it's
> actually frozen and isn't extensible, so IMO this is not a security bug.

Er, I mean it's not extensible, I'm still digging, but turns out obj_isFrozen is called a zillion times, so maybe giving up in reducing that wasn't such a good idea.
Ok, got it, will dig into what's the correct solution and post a patch soon. I _think_ (note that I haven't done so much SM stuff) what is happening is the following:

When freezing the prototype, we call DefineProperty on each own property of Array.prototype, since it's in dictionary mode. That includes the "length" property.

Note that at this point the frozen flag is already set in the object due to the PreventExtensions call in <http://searchfox.org/mozilla-central/source/js/src/jsobj.cpp#463>.

Which this means is that now, when calling DefineProperty on the length prototype, lengthIsWritable will return false, and we'll take the fast path in <http://searchfox.org/mozilla-central/source/js/src/jsarray.cpp#595>, which won't update the length shape in <http://searchfox.org/mozilla-central/source/js/src/jsarray.cpp#752>, so the Object.isFrozen call, that iterates over all the property descriptors, will fail.
This reverts the offending patch. The two flags track different spec concepts, so on reflection I think we should keep them separate.
Assignee: nobody → ecoal95
Flags: needinfo?(ecoal95)
Attachment #8804634 - Flags: review?(nicolas.b.pierron)
This puts assertions in place to ensure that we don't mutate the length if the object elements are frozen.
Attachment #8804635 - Flags: review?(nicolas.b.pierron)
Attachment #8804634 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8804635 - Flags: review?(nicolas.b.pierron) → review+
Any chance we could get a test for this?
Flags: in-testsuite?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> Any chance we could get a test for this?

Sure, at first I thought constructing one was going to be hard, because we put objects in dictionary mode depending on heuristics, but after grepping for it I found ecma_5/extensions/watch-setter-become-setter.js, which seems to indicate that deleting a property should work, so I'll try to construct one :)
Attached patch TestSplinter Review
Attachment #8804656 - Flags: review?(nicolas.b.pierron)
Attachment #8804656 - Flags: review?(nicolas.b.pierron) → review+
Thanks!
Hi :emilio,
Since this bug is a regression and also affects 51, do you consider to uplift this for 51 if this patch is not too risky?
Flags: needinfo?(ecoal95)
Comment on attachment 8804634 [details] [diff] [review]
Revert offending patch.

Approval Request Comment
[Feature/regressing bug #]: Bug 1310744
[User impact if declined]: Unknown
[Describe test coverage new/current, TreeHerder]: Tests in this patch set.
[Risks and why]: Low, removes one-liner patch from bug 1310744 that causes one fast path to be taken incorrectly.
[String/UUID change made/needed]: None
Flags: needinfo?(ecoal95)
Attachment #8804634 - Flags: approval-mozilla-aurora?
(In reply to Mark S. Miller from comment #16)
> Thanks!

Thank _you_ for reporting it :)
Flags: in-testsuite? → in-testsuite+
Hi :emilio,
Is this the only patch in uplift request or all of these 3 patches should be in the uplift request?
Flags: needinfo?(ecoal95)
The first one is enough to fix the bug, though the second and third one add debug assertions and tests, so I'd uplift all the three of them, sorry for not specifying it.
Flags: needinfo?(ecoal95)
Comment on attachment 8804634 [details] [diff] [review]
Revert offending patch.

Fix a regression. Take it in 51 aurora.
Attachment #8804634 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: core-security → core-security-release
When can this issue be declassified?

As the one who reported, you now have my consent to declassify at any time.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.