1649228 - Crash in mozilla::dom::ContentChild::RecvNotifyAlertsObserver caused by bug 1642991

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Emilio Cobos Álvarez [:emilio]]

Sebastian Simon helpfully came up with the STR for a crash in bug 1427459, which is a recent regression introduced in bug 1642991.

It is a pre-existing bug in RemoveElementsBy.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Hmm, maybe my diagnostic is wrong, let me build without optimizations to double-check me.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: Test-case

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Looks like an UAF. :(

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: ASAN log

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1649228 - Fix ContentChild::RecvNotifyAlertsObserver to notify after, not while, removing observers from the vector. r=froydnj,smaug

This probably had logic issues before bug 1642991, but not security
issues (at worst, an array out of bounds which is a release assertion
that would crash the process in a safe way).

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9160187 [details]
Bug 1649228 - Fix ContentChild::RecvNotifyAlertsObserver to notify after, not while, removing observers from the vector. r=froydnj,smaug

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Relatively easy, the patch is very obvious.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: beta
• If not all supported branches, which bug introduced the flaw?: Bug 1642991
• Do you have backports for the affected branches?: Yes, applies cleanly on beta
• 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?: Not likely at all. It's more correct than the code that used to be before bug 1642991.

Comment 9

[Daniel Veditz [:dveditz]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

• Which older supported branches are affected by this flaw?: none, nightly-only
• If not all supported branches, which bug introduced the flaw?: Bug 1642991

I think the merge already happened. The bug is effectively on Beta now and we'll need to uplift the patch.

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Ah, true... I edited the form.

Comment 11

[Simon Giesecke [:sg] [he/him]]

The assertion suggested to be added in Bug 1649770 would probably have caught this earlier on.

Comment 12

[Tom Ritter [:tjr]]

Comment on attachment 9160187 [details]
Bug 1649228 - Fix ContentChild::RecvNotifyAlertsObserver to notify after, not while, removing observers from the vector. r=froydnj,smaug

Approved to land and uplift

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/1de85484b75c90e5d34a70089b3b921d39b22858

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/826468e886b2

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/1de85484b75c

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 17

[Emilio Cobos Álvarez [:emilio]]

done

Comment 18

[Ciprian Georgiu, Desktop QA]

I've reproduced this crash using an affected Nightly asan build from 2020-07-02, and by loading into the browser the test case from comment 2.

The crash is verified as fixed on the latest asan builds, Nightly 80.0a1 and Beta 79.0b9, running Ubuntu 18.04 x64.