1753810 - Firefox crashes when a content script calls Cache.put()

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Danny Lin]

Attachment: test-bug - addon-cachestorage.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:96.0) Gecko/20100101 Firefox/96.0

Steps to reproduce:

1. Download and install the attached test addon.
2. Visit https://example.com/

Actual results:

Firefox crashes immediately when the content script calls Cache.put()

Expected results:

Firefox should not crash.

Cache.add() doesn't seem to cause the same issue.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'WebExtensions::Untriaged' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Alex Cornestean]

Hello,

I reproduced the issue on the latest Nightly (98.0a1/20220206212741), Beta (97.0/20220202182137) and Release (96.0.3/20220126154723) under Windows 10 x64 and Ubuntu 16.04 LTS.

The browser immediately crashes when visiting https://example.com/ while having the attached extension installed.

Here is a link to a crash report from Nightly: https://crash-stats.mozilla.org/report/index/0571a986-5379-4640-b435-e36220220207.

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

The reason for the crash is that:

• the content script is creating a request using await fetch(...), which created a Request instance that belongs to the ExpandedPrincipal that the content script sandbox uses, then it does call cache.put(...) to insert that Request into a Cache that belongs to the webpage (and so it belongs to a ContentPrincipal).

• mozilla::dom::cache::db::InsertEntry (defined in dom/cache/DBSchema) does expect that all requests received are going to belong to a content principal.

• and so the following diagnostic assertion MOZ_DIAGNOSTIC_ASSERT(principalInfo.type() == mozilla::ipc::PrincipalInfo::TContentPrincipalInfo); triggers a crash when the call to cache.put(...) originated by the content script is handled in the parent process.

As additional side notes:

• fetch vs. content.fetch: in the content scripts global, there is also a content.fetch available, which belogs to the content page (unlike fetch which belongs to the content script sandbox), and so if the request is created using await content.fetch(...) then the diagnostic assertion is not going to be triggered (because the resulting Requests belongs to the webpage, and so a ContentPrincipal) and it is stored in the cache as expected.

• In MV3 the fetch (and XMLHttpRequest) cross-origin behaviors are going to be deprecated (and Bug 1578405 is tracking the deprecation of them also in MV2), and at that point they would be replaced with content.fetch (and content.XMLHttpRequest) and that would also technically prevent extensions content scripts from triggering this issue in the long run.

My current opinion is that cache.put(...) should check explicitly for the ExpandedPrincipal and reject the call before having sent it to the parent process, with the following motivation:

• an extension may be leaking some data (data that may be only accessible to the extension, e.g. because of its host permission) to the webpage by storing it into a cache API storage that is also accessible from the webpage and not exclusive to the webpage (they can still use content.fetch to store one that wouldn't differ from what the webpage itself would have access even without the extension).

• In MV3 (and eventually MV2, tracked by Bug 1578405) the content scripts fetch requests will belong to the webpage content principal (like content.fetch does already), and so in the long run the extensions shouldn't be able to trigger this issue anymore.

Comment 4

[kernp25]

Isn't this the same bug 1625995?

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1753810 - Make sure Cache::Put rejects calls originated from an expanded principal.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to kernp25 from comment #4)

Isn't this the same bug 1625995?

eh, yep it is definitely the same issue, thanks so much for pointing it out!!!

I'm going to close bug 1625995 as a duplicate of this one, given that this one now has a description of the actual underlying issue (comment 3) and a patch with a draft for the proposed fix and a test case (comment 5).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/316eadf6ac4b
Make sure Cache::Put rejects calls originated from an expanded principal. r=dom-storage-reviewers,asuth

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/316eadf6ac4b

Comment 10

[Alex Cornestean]

Hello,

Verified the fix on the latest Nightly (99.0a1/20220213214259) under Windows 10 x64 and Ubuntu 16.04 LTS.

The browser no longer crashes when visiting https://example.com/ while having the attached extension installed, confirming the fix.

Comment 11

[Pascal Chevrel:pascalc]

Luca, is that something that we should uplift to beta or can it ride the 99 train? Thanks

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Pascal Chevrel:pascalc from comment #11)

Luca, is that something that we should uplift to beta or can it ride the 99 train? Thanks

My short answer would be:

the fix seems trivial enough to be pretty low risky and it is also covered by an automated test included in the same patch, and so I would be ok to proceed to create an uplift request for it.

As additional context to evaluate if we need to:

• The issue can be triggered consistently (it is not a race or something that would still need a combination of conditions to be all verified) with a two lines content script.
• Honestly I think that some extension developers may even do it without really realizing that it would trigger this issue (that the Response object got from the fetch global available would trigger that if tried to be added to the webpage cache storage).
• I'm not sure how common for a content script is to try that in practice, but Bug 1625995 (the one that I closed as a duplicate of this one) was filed 2 years ago, and so we may want to take a look to crash stats and based on the volume confirm if this was happening often enough to be something that may be happening to users in the wild (vs. tried out by developers locally first and never included in an extension used in production).

Comment 13

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

The patch landed in nightly and beta is affected.
:rpl, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 9262736 [details]
Bug 1753810 - Make sure Cache::Put rejects calls originated from an expanded principal.

Beta/Release Uplift Approval Request

• User impact if declined: If the user has installed an extension that includes a content script that trying to put a content script's fetch Response object into the webpage Cache API storage, then the entire browser would crash consistently every time the extension content script triggers that.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Same STR used to verify the fix on Nightly
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): the fix is small and it does just trigger a rejection when Cache::Put is called with a Response object that belongs to an expanded principal (which is something that only Extensions' content scripts can trigger nowdays).
• String changes made/needed:

Comment 15

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/5f8a17eac98e

Comment 16

[Pascal Chevrel:pascalc]

Comment on attachment 9262736 [details]
Bug 1753810 - Make sure Cache::Put rejects calls originated from an expanded principal.

Approved for 98 beta 8, thanks.

Comment 17

[Alex Cornestean]

Hello,

Verified the fix on the latest Beta (98.0b8/20220222185824) under Windows 10 x64 and Ubuntu 16.04 LTS.

The browser no longer crashes when visiting https://example.com/ while having the attached extension installed, confirming the fix.