1524214 - Could be use-after-free of `capturingContent` in PresShell::HandleEvent()

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

While I'm working on bug 1466208, I found this bug.

Here is nsIContent* variable:
https://searchfox.org/mozilla-central/rev/78cd247b5d7a08832f87d786541d3e2204842e8e/layout/base/PresShell.cpp#6523-6527

And this will be referred here:
https://searchfox.org/mozilla-central/rev/78cd247b5d7a08832f87d786541d3e2204842e8e/layout/base/PresShell.cpp#6645-6648

However, immediately before that, some script may be run with a call of Document::UnlockPointer() and a call of FlushThrottledStyles().

https://searchfox.org/mozilla-central/rev/78cd247b5d7a08832f87d786541d3e2204842e8e/layout/base/PresShell.cpp#6616,6620-6624,6645-6648

So, capturingContent should be nsCOMPtr<nsIContent>.

I don't know whether this causes actual security issue or not, though.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1524214 - Grab caching capturing content with local variable

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1524214 - Grab caching capturing content with local variable (for beta, release and esr60)

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9040384 [details]
Bug 1524214 - Grab caching capturing content with local variable

Security Approval Request

How easily could an exploit be constructed based on the patch?

I guess that just to make crash the user's browser, it might be easy to do that with removing the DOM node from script which is kicked by flushing animation. However, it might be no problem if somebody else grabs the DOM node with a strong pointer, 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

Which older supported branches are affected by this flaw?

ESR60

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

None

Do you have backports for the affected branches?

Yes

If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

Won't cause any regressions due to switching a raw pointer variable to a strong pointer variable. But the patch is too simple. Therefore, I have no idea to hide the change from commit message. So, it might make the attackers realize what is problem.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9040385 [details]
Bug 1524214 - Grab caching capturing content with local variable (for beta, release and esr60)

Security Approval Request

How easily could an exploit be constructed based on the patch?

I guess that just to make crash the user's browser, it might be easy to do that with removing the DOM node from script which is kicked by flushing animation. However, it might be no problem if somebody else grabs the DOM node with a strong pointer, 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

Which older supported branches are affected by this flaw?

ESR60

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

None

Do you have backports for the affected branches?

Yes

If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

Won't cause any regressions due to switching a raw pointer variable to a strong pointer variable. But the patch is too simple. Therefore, I have no idea to hide the change from commit message. So, it might make the attackers realize what is problem.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9040385 [details]
Bug 1524214 - Grab caching capturing content with local variable (for beta, release and esr60)

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

Potential use-after-free risk at handling most events. After caching the pointing device capturing DOM node, some DOM events may be fired and then, we refer the cached node.

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

No

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Just switching a raw pointer to a strong pointer.

String changes made/needed

none.

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration

This is sec-high.

User impact if declined

See request for Beta/Release Uplift Approval Request.

Fix Landed on Version

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

See request for Beta/Release Uplift Approval Request.

String or UUID changes made by this patch

none.

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 9040384 [details]
Bug 1524214 - Grab caching capturing content with local variable

sec-approval+ for trunk.

I'm not sure why approval for release has been requested. I'll approve for beta as well.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm not sure why approval for release has been requested. I'll approve for beta as well.

I meant it for dot release because of really safe fix.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/e53f607940cb5e1db1bb7eeb3c5c0bc6f4b0d608
https://hg.mozilla.org/mozilla-central/rev/e53f607940cb

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/c5a3a2a563c9

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9040385 [details]
Bug 1524214 - Grab caching capturing content with local variable (for beta, release and esr60)

I don't think we need to go out of our way to ship this in a dot release. Let's let it ride with 66/60.6esr.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9040385 [details]
Bug 1524214 - Grab caching capturing content with local variable (for beta, release and esr60)

Sec-high fix. Approved for 60.6esr.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/17c3615c2585