Closed Bug 1261230 Opened 4 years ago Closed 4 years ago

be sure to clear stashed views when reconstructing/destroying a subdocument frame

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 + fixed
firefox48 --- fixed
firefox-esr38 --- wontfix
firefox-esr45 47+ fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main47+][adv-esr45.2+] potential UAF of poisoned nsView)

Attachments

(2 files, 1 obsolete file)

No description provided.
Attached patch patch (obsolete) — Splinter Review
The change in nsHideViewer::Run is the only one I think it might be possible to trigger. The rest are good hygiene I think.
Attachment #8736993 - Flags: review?(mats)
Attached patch patch v2Splinter Review
Also call Hide on the frameloader if adding the script runner failed.
Attachment #8736993 - Attachment is obsolete: true
Attachment #8736993 - Flags: review?(mats)
Attachment #8737291 - Flags: review?(mats)
Group: core-security → layout-core-security
Comment on attachment 8737291 [details] [diff] [review]
patch v2

>-               nsFrameLoader* aFrameLoader,
>+               RefPtr<nsFrameLoader>& aFrameLoader,

This seems a little unnecessary, but fine.  If you keep it, then it should be
"const RefPtr<nsFrameLoader>&" though.

>+    bool succeeded =
>+      nsContentUtils::AddScriptRunner(
>+        new nsHideViewer(mContent,

Good point on the return value, but AddScriptRunner is actually infallible.
It's using nsTArray internally and it only returns false on OOM.
So instead of doing this part, I think it would be better to file
a follow-up bug to make AddScriptRunner return void.
Attachment #8737291 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #3)
> >+               RefPtr<nsFrameLoader>& aFrameLoader,
> 
> This seems a little unnecessary, but fine.  If you keep it, then it should be
> "const RefPtr<nsFrameLoader>&" though.

Removed it.

> Good point on the return value, but AddScriptRunner is actually infallible.
> It's using nsTArray internally and it only returns false on OOM.
> So instead of doing this part, I think it would be better to file
> a follow-up bug to make AddScriptRunner return void.

Followup bug 1261499.
https://hg.mozilla.org/mozilla-central/rev/ff41464ac92d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
This code seems to go back at least to the still-supported ESR-38 branch so we need a severity evaluation: please retroactively request sec-approval on the patch and fill out the info. Looks like a triggerable use-after-free?
Group: layout-core-security → core-security-release
(In reply to Daniel Veditz [:dveditz] from comment #8)
> This code seems to go back at least to the still-supported ESR-38 branch so
> we need a severity evaluation: please retroactively request sec-approval on
> the patch and fill out the info. Looks like a triggerable use-after-free?

Yes, the code is very old.

This was found by code inspection, so there is no proof the condition can be triggered.

This would be a use-after-free, but the thing we "use" is a nsView, which we poison on destruction.
Comment on attachment 8737291 [details] [diff] [review]
patch v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
not sure if it's possible, it points to where one might start though


Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
no, if they did I'd have a test case :)


Which older supported branches are affected by this flaw?
all

If not all supported branches, which bug introduced the flaw?
bug 775965 on Firefox 17

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
Attachment #8737291 - Flags: sec-approval?
Comment on attachment 8737291 [details] [diff] [review]
patch v2

sec-approval+ for trunk but I don't think we'll take this on beta or ESR since we have no trigger for it. Please nominate a patch for Aurora though.
Attachment #8737291 - Flags: sec-approval? → sec-approval+
Comment on attachment 8737291 [details] [diff] [review]
patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 775965
[User impact if declined]: maybe a crash, if this condition is possible
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low risk, just clears stashed views, we handle the case where there are stashed views and no stashed views already
[String/UUID change made/needed]: none
Attachment #8737291 - Flags: approval-mozilla-aurora?
Comment on attachment 8737291 [details] [diff] [review]
patch v2

Sec-high, Aurora47+
Attachment #8737291 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1261175
Blocks: 775965
Keywords: sec-highsec-moderate
Whiteboard: potential UAF of poisoned nsView
Attached patch patch for esr45Splinter Review
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: bug 1265577 (sec-crit we want for esr45) touches a lot of the same lines as this patch. less risky to uplift this fix the re-write bug 1265577.
User impact if declined: maybe security crashes?
Fix Landed on Version: 48
Risk to taking this patch (and alternatives if risky): this is the least risk option. should be pretty safe.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8751042 - Flags: approval-mozilla-esr45?
Hi Tim, we also need a patch for Beta47. Could you please nominate one for uplift?
Flags: needinfo?(tnikkel)
(In reply to Ritu Kothari (:ritu) from comment #16)
> Hi Tim, we also need a patch for Beta47. Could you please nominate one for
> uplift?

My bad! This is already fixed in Fx47.
Flags: needinfo?(tnikkel)
Comment on attachment 8751042 [details] [diff] [review]
patch for esr45

This patch is needed for a sec-crit bug, ESR45+
Attachment #8751042 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Whiteboard: potential UAF of poisoned nsView → [adv-main47+][adv-esr45.2+] potential UAF of poisoned nsView
Whiteboard: [adv-main47+][adv-esr45.2+] potential UAF of poisoned nsView → [post-critsmash-triage][adv-main47+][adv-esr45.2+] potential UAF of poisoned nsView
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.