Closed Bug 1769635 Opened 2 years ago Closed 1 year ago

Crash in mozilla::css::SheetLoadData::~SheetLoadData (dropping listener without notifying)

Categories

(Core :: Networking, defect, P2)

Product:
Core
Component:
Networking
Platform:
x86_64
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
110 Branch
Tracking Flags:
Tracking Status
firefox-esr102 109+ fixed
firefox108 --- wontfix
firefox109 + fixed
firefox110 + fixed

People

(Reporter: thee.chicago.wolf, Assigned: kershaw)

References

Details

(Keywords: crash, csectype-uaf, csectype-wildptr, Whiteboard: [tbird crash][necko-triaged][necko-priority-queue])

Crash Data

Attachments

(1 file)

Bug 1769635 - Make sure AsyncAbort is always called, r=#necko
48 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
RyanVM
: approval-mozilla-esr102+
tjr
: sec-approval+
Details | Review

No STR that I can provide. I remember that before the crash I had clicked on a newly arrived and un-read message.

Crash is at: https://crash-stats.mozilla.org/report/index/78f70879-b072-460d-a230-c11d90220516

Don't know if it's related to bug 1221902 but I noted it there too.

There are more Firefox crashes than Thunderbird, but quite rare for both.
For example bp-c5257b57-d1e7-4660-9b37-d5dd40220507

Please use the "create a bug" link listed in the crash report page at https://crash-stats.mozilla.org/report/index/78f70879-b072-460d-a230-c11d90220516#tab-bugzilla so that it builds a full picture in the bug report.

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(mSheetCompleteCalled || mIntentionallyDropped) (Should always call SheetComplete, except when dropping the load)

Top 10 frames of crashing thread:

0 xul.dll mozilla::css::SheetLoadData::~SheetLoadData layout/style/Loader.cpp:386
1 xul.dll mozilla::css::SheetLoadData::Release layout/style/Loader.cpp:264
2 xul.dll mozilla::css::StreamLoader::~StreamLoader layout/style/StreamLoader.cpp:24
3 xul.dll CNavDTD::Release parser/htmlparser/CNavDTD.cpp:13
4 xul.dll mozilla::net::HttpBaseChannel::~HttpBaseChannel netwerk/protocol/http/HttpBaseChannel.cpp:252
5 xul.dll mozilla::net::nsHttpChannel::~nsHttpChannel netwerk/protocol/http/nsHttpChannel.cpp:315
6 xul.dll mozilla::net::HttpBaseChannel::Release netwerk/protocol/http/HttpBaseChannel.cpp:396
7 xul.dll mozilla::net::nsLoadGroup::Cancel netwerk/base/nsLoadGroup.cpp:241
8 xul.dll nsDocLoader::Stop uriloader/base/nsDocLoader.cpp:258
9 xul.dll nsDocShell::Stop docshell/base/nsDocShell.cpp:4353
Crash Signature: [@ mozilla::css::SheetLoadData::~SheetLoadData]
Flags: needinfo?(emilio)
Keywords: crash
OS: Windows 10 → All

This means that a channel didn't notify of an error before releasing its listener, so it's a networking bug.

Component: General → Networking
Flags: needinfo?(emilio)
Product: Thunderbird → Core
Summary: Crash in mozilla::css::SheetLoadData::~SheetLoadData → Crash in mozilla::css::SheetLoadData::~SheetLoadData (dropping listener without notifying)
Version: Thunderbird 101 → unspecified
Whiteboard: [tbird crash]

(In reply to Wayne Mery (:wsmwk) from comment #2)

Please use the "create a bug" link listed in the crash report page at https://crash-stats.mozilla.org/report/index/78f70879-b072-460d-a230-c11d90220516#tab-bugzilla so that it builds a full picture in the bug report.

Will do. I didn't even know that option existed there until you pointed it out.

Severity: -- → S3
Priority: -- → P2
Whiteboard: [tbird crash] → [tbird crash][necko-triaged]

UAF ("rax": "0xe5e5e5e5e5e5e5e5",) in https://crash-stats.mozilla.org/report/index/7845a741-da98-4b9a-8337-4ab5f0220804#tab-details
Also in https://crash-stats.mozilla.org/report/index/5790a8b2-e0e8-4366-8fb8-402c70220612
Also "r9": "0xe5e5e5e5e5e5e5e5" in https://crash-stats.mozilla.org/report/index/42143129-d231-4a97-8d89-ed6800220621

There are many DIAGNOSTIC crashes due to a MOZ_DIAGNOSTIC_ASSERT(mSheetCompleteCalled || mIntentionallyDropped,...), which may be UAFs as well.

Group: core-security
Keywords: csectype-uaf, csectype-wildptr
Priority: P2 → --

It's been a very long time since I've hit this issue. Definitely not since 102. Many of the improvements made to 102 and subsequent betas seems to make rendering a newly arrived email almost instant. I have vague recollection that pre-102 I would click an email and it rendered it in the message pane window slowly and I hit this issue. It's nearly instant since at least 105 and newer.

I haven't had to use Wayne's suggestion in comment 2 since then.

Group: core-security → network-core-security

Hit this again and when I hit it I had about 10 unread messages in my Inbox. I began clicking on them sequentially in rapid succession and then it crashed: https://crash-stats.mozilla.org/report/index/4f195061-19f4-4988-8498-8209e0221030

Let me also add that the emails I get from airfarewatchdog.com seem to trigger it. I have emails from airfarewatchdog.com as "allow viewing remote content to load" or whatever the setting is.

Hi Arthur, could you forward one of the emails that triggers this bug to necko@mozilla.com?
Thanks!

Flags: needinfo?(thee.chicago.wolf)
Priority: -- → P2
Whiteboard: [tbird crash][necko-triaged] → [tbird crash][necko-triaged][necko-priority-review]
Severity: S3 → S2
Flags: needinfo?(kershaw)

(In reply to Valentin Gosu [:valentin] (he/him) from comment #10)

Hi Arthur, could you forward one of the emails that triggers this bug to necko@mozilla.com?
Thanks!

I sent one example to the necko address. Of course, anyone can subscribe to the free Airfare Watchdog site and get the same emails as well. It's sent once per day and simply letting a bunch of them pile up unread in some test Inbox or what have you will give one the opportunity to potentially repro. Again, this sender I do allow remote images from so that is also part of the means to potentially repro.

Flags: needinfo?(thee.chicago.wolf)

And if there's any kind of super verbose debug mode I can enable to potentially catch it in the act, let me know and I'll turn it on.

Added Wayne since they'd commented before, and the repro case is in tbird

I don't have any insight into your crashes comment 8, comment 7 and comment 0 or what logging to enable. Maybe magnus does?

I've signed up to see if I get any crashes.

Flags: needinfo?(mkmelin+mozilla)

No input, sorry.

Flags: needinfo?(mkmelin+mozilla)
Assignee: nobody → kershaw
Flags: needinfo?(kershaw)
Whiteboard: [tbird crash][necko-triaged][necko-priority-review] → [tbird crash][necko-triaged][necko-priority-queue]

It's unfortunate how often this happens to the reporter given how infrequently we see this signature on crash-stats :-(

Could we change the diagnostic assert into a release assert and at least make sure this isn't a security problem? Could increase crash frequency though.

Flags: needinfo?(kershaw)

I'm going to amend this by stating that the only manner of STR that might trigger this is:

  1. Using a test email account or one of your usual email accounts, subscribe yourself to Airfare Watchdog and / or The Flight Deal alerts (they are not spammy, 1 email per day give or take)
  2. When you receive your first email from them, click it and allow TB to load remote content from them going forward
  3. Let quite a few (10?) accumulate in your Inbox in an unread state
  4. Once you have a good amount of them in a row, click each one in rapid succession from oldest to newest so that it begins to load in the message view window and, before it can fully finish loading, quickly go to the next messages repeating this same step

Clicking a message should be long enough change the message state from unread to read but not long enough to, perhaps, fully load the message. Gotta be quick!

Since the crash volume is low (less than 15 per week), the severity is downgraded to S3. Feel free to change it back if you think the bug is still critical.

For more information, please visit auto_nag documentation.

Severity: S2 → S3

(In reply to Daniel Veditz [:dveditz] from comment #16)

It's unfortunate how often this happens to the reporter given how infrequently we see this signature on crash-stats :-(

Could we change the diagnostic assert into a release assert and at least make sure this isn't a security problem? Could increase crash frequency though.

Will do. Thanks.

Flags: needinfo?(kershaw)

(In reply to Arthur K. (he/him) from comment #17)

I'm going to amend this by stating that the only manner of STR that might trigger this is:

  1. Using a test email account or one of your usual email accounts, subscribe yourself to Airfare Watchdog and / or The Flight Deal alerts (they are not spammy, 1 email per day give or take)
  2. When you receive your first email from them, click it and allow TB to load remote content from them going forward
  3. Let quite a few (10?) accumulate in your Inbox in an unread state
  4. Once you have a good amount of them in a row, click each one in rapid succession from oldest to newest so that it begins to load in the message view window and, before it can fully finish loading, quickly go to the next messages repeating this same step

Clicking a message should be long enough change the message state from unread to read but not long enough to, perhaps, fully load the message. Gotta be quick!

Not sure why I can't reproduce this locally.
Maye I ask you to record a http log if you can reproduce this easily?
Thanks.

Flags: needinfo?(thee.chicago.wolf)

(In reply to Kershaw Chang [:kershaw] from comment #21)

(In reply to Arthur K. (he/him) from comment #17)

I'm going to amend this by stating that the only manner of STR that might trigger this is:

  1. Using a test email account or one of your usual email accounts, subscribe yourself to Airfare Watchdog and / or The Flight Deal alerts (they are not spammy, 1 email per day give or take)
  2. When you receive your first email from them, click it and allow TB to load remote content from them going forward
  3. Let quite a few (10?) accumulate in your Inbox in an unread state
  4. Once you have a good amount of them in a row, click each one in rapid succession from oldest to newest so that it begins to load in the message view window and, before it can fully finish loading, quickly go to the next messages repeating this same step

Clicking a message should be long enough change the message state from unread to read but not long enough to, perhaps, fully load the message. Gotta be quick!

Not sure why I can't reproduce this locally.
Maye I ask you to record a http log if you can reproduce this easily?
Thanks.

This isn't easily reproducible. I can give it a shot in a few weeks time with the http log instructions but I need to let a bunch of unread messages back up in Inbox first.

Flags: needinfo?(thee.chicago.wolf)

Comment on attachment 9307791 [details]
Bug 1769635 - Make sure AsyncAbort is always called, r=#necko

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Not sure. We still don't understand how to trigger this exactly.
  • 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?: all
  • 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?: Low. This patch only makes sure when a channel is canceleld, we always call notifies listener. Should be a low risk patch.
  • Is Android affected?: Yes
Attachment #9307791 - Flags: sec-approval?

Comment on attachment 9307791 [details]
Bug 1769635 - Make sure AsyncAbort is always called, r=#necko

Approved to land and request uplift

Attachment #9307791 - Flags: sec-approval? → sec-approval+
Flags: needinfo?(kershaw)

Landed: https://hg.mozilla.org/integration/autoland/rev/53b86a805445a4fd781c4b7f2ca19a5d74a84c5a

Backed out for causing mochitest failure in test_meta_referrer.html:
https://hg.mozilla.org/integration/autoland/rev/88e642f9e2dca084cba6fe228d7b5456f62b7128

Push with failures
Failure log

TEST-UNEXPECTED-FAIL | /tests/dom/security/test/general/test_meta_referrer.html logged result after SimpleTest.finish(): the referer header should not be set - true == true

Flags: needinfo?(kershaw)
Flags: needinfo?(kershaw)
Flags: needinfo?(kershaw)

Make sure AsyncAbort is always called, r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/17e0bbb609a9cf4f2727b72ff74818e39f15b9a4
https://hg.mozilla.org/mozilla-central/rev/17e0bbb609a9

Group: network-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
status-firefox110: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 110 Branch

Please nominate this for Beta and ESR102 approval when you get a chance.

status-firefox108: --- → wontfix
status-firefox109: --- → affected
status-firefox-esr102: --- → affected
tracking-firefox109: --- → +
tracking-firefox110: --- → +
tracking-firefox-esr102: --- → 109+
Flags: needinfo?(kershaw)

Comment on attachment 9307791 [details]
Bug 1769635 - Make sure AsyncAbort is always called, r=#necko

Beta/Release Uplift Approval Request

  • User impact if declined: Could crash due to UAF.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: Bug 1807879
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The logic of this patch is straightforward, but this patch changes one assertion to a release assertion. So the crash rate might be a bit higher.
  • String changes made/needed: N/A
  • Is Android affected?: Yes

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: Could crash due to UAF.
  • Fix Landed on Version: 110
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The logic of this patch is straightforward, but this patch changes one assertion to a release assertion. So the crash rate might be a bit higher.
Flags: needinfo?(kershaw)
Attachment #9307791 - Flags: approval-mozilla-esr102?
Attachment #9307791 - Flags: approval-mozilla-beta?

Please note that D164449 needs to be uplifted together with D165658 in Bug 1807879.

I've linked these two patches together on phabricator. Thanks.

Comment on attachment 9307791 [details]
Bug 1769635 - Make sure AsyncAbort is always called, r=#necko

Approved for 109.0b8 and 102.7esr.

Attachment #9307791 - Flags: approval-mozilla-esr102?
Attachment #9307791 - Flags: approval-mozilla-esr102+
Attachment #9307791 - Flags: approval-mozilla-beta?
Attachment #9307791 - Flags: approval-mozilla-beta+

Will the fixes make it into TB 109.0b3?

(In reply to Arthur K. (he/him) from comment #35)

Will the fixes make it into TB 109.0b3?

It is fixed in Firefox 109, so the latest Thunderbird beta should have the same fix.

I just hit this crash moments ago when I clicked on a newly arrived email from AirFare Watchdog: https://crash-stats.mozilla.org/report/index/5444c8c0-c5b5-4a99-8759-b5fe00230119

And sorry, I didn't have HTTP logging running. I created a batch file to launch TB using info from https://firefox-source-docs.mozilla.org/networking/http/logging.html so maybe one day I will catch it in the act and have something for you.

Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: