Closed Bug 827596 Opened 7 years ago Closed 7 years ago
Assigning the same pointer value to an ns
Auto Ptr shouldn't crash exploitably
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.
Hmm, the branch mentioned in bug 723446 comment 22 should be a well-predicted one, right?
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
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+
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?
Attachment #705342 - Flags: sec-approval? → sec-approval+
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.
https://hg.mozilla.org/mozilla-central/rev/70c64112a171 Is it possible to test this?
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-
If this fix is low-risk enough please nominate for Beta approval so it can land in the first couple of FF20 beta builds.
Bsmedberg - can we uplift this on Monday and ship it in the FF 20 beta 2?
You need to log in before you can comment on or make changes to this bug.