1038068 - Add pref to require installs be signed by a Mozilla-issued add-on signing certificate

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Daniel Veditz [:dveditz]]

As a step toward bug 238960 we need to add a pref that controls whether add-ons are required to be signed by a Mozilla-issued add-on-signing certificate. If the pref is set then unsigned .xpis will result in an error message and will not install.

Comment 1

[Daniel Veditz [:dveditz]]

When the signed-addons feature is done the default will be to require signing. The code to respect the pref must be behind a build flag so that it can't be turned off in Beta and Release builds. The pref will only work for Nightly, Aurora, Debug, and self-built versions.

Comment 2

[Dave Townsend [:mossop]]

add-ons manager is toolkit code so we will also have to take into account allowing the pref to work in other applications if they want to be able to turn it off.

Comment 3

[Dave Townsend [:mossop]]

Dan, what is the status here? Without any work-in-progress patch or something we can't make any more progress on the frontend UI changes in bug 1148021.

Comment 4

[Daniel Veditz [:dveditz]]

Attachment: wip

WIP. Merged to the latest, seems to work with the new frontend UI code. The "success" case works. If it runs into an error the async promises don't throw as expected back to the caller so the UI just sits there waiting without reporting the error.

Since AMO won't have signed all addons in the near future we should flip the pref in firefox.js to false for the initial landing. To try this out use the two signed add-ons in bug 1130020. If you create and set the pref xpinstall.signatures.dev-root to true then you can install the two add-ons from bug 1130109 instead (which are the same two, just signed with the staging root).

Comment 5

[Dave Townsend [:mossop]]

Attachment: make loadManifest fully async

This adds on top of the wip and makes loadManifest fully async by returning a promise. All callers use the promise to catch errors rather than exceptions. I also added a few error states to set on the install and made up some dumb strings to display in the failure notification. This seems to make things work in my limited testing.

Also made the xpcshell tests default to not checking signatures and all tests pass with this code so I'm pretty confident that the loadManifest refactoring is right.

Dan, hopefully this is enough to keep you moving but let me know if there is anything else I can help with.

Comment 6

[Dave Townsend [:mossop]]

Thinking about it I don't think we need the error code for ERROR_SIGNATURE_MISMATCH, users shouldn't care that it's a mismatch, just use ERROR_CORRUPT_FILE for that case too. ERROR_SIGNATURE_REQUIRED should be -5 to match the string we will land in bug 1149696

Comment 7

[Dave Townsend [:mossop]]

[Tracking Requested - why for this release]:

First two stages of add-ons signing work are targeted at Firefox 39.

Comment 8

[Dave Townsend [:mossop]]

Attachment: patch

Taking over the remaining work here.

I did some tinkering with this this afternoon and despite what I said earlier I think it makes sense to change a few things so I'd like your input Dan before I proceed further:

I went through all your comments and think you're right that we basically have to validate the cert everytime we load the install manifest so I've made those functions do that. This means that the loadManifest function (confusing names!) used for installing add-ons doesn't really need to call the cert validation function itself, it can just rely on its call to load the manifest. This also all makes multi-package add-ons work ok, the main XPI doesn't need to be signed but every XPI inside does.

The downside is that some of the manifest loads have to happen synchronously during startup. I don't really see a way around that without massive user impact or opening up risk. To work around this I just spin an event loop to wait for the manifest load to complete for the cases that need it. It's not great but we've done it for other cases in startup before.

I don't think we need to retain the certname particularly so I just retain whether the cert is preliminary, full, missing or broken. This goes into the database and is available to outside code like the UI to use to display things right. This also allows us to set appDisabled correctly which has the knock-on effect of immediately fixing bug 1123918. Toggling the signature required pref at runtime even does the right thing and enabled/disabled add-ons that are no longer correct.

With the added string included here this patch also fixes bug 1149696.

Because of the database change the first run a user makes with this will force a rescan of all add-ons meaning unsigned add-ons will be immediately disabled according to the pref. No UI is hooked up to that yet but it will be fairly straightforward to get the list of add-ons that were disabled by this which is pretty nice.

I added a function stub we can fill out for bug 1038072, it currently just assumes nothing is signed.

While this code will refuse to install add-ons that aren't signed properly it will no auto-remove add-ons that are already installed and aren't signed properly, it will just disable them and the user will not be able to enable. If we actually want to do that I'd want to move it off to a separate bug for post-39 work to reduce the complexity of this patch.

I need to write a lot of automated tests for this but otherwise I think the implementation side is close to done here.

Some specific questions:

A comment you put in mentions "TOCTOU window". What is that?

This patch effectively kills the old XPI signature verification for other applications, is that what we want?

Comment 9

[Daniel Veditz [:dveditz]]

(In reply to Dave Townsend [:mossop] from comment #8)
> A comment you put in mentions "TOCTOU window". What is that?

"Time of Check/Time of Use" -- a class of potential security vulns. I think at that spot I was considering not revalidating the manifest (perf concerns) because it had just been checked a couple of dozen lines earlier. Technically that would mean a local attacking program could monitor Firefox and after it validated the file the first time swap in a malicious add-on that we would then just load (if we skipped validation the second time). But unless the malicious side program was actively swapping the add-on in and out we'd eventually detect and disable it on a future startup. Doesn't seem like the most plausible attack if you already presume malware that can monitor your process that closely.

I'm not sure it's worth worrying about, and if you've changed all the loadManifestFrom calls to validate the archive then that takes care of it anyway.

> This patch effectively kills the old XPI signature verification for other
> applications, is that what we want?

I wrestle with that. My initial patches preserved it but it made the code more complex. For apps that aren't going to require AMO-signing (SeaMonkey, Thunderbird?) there's still a small amount of value to CA-issued code-signing in terms of tamper resistance. I'm not convinced, however, that more than a handful of paranoid users would notice when a package that was supposed to be signed was not since not being signed is the usual case. And since our UI didn't show the certificate itself, just the common name, it was of limited value anyway.

And then a fair number of SeaMonkey add-ons are actually also Firefox addons targeting both apps. If SM supported (but didn't require) the newstyle signing then they'll actually be more secure when installing AMO addons because they'll all be signed (but they would appear unsigned if SM does old-style verification). Ditto for Thunderbird add-ons (though less overlap).

So in the end I leaned toward ripping the old stuff out.

Comment 10

[Dave Townsend [:mossop]]

(In reply to Daniel Veditz [:dveditz] from comment #9)
> (In reply to Dave Townsend [:mossop] from comment #8)
> > A comment you put in mentions "TOCTOU window". What is that?
> 
> "Time of Check/Time of Use" -- a class of potential security vulns. I think
> at that spot I was considering not revalidating the manifest (perf concerns)
> because it had just been checked a couple of dozen lines earlier.
> Technically that would mean a local attacking program could monitor Firefox
> and after it validated the file the first time swap in a malicious add-on
> that we would then just load (if we skipped validation the second time). But
> unless the malicious side program was actively swapping the add-on in and
> out we'd eventually detect and disable it on a future startup. Doesn't seem
> like the most plausible attack if you already presume malware that can
> monitor your process that closely.
> 
> I'm not sure it's worth worrying about, and if you've changed all the
> loadManifestFrom calls to validate the archive then that takes care of it
> anyway.

The code that this comment was in is the code that handles migrating staged installs from pre-Firefox 4.0 days. I think I'll take the opportunity to rip that code out. While not particularly complicated it does add another potential vector for slipping add-ons into Firefox and I'm guessing that it has outlived its usefulness.

Tyler, I'm told you might be able to get me numbers on how many people upgrade from pre-Firefox 4.0 to current release versions?

Comment 11

[Tyler Downer [He/Him]]

To cover what was said in IRC, very few users will be updating directly from <4 to Firefox current, they will go from 3.6->12->Current. The only users who would see this are those who have installed staged and at the same time download the full installer to go straight to Current. I'm going to say that is a pretty small population.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 8587751 [details] [diff] [review]
patch

Review of attachment 8587751 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I had tried to do the signature verification first so I wouldn't have to parse the zipfile twice (assuming it succeeded) due to perf worries. But this way simplifies the code and saves looking for signatures in add-on types that won't have them.

::: browser/app/profile/firefox.js
@@ +69,5 @@
>  // See the SCOPE constants in AddonManager.jsm for values to use here.
>  pref("extensions.autoDisableScopes", 15);
>  
> +// Require signed add-ons by default
> +pref("xpinstall.signatures.required", true);

I worry about this being the state in the initial check-in: lack of a warning period is going to be extremely painful and contentious. (Of course my version had it this way, too, but I wasn't entirely happy about that.)

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +4212,5 @@
>          this.minCompatibleAppVersion = Preferences.get(PREF_EM_MIN_COMPAT_APP_VERSION,
>                                                         null);
>          this.minCompatiblePlatformVersion = Preferences.get(PREF_EM_MIN_COMPAT_PLATFORM_VERSION,
>                                                              null);
> +      case PREF_XPI_SIGNATURES_REQUIRED:

You should add a  "// fallthrough" comment or someone might come along and "fix" the missing break. Or just go ahead and duplicate "this.updateAddonAppDisabledStates(); break;" for clarity

Comment 13

[Dave Townsend [:mossop]]

Attachment: talos patch

This fixes talos to allow unsigned extensions to work. It is a stop-gap measure until we can get bug 1132995 fixed.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8588621 [details] [diff] [review]
talos patch

Review of attachment 8588621 [details] [diff] [review]:
-----------------------------------------------------------------

how fast do you need this to be deployed?  Can it wait a week?  will there be issues if it ends up on aurora/beta?

Comment 15

[Dave Townsend [:mossop]]

(In reply to Joel Maher (:jmaher) from comment #14)
> Comment on attachment 8588621 [details] [diff] [review]
> talos patch
> 
> Review of attachment 8588621 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> how fast do you need this to be deployed?  Can it wait a week?  will there
> be issues if it ends up on aurora/beta?

It would block this bug landing and I was hoping to get this through review and landed in the next few days. There is no issue with this ending up on aurora/beta/release.

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

ok, I just updated talos not even an hour ago- it isn't hard to do, but it would have saved a bit of infrastructure.  I don't have immediate changes landing soon (at least until next week), so lets deploy this sooner.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

https://hg.mozilla.org/build/talos/rev/476d6186aa56

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: talos.json updates required for this (1.0)

I have a talos.zip created and all the other stuff ready, this talos.json will update talos when you are ready for it.

Comment 19

[Dave Townsend [:mossop]]

Attachment: patch

Current WIP for basing other work on. I've brought back support for old-style signatures in the case where the new signing requirements are disabled so that other applications that are signing hotfixes will still work.

Comment 20

[Dave Townsend [:mossop]]

Attachment: patch

Updated WIP with actual real signed add-ons in the automated tests. Preffed off by default. Just need the final string from bug 1149696 and this should be ready for review.

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8588628 [details] [diff] [review]
talos.json updates required for this (1.0)

landed the talos pref change on inbound.

Comment 22

[Dave Townsend [:mossop]]

Attachment: patch

Chatted with Markus in UX and we're going to go with the string in this patch for now so I think we're ready to land this. Dan seems like you should know enough to review this but if you'd like me to also get someone else to look over the add-ons manager changes let me know. Also you wrote the bits adding the cert, they seem straightforward but let me know if they could do with additional review.

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8588628 [details] [diff] [review]
talos.json updates required for this (1.0)

this was backed out, in fact this patch is safe to land it was the other preference which needed backing out.

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 8590491 [details] [diff] [review]
patch

Review of attachment 8590491 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good except for some little stuff about error cases.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +75,5 @@
>  addonError-1=The add-on could not be downloaded because of a connection failure on #2.
>  addonError-2=The add-on from #2 could not be installed because it does not match the add-on #3 expected.
>  addonError-3=The add-on downloaded from #2 could not be installed because it appears to be corrupt.
>  addonError-4=#1 could not be installed because #3 cannot modify the needed file.
> +addonError-5=#3 has prevented this site from installing an uncertified add-on.

Is "uncertified" the term we're going to use rather than "unsigned"?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +1333,5 @@
> +        // was no cert found.
> +        if (REQUIRE_SIGNING || Preferences.get(PREF_XPI_SIGNATURES_REQUIRED, false))
> +          resolve(AddonManager.SIGNEDSTATE_BROKEN);
> +        else
> +          resolve(AddonManager.SIGNEDSTATE_MISSING);

This glosses over the important distinction between a signature we don't accept and a valid signature with a tampered archive. If something is signed by a new cert, but doesn't validate it could be an attack. It could also be a developer playing with stuff locally, but in that case it's not hard for them to remove the *.rsa file and make it unsigned rather than invalid.

Errors that I think should == SIGNEDSTATE_BROKEN (because they imply the cert itself checked out OK) are:
  NS_ERROR_SIGNED_JAR_MANIFEST_INVALID
  NS_ERROR_SIGNED_JAR_ENTRY_INVALID
  NS_ERROR_SIGNED_JAR_MODIFIED_ENTRY
  NS_ERROR_SIGNED_JAR_UNSIGNED_ENTRY
  NS_ERROR_SIGNED_JAR_ENTRY_MISSING

I tried to go the other way, figure out which errors meant a bad/unknown signature, but quickly got lost in the bowels of mozilla::pkix and NSS. Too many possible errors, some fraction of
https://dxr.mozilla.org/mozilla-central/source/security/pkix/include/pkix/Result.h#86

@@ -2423,5 @@
>        // We can't install or uninstall anything in locked locations
>        if (aLocation.locked)
>          return;
>  
> -      let stagedXPIDir = aLocation.getXPIStagingDir();

This stuff being removed is for compatibility with staged installs left over from much older Firefox versions? I agree we don't need to support that case any longer.

@@ +5350,1 @@
>      let x509 = zipreader.getSigningCert(null);

You really only want to do this if SIGNEDSTATE_MISSING. if you've gotten a newfile signature then this getSigningCert() will reparse the manifest, re-check signatures, and then get an error if it's a new-style signature because those don't chain to a CA.

This will throw if it gets that kind of error, bypassing the rest of the install.

Comment 25

[Dave Townsend [:mossop]]

Attachment: patch

This addresses most of the concerns I made a couple of slight changes though:

For the case where the add-on appears to have a signature that doesn't chain to the trusted root I added a new signedState flag, this allows us to only check that signing in that case, it also made a bit more sense than using either missing or broken for that case.

When signing isn't required we just ignore the broken case where previously this would stop the add-on working, this makes the signing toggle cleaner, when off it acts exactly like current Firefox, so if we needed to disable this while riding the trains it would be a simple pref change rather than anything more involved.

I haven't changed the UI string yet as we're waiting on copy from this morning's meeting.

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 8591858 [details] [diff] [review]
patch

Review of attachment 8591858 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks great. r=dveditz

Comment 27

[Dave Townsend [:mossop]]

Attachment: patch

Final patch, only change is the wording to use the new verified term instead of certified.

Comment 28

[Dave Townsend [:mossop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a48282c6b3f1

Comment 29

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/3b48e1a81a17

Comment 30

[Dave Townsend [:mossop]]

For some reason one xpcshell test is crashing on windows. The crash is in the test code rather than the new code that is landing so I've chosen to land with this test disabled and I'll work to fix it in bug 1156985.

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

xpcshell was failing, mossop thought a clobber would fix it, I clobbered the tree and rebuilt, xpcshell was still failing.

Backed out in https://hg.mozilla.org/integration/fx-team/rev/19ae21373658

https://treeherder.mozilla.org/logviewer.html#?job_id=2779891&repo=fx-team

Comment 32

[Dave Townsend [:mossop]]

Tried pushing again with an IID rev and CLOBBER change since I don't understand why this would be failing otherwise: https://hg.mozilla.org/integration/fx-team/rev/f99621542727

Comment 33

[Carsten Book [:Tomcat]]

sorry had to back this out again for causing failures like in https://bugzilla.mozilla.org/show_bug.cgi?id=1157583 and https://treeherder.mozilla.org/logviewer.html#?job_id=2795435&repo=fx-team

Comment 34

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/cac8a0359113

Comment 35

[Carsten Book [:Tomcat]]

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

Comment 36

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #35)
> https://hg.mozilla.org/mozilla-central/rev/f99621542727

ignore that, my bad used the wrong cset for the merge and fixed this

Comment 37

[Dave Townsend [:mossop]]

Attachment: log all failures to write extensions database

The intermittent failures caused by this are because in some cases we don't catch the error if saving the extensions database fails. The new code seems to change exactly when the save is attempted in the case of a corrupt database. That test is itself broken (bug 1157923) but we can get rid of the failure caused by this patch by just always catching the failing promise and logging the error.

Comment 38

[Dave Townsend [:mossop]]

Attachment: log all failures to write extensions database

Accidentally included some stuff that shouldn't be in there

Comment 39

[Daniel Veditz [:dveditz]]

Comment on attachment 8597362 [details] [diff] [review]
log all failures to write extensions database

Review of attachment 8597362 [details] [diff] [review]:
-----------------------------------------------------------------

r=dveditz

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ebc84b792237
https://hg.mozilla.org/integration/fx-team/rev/1cfe7aee4508

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ebc84b792237
https://hg.mozilla.org/mozilla-central/rev/1cfe7aee4508

Comment 42

[Vasilica Tamas, QA [:vtamas]]

Confirmed fixed on Firefox 40.0a2 (2015-05-26/27) and Firefox 41.0a1 (2015-05-27) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5. 
The xpinstall.signatures.required pref exists and is set to false by default.