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

RESOLVED FIXED in Firefox 47

Status

()

Core
Layout
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

(Blocks: 1 bug, {sec-moderate})

unspecified
mozilla48
sec-moderate
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 wontfix, firefox46 wontfix, firefox47+ fixed, firefox48 fixed, firefox-esr38 wontfix, firefox-esr4547+ fixed)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

a year ago
Created attachment 8736993 [details] [diff] [review]
patch

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)
(Assignee)

Comment 2

a year ago
Created attachment 8737291 [details] [diff] [review]
patch v2

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+
(Assignee)

Comment 4

a year ago
(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.
(Assignee)

Comment 5

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dd9ea4b7f61
(Assignee)

Comment 6

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff41464ac92d19c020112093a16ca4b364a7ad82
https://hg.mozilla.org/mozilla-central/rev/ff41464ac92d
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
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
status-firefox45: --- → wontfix
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox-esr38: --- → affected
status-firefox-esr45: --- → affected
(Assignee)

Comment 9

a year ago
(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.
(Assignee)

Comment 10

a year ago
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?
Keywords: sec-high
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+
tracking-firefox47: --- → +
(Assignee)

Comment 12

a year ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/f80143d36adf
status-firefox47: affected → fixed

Updated

a year ago
Blocks: 1261175
Blocks: 775965
status-firefox46: affected → wontfix
status-firefox-esr38: affected → wontfix
status-firefox-esr45: affected → wontfix
Keywords: sec-high → sec-moderate
Whiteboard: potential UAF of poisoned nsView
(Assignee)

Comment 15

a year ago
Created attachment 8751042 [details] [diff] [review]
patch for esr45

[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?

Updated

a year ago
status-firefox-esr45: wontfix → affected
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+
https://hg.mozilla.org/releases/mozilla-esr45/rev/4290826b078c
status-firefox-esr45: affected → fixed

Updated

11 months ago
tracking-firefox-esr45: --- → 47+
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.