Closed Bug 998355 Opened 10 years ago Closed 10 years ago

WeakMap::markIteratively calls front() after rekeyFront()

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

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)

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.
Terrence, how bad is this?
Flags: needinfo?(terrence)
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)
Attached patch weakmap_fix-v0.diff (obsolete) — Splinter Review
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: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8411391 - Flags: review?(jcoppeard)
Actually, I guess that's only the new crashes since ggc, which is a fairly small number still.
(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. :)
Andrew could this bug or bug 982561 be related to bug 966490 somehow?
Attachment #8411391 - Flags: review?(jcoppeard) → review+
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.
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+
Keywords: checkin-needed
Keywords: sec-highsec-other
https://hg.mozilla.org/mozilla-central/rev/5c026fd7e73c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Marking [qa-] based on lack of exploitability/ease of verification.
Whiteboard: [qa-]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.