1770468 - 1password addon won't open window: call to WebAssembly.instantiateStreaming() blocked by CSP

Status: RESOLVED FIXED

(WebExtensions :: Compatibility, defect, P1)

Description

[David Parks [:handyman]]

The 1password addon stopped working for me in the last couple of days. Neither Ctrl+. nor clicking on the 1password icon in the toolbar do anything. Of course, this could be a 1password issue but it was regressed by nightly. This happened on Windows 10.

mozregression points to this window. The cookies commits appear to be the most probable origin (maybe).

Comment 1

[Tatsuyuki Ishi]

Same symptom on Linux, ~/.config/1Password/logs/BrowserSupport/1Password_rCURRENT.log shows:

INFO 2022-05-21T13:35:35.967 main(ThreadId(1)) [1P:native-messaging/op-browser-support/src/main.rs:163] Starting 1Password-BrowserSupport 8.7.0 production build no. 80700098.
ERROR 2022-05-21T13:35:35.990 main(ThreadId(1)) [1P:native-messaging/op-browser-support/src/main.rs:195] Browser support error: InvalidExtensionId, happened in: native-messaging/op-browser-support/src/main.rs:110
Additional error context: [] doesn't match any valid extension ids

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

I'm seeing this symptom on Mac also, both with the production 1password browser extension version 2.3.3 and the beta version of 2.3.6.

Adding a few of the folks touching stuff in that regrssion window to the CC to see what their thoughts are.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

It seems like bug 1740263?

If I open the Javascript console for the add-on background page I get:

👋 Initializing 1Password background.js:2:2220051
channel: stable
version: 2.3.3
build: 20207 (102.0)
browser: Firefox (102.0)
os: Windows (10.0) background.js:2:2220144
Content Security Policy: ページの設定により次のリソースの読み込みをブロックしました: wasm-eval (“script-src”) moz-extension:1:10231
Uncaught (in promise) CompileError: call to WebAssembly.instantiateStreaming() blocked by CSP
    <anonymous> moz-extension://8c8bbfe3-d99b-442b-b9d6-a566a2b6e2cf/1.js:1
    n moz-extension://8c8bbfe3-d99b-442b-b9d6-a566a2b6e2cf/1.js:1

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

For now, turning off security.csp.wasm-unsafe-eval.enabled allows me to continue using 1Password.

Comment 5

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

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

Comment 6

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

:tschuster, since you are the author of the regressor, bug 1740263, could you take a look?
For more information, please visit auto_nag documentation.

Comment 7

[Tom Schuster (MoCo)]

Does anyone have a contact at 1Password and can ask them to fix their CSP?

Currently they are are using:
"content_security_policy": "default-src 'none'; base-uri 'none'; form-action 'none'; frame-ancestors http: https:; img-src 'self' data: blob: https://c.1password.com https://a.1passwordentusercontent.com https://a.1passwordusercontent.com https://a.1passwordusercontent.eu https://a.1passwordusercontent.ca https://cache.agilebits.com; font-src 'self'; connect-src https://*.1password.ca wss://b5n.1password.ca https://*.1password.com wss://b5n.1password.com wss://b5n.ent.1password.com https://*.1password.eu wss://b5n.1password.eu https://*.agilebits.com https://*.b5dev.ca wss://b5n.b5dev.ca https://*.b5dev.com wss://b5n.b5dev.com https://*.b5dev.eu wss://b5n.b5dev.eu https://*.b5local.com:3000 wss://b5local.com:3001 https://*.b5local.com:4000 wss://b5local.com:4001 https://*.b5test.ca wss://b5n.b5test.ca https://*.b5test.com wss://b5n.b5test.com https://*.b5test.eu wss://b5n.b5test.eu https://*.b5rev.com wss://*.b5rev.com https://api.privacy.com https://sandbox.privacy.com https://api.pwnedpasswords.com https://f.1passwordentusercontent.com https://f.1passwordusercontent.ca https://f.1passwordusercontent.com https://f.1passwordusercontent.eu https://www.fastmail.com https://jmap.fastmail.com https://betajmap.fastmail.com https://api.fastmail.com https://accounts.staging.brexapps.com https://platform.staging.brexapps.com https://platform.brexapis.com https://accounts.brex.com; style-src 'self' 'unsafe-inline'; script-src 'self';",

script-src 'self'; must be changed to script-src 'self' 'wasm-unsafe-eval';

Comment 8

[Tomislav Jovanovic :zombie]

Hi Oliver, can you please look into this or forward to the right person?

Here is the summary of the changes, sorry it isn't better documented yet:
https://bugzilla.mozilla.org/show_bug.cgi?id=1766027#c2

Comment 9

[Rob Wu [:robwu]]

Oliver is aware of it; we chatted about this on Matrix last Friday.

Comment 10

[Oliver Dunk]

Hi Tomislav, thanks for passing this one along! We're tracking it internally (as Rob mentioned I asked about it on Friday) and have started looking at updating our manifest to include the new CSP directive. No timeline to share there yet but hopefully we'll have something out soon. It's definitely on our mind.

On that note - could someone confirm that despite the warning, when an unknown keyword like unsafe-wasm-eval is present, this is ignored and the rest of script-src and the overall CSP continues to function as normal?

I think there may be a (potentially important) misunderstanding about the behaviour in Chrome. Unless I'm missing something, Chrome explicitly continued to support wasm-eval for extensions when implementing the new keyword: https://chromium-review.googlesource.com/c/chromium/src/+/2937442. This is still the behaviour today which I was able to verify but loading our stable extension in Chrome 104 (Canary). As such, Chrome were able to implement the change without breaking any MV2 extensions, even those which explicitly specify a CSP.

I'm not sure where the best place for feedback is (I was thinking a bug) but this is evidently a breaking change for us and I feel like it could've done with more warning. While Nightly is inherently unstable to a degree, the existence of this issue shows that changes still impact real users. In the future, I'd love to see a change like this announced at WECG with a warning shown in Firefox for a period of time before enforcement.

Despite all of the above it seems like there was a significant amount of work to try and coordinate this change with Chrome and understand the affected extensions. I really appreciate that! I'd still love to hear your thoughts on the above but don't let that take away from the good bits 🙂

Comment 11

[Rob Wu [:robwu]]

I originally assumed that changing the default CSP is good enough for backcompat, since overriding the CSP is generally discouraged.

If there is more unexpected fallout than expected, then I'm willing to (temporarily?) accept/land a patch to automatically add 'wasm-unsafe-eval' to the extension's script-src directive for MV2 extensions with a custom CSP.

Comment 12

[Oliver Dunk]

Rob: I think a temporary change like that would be great if you're open to it - happy to test something when it's ready. We're still looking at fixing this on our end, and hopefully we'll get that live as soon as possible, but just in case we can't get it out right away relaxing the Firefox change seems like a nice fallback. That means we can be sure things are up and running for people by tomorrow morning.

If that ends up being the route we take it might be nice to add a warning that says something like "You're using WASM and don't have this keyword, this won't be supported in the future"?

Comment 13

[Rob Wu [:robwu]]

(In reply to Oliver Dunk from comment #12)

Rob: I think a temporary change like that would be great if you're open to it - happy to test something when it's ready. We're still looking at fixing this on our end, and hopefully we'll get that live as soon as possible, but just in case we can't get it out right away relaxing the Firefox change seems like a nice fallback. That means we can be sure things are up and running for people by tomorrow morning.

Since Nightly branches off to a new version next week, a change like that would need to land within a few days. Note that you will need the keyword anyway in MV3, and in Chrome (in MV2), so if you are already maintaining a cross-browser extension, then you should be familiar with this keyword.

I'll discuss this with my team, and get back with a reply within an hour or two.

If that ends up being the route we take it might be nice to add a warning that says something like "You're using WASM and don't have this keyword, this won't be supported in the future"?

It's not straightforward to know whether an extension uses wasm, until it actually tries to compile/run wasm code. At that point, there is already a warning about execution being blocked by the CSP, if the extension has overridden the default CSP.

Comment 14

[Oliver Dunk]

Note that you will need the keyword anyway in MV3, and in Chrome (in MV2), so if you are already maintaining a cross-browser extension, then you should be familiar with this keyword.

We're definitely aware of this keyword but had never thought to include it in Firefox, since it wasn't previously required and would even generate a warning if included. I'm 100% in support of the change longer term but I think extensions need time to update - both in cases like ours, where we've already noticed, and in the case of smaller developers who may not have a significant user base in nightly.

...there is already a warning about execution being blocked by the CSP, if the extension has overridden the default CSP.

This would only be if it gets blocked, right? What I mean is that if you land a change temporarily adding the keyword by default, and then someone tries to load WASM, it would be nice to have something that says "it worked this time because we added the WASM flag by default, but that isn't going to be the case in version XXX". Understand if that is hard to hook up but it feels like the only way to warn developers who may only be testing in stable/beta. A log in the console feels sufficient.

Comment 15

[Rob Wu [:robwu]]

(In reply to Oliver Dunk from comment #14)

...there is already a warning about execution being blocked by the CSP, if the extension has overridden the default CSP.

This would only be if it gets blocked, right? What I mean is that if you land a change temporarily adding the keyword by default, and then someone tries to load WASM, it would be nice to have something that says "it worked this time because we added the WASM flag by default, but that isn't going to be the case in version XXX". Understand if that is hard to hook up but it feels like the only way to warn developers who may only be testing in stable/beta. A log in the console feels sufficient.

Add exception to DOM security code (C++)

An alternative to altering the declaration is to add another check before wasm is about to be blocked by the CSP: when that is the case, check whether AddonPolicy()->manifestVersion() == 3, and if so, just permit it (optionally with some logging somewhere). The code could be put here: https://searchfox.org/mozilla-central/rev/70cf6863bd85af2a3188ec1fe5209a3ec1b2de86/caps/nsScriptSecurityManager.cpp#515-516.

Modify content_security_policy on read from manifest.json (JavaScript)

Another alternative (without C++ changes, fully contained within extensions, but more complex) is to modify the content_security_policy key parser from manifest.json, for MV2, to unconditionally add ``wasm-unsafe-inline'` in script-src (and if that's missing, in default-src), at https://searchfox.org/mozilla-central/rev/70cf6863bd85af2a3188ec1fe5209a3ec1b2de86/toolkit/components/extensions/Schemas.jsm#1143-1161.

With this approach, it's not possible to know whether the policy is violated, because the policy is declared upfront.

Modify ExtensionCSP callers (C++)

If we really want to have logging, then we could build upon the Content-Security-Policy-Report-Only machinery.
We'd need to modify the CSP (+wasm-unsafe-eval as described before) and set a report-only policy with the unmodified policy. The first is to allow MV2 extensions to still use WebAssembly without CSP, the latter is to log.
While that offers logging, I'm not that thrilled about changing that here and here on such short notice.

Comment 16

[Tom Schuster (MoCo)]

An alternative to altering the declaration is to add another check before wasm is about to be blocked by the CSP: when that is the case, check whether AddonPolicy()->manifestVersion() == 3, and if so, just permit it (optionally with some logging somewhere). The code could be put here: https://searchfox.org/mozilla-central/rev/70cf6863bd85af2a3188ec1fe5209a3ec1b2de86/caps/nsScriptSecurityManager.cpp#515-516.

We would have to make the same change for Workers. This might be a bit harder.

If you don't have a fix before merge we are open restricting wasm-unsafe-eval to nightly/beta for now.

Comment 17

[Rob Wu [:robwu]]

I'm taking this so that this regression doesn't hit beta/release.

Comment 18

[Rob Wu [:robwu]]

(In reply to Tom Schuster (MoCo) from comment #16)

An alternative to altering the declaration is to add another check before wasm is about to be blocked by the CSP: when that is the case, check whether AddonPolicy()->manifestVersion() == 3, and if so, just permit it (optionally with some logging somewhere). The code could be put here: https://searchfox.org/mozilla-central/rev/70cf6863bd85af2a3188ec1fe5209a3ec1b2de86/caps/nsScriptSecurityManager.cpp#515-516.

We would have to make the same change for Workers. This might be a bit harder.

CSP is not nicely enforced in workers atm - bug 1685627. I do agree that we need tests and enforcement of the CSP in workers when that bug gets fixed.

Comment 19

[Shane Caraveo (:mixedpuppy)]

(In reply to Tom Schuster (MoCo) from comment #16)

An alternative to altering the declaration is to add another check before wasm is about to be blocked by the CSP: when that is the case, check whether AddonPolicy()->manifestVersion() == 3, and if so, just permit it (optionally with some logging somewhere). The code could be put here: https://searchfox.org/mozilla-central/rev/70cf6863bd85af2a3188ec1fe5209a3ec1b2de86/caps/nsScriptSecurityManager.cpp#515-516.

I this it the right thing to do.

We would have to make the same change for Workers. This might be a bit harder.

Not by much. Call locations have the principal already, just add a helper on BasePrincipal. I think it would be something like:

bool BasePrincipal::AddonAllowsWASM() {
  if (auto policy = AddonPolicy()) {
    return policy->manifestVersion() < 3;
  }
  return false;
}

callers:

    if (!BasePrincipal::Cast(aPrincipal)->AddonAllowsWASM() && NS_FAILED(csp->GetAllowsWasmEval(&reportViolation, &evalOK))) {
      return false;
    }

Comment 20

[Oliver Dunk]

Rob, could you confirm the following?

…despite the warning in older versions, when an unknown keyword like unsafe-wasm-eval is present, this is ignored and the rest of script-src and the overall CSP continues to function as normal?

Comment 21

[Rob Wu [:robwu]]

(In reply to Oliver Dunk from comment #20)

Rob, could you confirm the following?

…despite the warning in older versions, when an unknown keyword like unsafe-wasm-eval is present, this is ignored and the rest of script-src and the overall CSP continues to function as normal?

In Firefox, the value is ignored (i.e. not included in the effective policy) and the following is reported:

Content Security Policy: Couldn’t parse invalid host 'wasm-unsafe-eval'"

Comment 22

[Rob Wu [:robwu]]

Attachment: Bug 1770468 - Refactor test_ext_wasm.js

Refactor test to remove duplication, and onsecuritypolicyviolation event
handler to observe CSP violation reports.

Comment 23

[Rob Wu [:robwu]]

Attachment: Bug 1770468 - Report-only wasm-unsafe-eval in MV2

For backcompat, do not enforce wasm-unsafe-eval even if the extension
has specified a custom CSP. Do report the errors though, to allow
extension authors to discover the issue and fix it.

Comment 24

[Oliver Dunk]

For any 1Password users tracking this, we have now released a beta which includes a fix: https://releases.1password.com/b5x/beta/#1password-in-the-browser-2.3.7. It can be downloaded from here: https://1password.com/browsers/beta/firefox/.

This does still affect the stable channel. We'll try to get that sorted soon but will likely be beaten by the temporary relaxation of the change in https://bugzilla.mozilla.org/attachment.cgi?id=9277827.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2285a531fc2d
Refactor test_ext_wasm.js r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/f9d3225a4e4c
Report-only wasm-unsafe-eval in MV2 r=mixedpuppy,freddyb,ckerschb

Comment 26

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/2285a531fc2d
https://hg.mozilla.org/mozilla-central/rev/f9d3225a4e4c