Closed
Bug 827596
Opened 12 years ago
Closed 12 years ago
Assigning the same pointer value to an nsAutoPtr shouldn't crash exploitably
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
People
(Reporter: jruderman, Assigned: benjamin)
Details
(Keywords: sec-want, Whiteboard: [adv-main20+])
Attachments
(2 files)
|
1015 bytes,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
|
1.06 KB,
patch
|
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
nsAutoPtr prematurely frees its pointer if you assign "x = y;" when x is already equal to y.
Recently we've had some security bugs due to this behavior (bug 723446 and bug 827591). Bug 723446 added a debug-only assertion to notice when this happens, and there was some discussion in that bug about changing the behavior.
"Smart" pointer classes shouldn't be such bad footguns, IMO.
So if you're assigning something to an nsAutoPtr you're asserting that you, uniquely, own that object, and are transferring that ownership to the nsAutoPtr. If the nsAutoPtr already owns that object, how could you possibly make such an assertion...unless you know for sure you're not changing anything, in which case why bother with the assignment?
I guess I'd be ok with making the behavior safer... but I'd be pretty uncomfortable removing the assertion.
Comment 2•12 years ago
|
||
Hmm, the branch mentioned in bug 723446 comment 22 should be a well-predicted one, right?
Updated•12 years ago
|
Group: core-security
Comment 3•12 years ago
|
||
Why would we remove the assertion? if anything we should change it to an abort unless we make it safe. If we make the operation safe we could leave the assertion as a warning that you're doing something unexpected.
Assignee: nobody → benjamin
Updated•12 years ago
|
tracking-firefox21:
--- → +
| Assignee | ||
Comment 4•12 years ago
|
||
Attachment #703539 -
Flags: review?(dbaron)
Comment on attachment 703539 [details] [diff] [review]
runtime abort instead, rev. 1
If you make this compile by moving it below the declaration of oldPtr, then r=dbaron
(Though the branch might be better predicted if you swap the order around the &&; I'm not sure, though.)
Attachment #703539 -
Flags: review?(dbaron) → review+
| Assignee | ||
Comment 6•12 years ago
|
||
How easily could an exploit be constructed based on the patch?: As a defensive measure, there is only evidence that somewhere we've had problems with same-pointer assignment. Bug 723446 has already been made public and bug 827591 has already landed in beta.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Not any more than the patch itself does.
Which older supported branches are affected by this flaw? As a defensive measure, we don't *have* to apply it to any older branches, but it should also be safe to do so if we want.
Do you have backports for the affected branches? This patch will probably apply directly to all the branches we care about.
How likely is this patch to cause regressions; how much testing does it need? This patch is unlikely to cause any regressions. It is vaguely possible for it to cause a peformance regression, but I consider that very unlikely.
Attachment #705342 -
Flags: sec-approval?
Updated•12 years ago
|
status-firefox18:
--- → affected
status-firefox19:
--- → affected
status-firefox20:
--- → affected
status-firefox21:
--- → affected
status-firefox-esr17:
--- → affected
tracking-firefox19:
--- → ?
tracking-firefox20:
--- → ?
tracking-firefox-esr17:
--- → ?
Updated•12 years ago
|
Attachment #705342 -
Flags: sec-approval? → sec-approval+
Comment 7•12 years ago
|
||
Based on comment 6 there's no rush to take this for Beta (19) but we could take an uplift to Aurora (20). This doesn't meet ESR landing criteria so unless the user base is clamouring for this fix we won't take it there.
Comment 8•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/70c64112a171
Is it possible to test this?
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
| Assignee | ||
Comment 9•12 years ago
|
||
It would not be worthwhile, I think. We'd have to do a binary-code test, and we're testing a condition that is a programming error anyway.
Flags: in-testsuite? → in-testsuite-
Updated•12 years ago
|
status-b2g18:
--- → wontfix
Comment 10•12 years ago
|
||
If this fix is low-risk enough please nominate for Beta approval so it can land in the first couple of FF20 beta builds.
Comment 11•12 years ago
|
||
Bsmedberg - can we uplift this on Monday and ship it in the FF 20 beta 2?
Flags: needinfo?(benjamin)
| Assignee | ||
Comment 12•12 years ago
|
||
Flags: needinfo?(benjamin)
Updated•12 years ago
|
Whiteboard: [adv-main20+]
Updated•12 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•