Closed
Bug 1357462
Opened 6 years ago
Closed 6 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•6 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•6 years ago
|
||
Test case from comment #0.
Attachment #8859577 -
Flags: review?(jdemooij)
Assignee | ||
Comment 3•6 years ago
|
||
Regression from bug 1283334. Similar to bug 1310744 (also allowed to modify a frozen array), rating probably sec-other.
Comment 4•6 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•6 years ago
|
Attachment #8859577 -
Flags: review?(jdemooij) → review+
Comment 5•6 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•6 years ago
|
||
Can this land?
Assignee | ||
Comment 7•6 years ago
|
||
Do we want/need to backport this to beta/esr52? (Aurora is defunct, right?)
Comment 8•6 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•6 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•6 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•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/27114f4cd6e7
Flags: in-testsuite? → in-testsuite+
Comment 12•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/27114f4cd6e7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•6 years ago
|
Group: javascript-core-security → core-security-release
Comment 13•6 years ago
|
||
I think we're good to nominate this for Beta/ESR52 approval now.
Flags: needinfo?(andrebargull)
Assignee | ||
Comment 14•6 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•6 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•6 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•6 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•6 years ago
|
Attachment #8862108 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment 18•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/ff1cbde07042
Comment 19•6 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr52/rev/38273203b827
Comment 20•6 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•6 years ago
|
Updated•6 years ago
|
Whiteboard: [adv-main54+][adv-esr52.2+]
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•