Closed
Bug 998355
Opened 10 years ago
Closed 10 years ago
WeakMap::markIteratively calls front() after rekeyFront()
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla31
Tracking | Status | |
---|---|---|
firefox30 | --- | unaffected |
firefox31 | --- | fixed |
firefox-esr24 | --- | unaffected |
People
(Reporter: mccr8, Assigned: terrence)
References
Details
(Keywords: sec-other, Whiteboard: [qa-])
Attachments
(1 file, 1 obsolete file)
1.25 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
191 } else if (keyNeedsMark(key)) { 192 gc::Mark(trc, &key, "proxy-preserved WeakMap entry key"); 193 if (e.front().key() != key) 194 entryMoved(e, key); 195 gc::Mark(trc, &e.front().value(), "WeakMap entry value"); entryMoved() calls rekeyFront() on the iterator. rekeyFront() says "|front()| is invalid after this operation until the next call to |popFront()|.". The solution to this is just to mark value() before key(). The other calls to entryMoved() look okay to me. Marked s-s because I'm not sure what the ramifications of this are.
Assignee | ||
Comment 2•10 years ago
|
||
Depends on the map, but if it's Object:Object, it's pretty bad, I think. |rekey| calls HashTableEntry::clearEntry, which calls ~HashMapEntry, which is the default destructor, which I just leaves the pointers alone. So marking the value with e.front() would succeed, but update the dead table entry, leaving the new entry pointing at the dead object slot. This would allow a subsequent read on that key to gain r/w access to re-used memory. Probably fairly hard to exploit, but not harder than other exploits we've been burned by. Of course if the value type is not object, this is more or less safe as the new and old pointer will be the same.
Flags: needinfo?(terrence)
Assignee | ||
Comment 3•10 years ago
|
||
This is almost certainly bug 984101 and all previous use-after-free through a weakmap that have been plaguing us. Andrew, how did you find this?
Assignee | ||
Comment 4•10 years ago
|
||
Actually, I guess that's only the new crashes since ggc, which is a fairly small number still.
Reporter | ||
Comment 5•10 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #3) > Andrew, how did you find this? I was just reading the weakmap code very closely while investigating bug 982561. :)
Comment 6•10 years ago
|
||
Andrew could this bug or bug 982561 be related to bug 966490 somehow?
Updated•10 years ago
|
Attachment #8411391 -
Flags: review?(jcoppeard) → review+
Comment 7•10 years ago
|
||
I'm probably missing something but I don't see how this can manifest. This code is in WeakMap::markIteratively() which is only called during full GC. At the moment any moving has to happen in a previous minor GC, which will call nonMarkingTraceValues() and nonMarkingTraceKeys() to mark any weak maps encountered.
Assignee | ||
Comment 8•10 years ago
|
||
You are quite right! Still, let's get this in to prevent future mistakes. At the least it will be chaff for the stream of other sec fixes getting pushed right now.
Attachment #8411391 -
Attachment is obsolete: true
Attachment #8411913 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Updated•10 years ago
|
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c026fd7e73c
Keywords: checkin-needed
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5c026fd7e73c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Updated•10 years ago
|
status-firefox-esr24:
--- → unaffected
Comment 11•10 years ago
|
||
Marking [qa-] based on lack of exploitability/ease of verification.
Whiteboard: [qa-]
Updated•9 years ago
|
Group: core-security → core-security-release
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
•