1929600 - (CVE-2024-11696) Broken extension can prevent XPIDatabase.verifySignatures from enforcing signatures

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Rob Wu [:robwu]]

Bug 1892961 introduced a loadManifestFromFile call in verifySignatures. Unfortunately, the logic does not account for the possibility of the method throwing (e.g. when a manifest is considered invalid or broken).

There are multiple ways to trigger this bug, but as an example I'll provide a STR that simulates a scenario where a previously-valid manifest is now considered invalid.

STR:

1. Launch Firefox with a new profile.
2. Install a manifest version 3 extension, e.g. https://addons.mozilla.org/en-US/firefox/addon/gnome-shell-integration/
3. Visit about:config and set extensions.manifestV3.enabled to false.
4. At about:config, set the app.update.lastUpdateTime.xpi-signature-verification preference to 0.
   (if it was already 0, skip step 5).
5. Restart Firefox.
6. In about:config search for the xpi-signature pref from step 4.
7. Open the Browser Console (Ctrl-Shift-J) and wait a bit, until the preference value in step 6 changes from 0 to some number (current timestamp).
8. Look at the Browser Console, for errors.

Expected:

• Non-fatal errors.

Actual:

• There is an error printed with XPI_verifySignature: Error: Unsupported manifest version: 3.
• Upon clicking on it to reveal the stack trace, it ultimately points to verifySignature, XPIDatabase.sys.mjs:2271. Note that this is in a catch block, which means that a runtime error prevented the logic from continuing as usual.
  • The security-sensitive part of this bug is that this can prevent Firefox from effectively enforcing signature validation on other unrelated add-ons.
• Although the log does not show it, the source of the error is the unchecked loadManifestFromFile call in verifySignatures.

Recommendation:

• The loadManifestFromFile call should be wrapped in a try-catch.

Comment 1

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

Attachment: Bug 1929600 r=robwu!

Comment 2

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

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

Comment 3

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

Attachment: Bug 1929600 - Test XPIDatabase.verifySignatures on extensions with an invalid manifest. r=robwu!

Depends on D228201

Comment 4

[Rob Wu [:robwu]]

Note on security impact: The verifySignatures function is a method to catch inconsistencies in what ought to be considered a valid signature vs the actual signature in the add-on database. Ordinarily, there should not be such an inconsistency. But the need for inconsistency fixups can arise if external software tampered with the database. Another situation where this logic can result in observable differences is in bugs like bug 1548973 (armagadd-on) - but that cannot happen any more for the next few decades (because we don't have expiring add-on signing certs for the next few decades).

For this reason I'd rate this as sec-low or sec-moderate at most.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4ee07708c3f
r=robwu

Comment 6

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/mozilla-central/rev/b4ee07708c3f

Comment 7

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

Attachment: Bug 1929600 r=robwu!

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

Comment 8

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: For Users with an installed extension with an invalid manifest, XPIProvider verifySignatures would not be able to catch incosistent signing for addons that follow the one with an invalid manifest
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: This patch is paired with a child revision that provides test coverage (not landed yet) and it has been tested manually. Comment 0 STR can be used as a reference if we want to run an additional manual QE verification.
• Risk associated with taking this patch: Low risk
• Explanation of risk level: the change is minimal (the call to loadManifestFromFile has been wrapped in a try/catch) and the issue well understood.
• String changes made/needed: -
• Is Android affected?: yes

Comment 9

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

Attachment: Bug 1929600 r=robwu!

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

Comment 10

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: For Users with an installed extension with an invalid manifest, XPIProvider verifySignatures would not be able to catch incosistent signing for addons that follow the one with an invalid manifest
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: This patch is paired with a child revision that provides test coverage (not landed yet) and it has been tested manually. Comment 0 STR can be used as a reference if we want to run an additional manual QE verification.
• Risk associated with taking this patch: Low risk
• Explanation of risk level: the change is minimal (the call to loadManifestFromFile has been wrapped in a try/catch) and the issue well understood.
• String changes made/needed: -
• Is Android affected?: no

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/e692f49dec28

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/3b50417aca25

Comment 13

[Tom Ritter [:tjr] (OOTO Oct 18-26)]

Attachment: advisory.txt

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a7799de94805
https://hg.mozilla.org/integration/autoland/rev/a8be673786ef
Test XPIDatabase.verifySignatures on extensions with an invalid manifest. r=robwu

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/43bf95dc909b
https://hg.mozilla.org/integration/autoland/rev/d814f435ee53
Revert "Bug 1929600 - Test XPIDatabase.verifySignatures on extensions with an invalid manifest. r=robwu" for causing xpc failures @ test_signed_verify.js

Comment 16

[Sandor Molnar[:smolnar]]

Backed out for causing xpc failures @ test_signed_verify.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/d814f435ee53a5ed24d9c90ecb59c373645d24c3

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | xpcshell.toml:toolkit/mozapps/extensions/test/xpcshell/test_signed_verify.js

Comment 17

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

(In reply to Pulsebot from comment #15)

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/43bf95dc909b
https://hg.mozilla.org/integration/autoland/rev/d814f435ee53
Revert "Bug 1929600 - Test XPIDatabase.verifySignatures on extensions with
an invalid manifest. r=robwu" for causing xpc failures @
test_signed_verify.js

I see that the failure was hit on mobile builds, the logic that this new test case covers is only executed on builds where Services.policies is defined and so it is completely skipped on mobile builds and so the test hits a failure.

I've just updated the phabricator revision to account for that (and skip the new test case on builds where Services.policies is not defined).

I'll push the updated revision to autoland as soon as the static analysis job has completed and confirmed there are no linting errors as expected.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ad2cd9c2a7be
https://hg.mozilla.org/integration/autoland/rev/abc44dcc5bf1
Test XPIDatabase.verifySignatures on extensions with an invalid manifest. r=robwu

Comment 19

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/abc44dcc5bf1