Closed Bug 1562493 Opened 3 years ago Closed 3 years ago

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

Categories

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

x86_64
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- disabled
firefox70 --- disabled

People

(Reporter: decoder, Assigned: rhunt)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [probably Fission-only])

Attachments

(2 files)

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.

Flags: sec-bounty?

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.

Group: core-security → dom-core-security
Component: DOM: Events → DOM: Content Processes
Flags: needinfo?(nika)

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

Type: -- → defect

(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

Flags: needinfo?(nika) → needinfo?(rhunt)

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

Whiteboard: [probably Fission-only]

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.

Flags: needinfo?(rhunt)

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.

Flags: needinfo?(ash153311)

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

Flags: needinfo?(ash153311)

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

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)
Priority: -- → P3
Attachment #9076394 - Attachment description: Bug 1562493 - Clear out pointer to parent BrowserBridgeParent when strong-ref from parent to child is released. r?nika → Bug 1562493 - Clear out pointer to parent BrowserBridgeParent or BrowserHost when strong-ref from parent to child is released. r?nika

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.
Attachment #9076394 - Flags: sec-approval?

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.

Attachment #9076394 - Flags: sec-approval?
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Assignee: nobody → rhunt
Flags: sec-bounty? → sec-bounty+
Keywords: sec-criticalsec-high
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.