Closed
Bug 1357462
Opened 8 years ago
Closed 8 years ago
Assertion failure: !denseElementsAreFrozen(), at /home/andre/git/mozilla-central/js/src/vm/NativeObject.h:1055
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: anba, Assigned: anba)
References
Details
(Keywords: regression, sec-moderate, Whiteboard: [adv-main54+][adv-esr52.2+])
Attachments
(4 files)
1.23 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
691 bytes,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
1.68 KB,
patch
|
anba
:
review+
gchang
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
1.69 KB,
patch
|
anba
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Test case:
---
var p = Object.freeze(["old-value"]);
var a = Object.setPrototypeOf([], p);
print(p[0]);
print(Reflect.set(a, 0, "new-value", p))
print(p[0]);
---
SetExistingProperty (NativeObject.cpp) needs to check |pobj| is frozen instead of |obj|.
And related:
Why do we call the setter-op for inherited slotless properties with |obj| as the this-value? I'd expect either |pobj| or |receiver|.
Assignee | ||
Comment 1•8 years ago
|
||
We need to check with the object on which the property was found instead of using the object which started the [[Set]] call.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8859305 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•8 years ago
|
||
Test case from comment #0.
Attachment #8859577 -
Flags: review?(jdemooij)
Assignee | ||
Comment 3•8 years ago
|
||
Regression from bug 1283334. Similar to bug 1310744 (also allowed to modify a frozen array), rating probably sec-other.
Comment 4•8 years ago
|
||
Comment on attachment 8859305 [details] [diff] [review]
bug1357462.patch
Review of attachment 8859305 [details] [diff] [review]:
-----------------------------------------------------------------
Good find, thanks!
Attachment #8859305 -
Flags: review?(jdemooij) → review+
Updated•8 years ago
|
Attachment #8859577 -
Flags: review?(jdemooij) → review+
Comment 5•8 years ago
|
||
calling this sec-moderate rather than sec-other on the off chance we rely on freezing objects we inject into content (maybe on android? certainly add-ons might).
Blocks: 1283334
Group: core-security → javascript-core-security
status-firefox53:
--- → affected
status-firefox54:
--- → affected
status-firefox-esr45:
--- → unaffected
status-firefox-esr52:
--- → affected
Keywords: regression,
sec-moderate
Comment 6•8 years ago
|
||
Can this land?
Assignee | ||
Comment 7•8 years ago
|
||
Do we want/need to backport this to beta/esr52? (Aurora is defunct, right?)
Comment 8•8 years ago
|
||
Aurora is gone, yes. And yes, I think we should nominate for Beta/ESR52 as well after it lands on m-c.
Assignee | ||
Comment 9•8 years ago
|
||
Ok, then I'll need to prepare a new patch for esr52, because the current patch only applies to beta, but not esr52.
Assignee | ||
Comment 10•8 years ago
|
||
Combined patch (bug fix + test case) for esr52. Patch is identical to the ones from comment #1 and comment #2, only needed to be updated to apply cleanly on esr52. Carrying r+ from jandem.
Flags: needinfo?(andrebargull)
Attachment #8862108 -
Flags: review+
Comment 11•8 years ago
|
||
Flags: in-testsuite? → in-testsuite+
Comment 12•8 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•8 years ago
|
Group: javascript-core-security → core-security-release
Comment 13•8 years ago
|
||
I think we're good to nominate this for Beta/ESR52 approval now.
Flags: needinfo?(andrebargull)
Assignee | ||
Comment 14•8 years ago
|
||
Combined bug1357462.patch + bug1357462-testcase.patch files for beta approval, carrying r+ because it contains no code changes.
Attachment #8864247 -
Flags: review+
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8864247 [details] [diff] [review]
bug1357462-beta.patch
Approval Request Comment
[Feature/Bug causing the regression]: bug 1283334
[User impact if declined]: Frozen (non-writable + non-configuable) properties in a JavaScript object can be modified.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only changes the lookup object in a single method.
[String changes made/needed]: None.
Flags: needinfo?(andrebargull)
Attachment #8864247 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8862108 [details] [diff] [review]
bug1357462-esr52.patch
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Frozen (non-writable + non-configuable) properties in a JavaScript object can be modified.
Fix Landed on Version: mozilla55
Risk to taking this patch (and alternatives if risky): No risks are known.
String or UUID changes made by this patch: None.
Attachment #8862108 -
Flags: approval-mozilla-esr52?
Comment 17•8 years ago
|
||
Comment on attachment 8864247 [details] [diff] [review]
bug1357462-beta.patch
Fix a security issue and should be low risk. Beta54+ & ESR52+. Should be in 54 beta 5.
Attachment #8864247 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•8 years ago
|
Attachment #8862108 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment 18•8 years ago
|
||
uplift |
Comment 19•8 years ago
|
||
Comment 20•8 years ago
|
||
(In reply to André Bargull from comment #15)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.
Setting qe-verify- based on André's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Updated•8 years ago
|
Updated•8 years ago
|
Whiteboard: [adv-main54+][adv-esr52.2+]
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•