1974770 - Crash in [@ mozilla::StoragePrincipalHelper::PartitionKeyForExpandedPrincipal]

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, defect)

Description

[Dianna Smith [:diannaS]]

Crash report: https://crash-stats.mozilla.org/report/index/6ba24148-6026-4142-833c-3734c0250630

Reason:

EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames:

0  xul.dll  mozilla::StoragePrincipalHelper::PartitionKeyForExpandedPrincipal(nsIPrincipal*)  toolkit/components/antitracking/StoragePrincipalHelper.cpp:713
1  xul.dll  NS_NewChannelWithTriggeringPrincipal(nsIChannel**, nsIURI*, nsINode*, nsIPrin...  netwerk/base/nsNetUtil.cpp:534
2  xul.dll  NewImageChannel(nsIChannel**, bool*, nsIURI*, nsIURI*, mozilla::CORSMode, nsI...  image/imgLoader.cpp:903
3  xul.dll  imgLoader::LoadImage(nsIURI*, nsIURI*, nsIReferrerInfo*, nsIPrincipal*, unsig...  image/imgLoader.cpp:2502
4  xul.dll  nsContentUtils::LoadImage(nsIURI*, nsINode*, mozilla::dom::Document*, nsIPrin...  dom/base/nsContentUtils.cpp:4546
5  xul.dll  mozilla::css::ImageLoader::LoadImage(mozilla::StyleComputedUrl const&, mozill...  layout/style/ImageLoader.cpp:447
5  xul.dll  mozilla::StyleComputedUrl::ResolveImage(mozilla::dom::Document&, mozilla::Sty...  layout/style/nsStyleStruct.cpp:121
6  xul.dll  mozilla::StyleGenericImage<mozilla::StyleGenericGradient<mozilla::StyleLineDi...  layout/style/nsStyleStruct.cpp:1685
6  xul.dll  nsStyleImageLayers::Layer::ResolveImage(mozilla::dom::Document&, nsStyleImage...  layout/style/nsStyleStruct.h:263
6  xul.dll  nsStyleImageLayers::ResolveImages(mozilla::dom::Document&, nsStyleImageLayers...  layout/style/nsStyleStruct.h:316

Comment 1

[Dianna Smith [:diannaS]]

:bvandersloot could this be related to bug 1959656?

Comment 2

[Daniel Holbert [:dholbert]]

I just hit this twice locally (with the same STR), which leads me to believe I have reliable STR.

My STR:

1. Be sure you have the Browser Toolbox enabled and that you're signed in to Sync.
2. Visit https://firefox.com/pair/ and click "I already have Firefox for mobile" and then Continue.
3. Open the Browser Toolbox (Tools | Browser Tools | Browser Toolbox -- only available if you did step 1)
4. Click the toolbox's "pick-an-element" button (the top left corner) and then click the QR code that the Sync pairing UI is showing you. This probably will focus an element with id="qrSpinner" in the toolbox which is the sibling of the element you really want.
5. In the toolbox inspector, select the previous element which has id="qrContainer" and a data-URI background-image.
6. In the "Rules" pane, uncheck the checkbox for background-image: url(data:... rule to disable that rule.
7. Check the checkbox again to reenable that rule.

ACTUAL RESULTS:
Crash. These two crash reports are from me following these^ STR:
bp-43cf67e2-7e71-4e6b-8a34-7b6310250701
bp-b23cba85-51eb-42a6-a4fe-aebf20250701

EXPECTED RESULTS:
No crash.

Comment 3

[Benjamin VanderSloot [:bvandersloot]]

Thank you so much for the STR! Confirmed on MacOS, latest Nightly.

Looks like when fixing Bug 1957355, this code fails to account for BasePrincipals in the ExpandedPrincipal's allowlist without a URI- in this case, maybe a system principal. In diagnosing it I have a patch that I think fixes it. I'll get it up for review now, r=baku who authored the original patch.

Comment 4

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 1974770 - Add null URI check while producing the partition key for ExpandedPrincipals - r=baku!

Comment 5

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

Set release status flags based on info from the regressing bug 1957355

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/68d966c60395
https://hg.mozilla.org/integration/autoland/rev/2fa131e45f54
Add null URI check while producing the partition key for ExpandedPrincipals - r=baku,anti-tracking-reviewers,timhuang

Comment 7

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/2fa131e45f54

Comment 8

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

The patch landed in nightly and beta is affected.
:bvandersloot, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox141 to wontfix.

For more information, please visit BugBot documentation.

Comment 9

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 1974770 - Add null URI check while producing the partition key for ExpandedPrincipals - r=baku!

Original Revision: https://phabricator.services.mozilla.com/D256823

Comment 10

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: crash with browser devtool use, if a data url is added to a resource
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: na
• Risk associated with taking this patch: none excepet the core risk of uplifting any change
• Explanation of risk level: no more than minimal
• String changes made/needed: no
• Is Android affected?: yes

Comment 11

[Benjamin VanderSloot [:bvandersloot]]

I've requested an uplift to beta since this is a crash. I don't have a strong opinion on ESR uplift.

Comment 12

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/e62b0d2867f0
https://hg.mozilla.org/releases/mozilla-beta/rev/bc0d6a20a38d

Comment 13

[Ryan VanderMeulen (PTO, back 6-April)]

Comment on attachment 9500058 [details]
Bug 1974770 - Add null URI check while producing the partition key for ExpandedPrincipals - r=baku!

Approved for 140.1esr.

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/cdb01a61a256