1562493 - AddressSanitizer: heap-use-after-free [@ mozilla::dom::PBrowserBridgeParent::SendFireFrameLoadEvent] with READ of size 4

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P3)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 69.0a1-20190625215814-https://hg.mozilla.org/mozilla-central/rev/b23bd78e4d949c20a5e83f852958cd5fa58e02ea.

For detailed crash information, see attachment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 3

[Andrew McCreight [:mccr8]]

Nika, it looks like you added BrowserParent::RecvFireFrameLoadEvent(). Any ideas?

I'm marking this sec-critical, because I think it is in the parent process. It also involves IPC which might make it more easily triggerable from the child. I'm not sure how exploitable it would actually be, though.

Comment 4

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

Bugbug thinks this bug is a defect, but please change it back in case of error.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrew McCreight [:mccr8] from comment #3)

Nika, it looks like you added BrowserParent::RecvFireFrameLoadEvent(). Any ideas?

I'm marking this sec-critical, because I think it is in the parent process. It also involves IPC which might make it more easily triggerable from the child. I'm not sure how exploitable it would actually be, though.

This code should theoretically be impossible to invoke without fission being enabled, so if we're somehow hitting this without fission being enabled I'm concerned and want to know how! It would be really bad if we're somehow triggering cross-process-iframe without fission being enabled.

In terms of this codepath, as it's fission only, I don't think it should be sec-critical unless we're somehow able to trigger it from non-fission firefox.

---

Other than that, It looks like we incorrectly never null out the BrowserParent -> BrowserBridgeParent reference for fission embedded windows, which should be OK, but the reference is a raw pointer and not a RefPtr<BrowserBridgeParent> despite never being nulled AFAICT . ni? :rhunt who IIRC wrote this code originally. https://searchfox.org/mozilla-central/rev/0671407b7b9e3ec1ba96676758b33316f26887a4/dom/ipc/BrowserParent.h#843

Comment 6

[Andrew McCreight [:mccr8]]

We rate things based on what they would be if enabled, so I'll leave this as sec-critical, but mark it as disabled.

Comment 7

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: Bug 1562493 - Clear out pointer to parent BrowserBridgeParent or BrowserHost when strong-ref from parent to child is released. r?nika

Comment 8

[Ryan Hunt [:rhunt] (on leave until early May)]

This is an error, we should be clearing out this pointer when we release the strong-ref in parent to child. The reason the child to parent link isn't strong is to avoid a cycle. We could probably cycle-collect or something here, but this is the easiest fix for now.

Comment 9

[Christian Holler (:decoder)]

Question for the original reporter: Did you manually enable the fission pref (any of the prefs in about:config with "fission" in it)? It would be great for us to know if this was hit in a standard configuration or if fission is manually enabled.

Comment 10

[Taegeon Lee]

Yes, I enabled all prefs in about:config with "fission" without fission.rebuild_frameloaders_on_remoteness_change.

Comment 11

[Taegeon Lee]

I have not experienced crashes on asan version without change fission prefs.

Comment 12

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

The priority flag is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 13

[Ryan Hunt [:rhunt] (on leave until early May)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc95bdea6a914877a5833b7fe560497b71468114

Comment 14

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 9076394 [details]
Bug 1562493 - Clear out pointer to parent BrowserBridgeParent or BrowserHost when strong-ref from parent to child is released. r?nika

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not trivially. This code is fission-only.
• 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?: None
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• 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?: This patch is low-risk for introducing regressions. It just clears out pointers when BrowserHost/BrowserBridgeHost may be destroyed. All users of this pointer are null-checked.

Comment 15

[Andrew McCreight [:mccr8]]

Comment on attachment 9076394 [details]
Bug 1562493 - Clear out pointer to parent BrowserBridgeParent or BrowserHost when strong-ref from parent to child is released. r?nika

Security fixes that only affect Fission don't need sec-approval. Fission isn't on by default on Nightly, and in fact can only be enabled on Nightly.

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6e684652e73fa1754f75b38bd56214b3fa11e4
https://hg.mozilla.org/mozilla-central/rev/7f6e684652e7