Closed
Bug 570657
Opened 14 years ago
Closed 14 years ago
nsCOMArray is inconsistent with the order in which it releases and removes elements from the array, which can cause a refcount to be decreased more than necessary
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
()
Details
(Whiteboard: [sg:critical] [qa-ntd-192] [qa-ntd-191])
Attachments
(1 file, 2 obsolete files)
6.15 KB,
patch
|
shaver
:
review+
dveditz
:
approval1.9.2.7+
christian
:
approval1.9.1.11+
|
Details | Diff | Splinter Review |
See bug 570350 comment 6 about what caused me to discover this issue. When nsCOMArray::Clear is called, the interfaces are released *before* they're actually removed from the array. So, given an interface which calls nsCOMArray::RemoveObject (or other similar methods) from its Release method, you could get into situations where an object is attempted to be removed twice, and therefore decrementing its refcount more than necessary. I also see similar things in other methods handling removing items from nsCOMArrays. I think we should have a well-defined order of "removing the object from the array and then calling Release on it, in order to avoid such problems. The reason that I'm filing this in the security group is that it is (theoretically) possible to study our code to determine places in the code where this might happen (either now or in the future) and then construct a scenario when this caused an object to be deleted with outstanding interface pointers to it, and then use the dangling pointer in combination with other techniques to construct an attach scenario.
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•14 years ago
|
||
Actually, make sure in the test that we never manage to remove the element from the destructor.
Attachment #449799 -
Attachment is obsolete: true
Attachment #449800 -
Flags: review?(shaver)
Attachment #449799 -
Flags: review?(shaver)
Updated•14 years ago
|
Whiteboard: [sg:critical][critsmash:patch]
Assignee | ||
Comment 3•14 years ago
|
||
I have *no* idea how this test was passing for me last night. But now the patch is actually correct, and passes the tests!
Attachment #449800 -
Attachment is obsolete: true
Attachment #449921 -
Flags: review?(shaver)
Attachment #449800 -
Flags: review?(shaver)
Assignee | ||
Updated•14 years ago
|
Attachment #449921 -
Attachment description: Patch (v1) → Patch (v1.2)
Comment on attachment 449921 [details] [diff] [review] Patch (v1.2) r=shaver
Attachment #449921 -
Flags: review?(shaver) → review+
Assignee | ||
Comment 5•14 years ago
|
||
The fix here is localized and safe, and the exact threat level is not something which can be evaluated with certainty, so I'd recommend to block on this for all branches as well as 1.9.3.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Updated•14 years ago
|
blocking2.0: ? → beta1+
Assignee | ||
Comment 6•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/af44e6577313
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [sg:critical][critsmash:patch] → [sg:critical]
Target Milestone: --- → mozilla1.9.3a5
Assignee | ||
Updated•14 years ago
|
Attachment #449921 -
Flags: approval1.9.2.5?
Attachment #449921 -
Flags: approval1.9.1.11?
Comment on attachment 449921 [details] [diff] [review] Patch (v1.2) a=LegNeato for 1.9.2.6 and 1.9.1.11. Please land this on mozilla-1.9.2 default and mozilla-1.9.1 default.
Attachment #449921 -
Flags: approval1.9.2.5?
Attachment #449921 -
Flags: approval1.9.2.5+
Attachment #449921 -
Flags: approval1.9.1.11?
Attachment #449921 -
Flags: approval1.9.1.11+
Updated•14 years ago
|
status1.9.1:
--- → wanted
status1.9.2:
--- → wanted
Updated•14 years ago
|
Attachment #449921 -
Flags: approval1.9.2.5+ → approval1.9.2.6+
Assignee | ||
Comment 8•14 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b1f21f894e9d http://hg.mozilla.org/releases/mozilla-1.9.1/rev/12a069c6f52d
Comment 9•14 years ago
|
||
Two questions, Ehsan: 1) Are there no tests to be checked in for 1.9.2 and 1.9.1? I see that there is a test running on builds in mozilla central. 2) Since this is a security bug, shouldn't the tests for mozilla central not be checked in yet, per policy, until we unhide the bug? For QA, I'm trying to figure out what needs to be done to verify this bug on branches.
Whiteboard: [sg:critical] → [sg:critical] [qa-examined-192] [qa-examined-191]
Assignee | ||
Comment 10•14 years ago
|
||
Unfortunately (or maybe fortunately?) we don't have any STRs to exploit this problem. I found out about this problem while working on the editor using a half-baked patch, but that patch was never committed to any repository. The tests for this bug only test the implementation, and do not uncover any potential attacks. My mozilla-central patch modified the existing nsCOMArray test, but unfortunately that test does not exist on our branches. Frankly, I can't think of any way to verify this except for reading the code and reasoning about it, but I'm not sure if that's good enough for your verification purposes.
Comment 11•14 years ago
|
||
Nothing for QA to do further here then.
Whiteboard: [sg:critical] [qa-examined-192] [qa-examined-191] → [sg:critical] [qa-ntd-192] [qa-ntd-191]
Updated•14 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•