Closed Bug 1312609 Opened 3 years ago Closed 3 years ago

make sure rootViewParent pointer is valid in all cases in nsDocShell::RestoreFromHistory because it may be destroyed

Categories

(Core :: DOM: Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox49 --- wontfix
firefox-esr45 50+ fixed
firefox50 + fixed
firefox51 + fixed
firefox52 + fixed
firefox53 + fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main50.1+][adv-esr45.6+])

Attachments

(1 file)

No description provided.
Attached patch patchSplinter Review
Attachment #8804107 - Flags: review?(bugs)
Comment on attachment 8804107 [details] [diff] [review]
patch

So this related to bug 725770.
Attachment #8804107 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (reviewing overload, queue closed for a day or two) from comment #2)
> Comment on attachment 8804107 [details] [diff] [review]
> patch
> 
> So this related to bug 725770.

Yes, thanks for finding the history.
Comment on attachment 8804107 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
not too easily, there are some clues that would be helpful: that we are in a function called RestoreFromHistory and are clearing a variable called rootViewParent might point in the direction to go, but there is still a long ways to go, and I'm not even sure that we can reach this situation at all

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
no

Which older supported branches are affected by this flaw?
all

If not all supported branches, which bug introduced the flaw?
bug 725770

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
trivial

How likely is this patch to cause regressions; how much testing does it need?
not likely at all, can be uplifted immediately
Attachment #8804107 - Flags: sec-approval?
We need a security rating on this before I can approve (and it is too late to make 50 anyway). What are the implications of this bug? Bug 725770 is a sec-critical bug.
(In reply to Al Billings [:abillings] from comment #5)
> We need a security rating on this before I can approve (and it is too late
> to make 50 anyway). What are the implications of this bug? Bug 725770 is a
> sec-critical bug.

This bug would be sec-critical too if it is indeed possible to trigger it.
Group: core-security → dom-core-security
sec-approval+ for checkin to trunk but not until 11/29, two weeks into the next cycle since we weren't able to take this into 50. We'll want to take Beta, Aurora, and ESR45 patches after trunk is green as well.
Whiteboard: [checkin on 11/29]
Attachment #8804107 - Flags: sec-approval? → sec-approval+
Should we be considering this for the 50.1 release?
Flags: needinfo?(tnikkel)
Whiteboard: [checkin on 11/29]
I don't really know what kind of thing we are looking to include in 50.1, so that question might be better directed at a release manager.
Flags: needinfo?(tnikkel)
I guess we can nominate it for now and let them decide then :)
https://hg.mozilla.org/mozilla-central/rev/65fa05989b39
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Please nominate this for Aurora/Beta/Release/ESR45 approval ASAP so we can hopefully get it uplifted in time for 51b6.
Flags: needinfo?(tnikkel)
Comment on attachment 8804107 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: potentially a dangling pointer (if this code path is possible to hit)
Fix Landed on Version: 53
Risk to taking this patch (and alternatives if risky): not risky
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: bug 725770
[User impact if declined]: potentially a dangling pointer (if this code path is possible to hit)
[Is this code covered by automated tests?]: no (not sure this code path is possible to be hit)
[Has the fix been verified in Nightly?]: not sure this code path can be hit, so can't really verify it
[Needs manual test from QE? If yes, steps to reproduce]: no, see answer to previous question
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: using null instead of a pointer that may be dangling
[String changes made/needed]: none
Flags: needinfo?(tnikkel)
Attachment #8804107 - Flags: approval-mozilla-release?
Attachment #8804107 - Flags: approval-mozilla-esr45?
Attachment #8804107 - Flags: approval-mozilla-beta?
Attachment #8804107 - Flags: approval-mozilla-aurora?
Comment on attachment 8804107 [details] [diff] [review]
patch

Fix a sec-high. Beta51+, Aurora52+, and ESR45+. Should be in 51 beta 6.
NI Ritu for awareness.
Flags: needinfo?(rkothari)
Attachment #8804107 - Flags: approval-mozilla-esr45?
Attachment #8804107 - Flags: approval-mozilla-esr45+
Attachment #8804107 - Flags: approval-mozilla-beta?
Attachment #8804107 - Flags: approval-mozilla-beta+
Attachment #8804107 - Flags: approval-mozilla-aurora?
Attachment #8804107 - Flags: approval-mozilla-aurora+
Tracking 53+ for this sec high issue.
Flags: needinfo?(rkothari)
Comment on attachment 8804107 [details] [diff] [review]
patch

Sec-high, meets the triage bar for inclusion in 50.1.0
Attachment #8804107 - Flags: approval-mozilla-release? → approval-mozilla-release+
Whiteboard: [adv-main50.1+]
Whiteboard: [adv-main50.1+] → [adv-main50.1+][adv-esr45.6+]
Group: dom-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.