1267318 - (armagadd-on) Moving date forward to May 2nd is causing add-on signature "unrecognized issuer" errors

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Jorge Villalobos [:jorgev] (he/him)]

STR:
1) Set system clock to May 2nd, 2016
2) Install https://addons.mozilla.org/addon/bugzilla-tweaks/

Result:
The signature isn't recognized. The console shows:

Signature Verification Error: the signature on this .jar archive is invalid because the certificate used to sign this file has an unrecognized issuer.

Expected:

Add-on should install without any issues.

Tested on Beta 46, but it should happen with other Firefox versions and other add-ons.

Comment 1

[Daniel Veditz [:dveditz]]

Note that add-on was signed April 30, 2015. May 2nd is after the cert expired. I noticed this when 6 or so of my addons were disabled this morning, apparently signed on April 24. They had the same version as the AMO copies but an earlier signing date -- maybe an early test run of the autosigning infrastructure?

Anyway, as a stop-gap we need to re-sign and version bump (".2-signed") anything signed a year ago (say, older than 10 or 11 months), and plan to do that every month until we can change the product to deal with code-cert expiration.

If we change the cert parameters to have a longer lifetime we won't have to do it so often in the future.

Comment 2

[Daniel Veditz [:dveditz]]

I don't know that this should be a "Toolkit" addons bug. There are two issues:

1. Immediately we need to re-sign and version-bump our addons so users don't get broken. This is in the AMO backend. I'm sure it has to be done by hand, but we might still have the scripts we used to sign everything last year to build on.

2. The client code changes are down in the Gecko app verification code, which would be Core::Security: PSM I believe.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Does this need to block the release or can this be resolved by changes to AMO?

Comment 4

[Daniel Veditz [:dveditz]]

Update: AMO lives in github, so rtilder and jason are covering item #1 above in github issues. This bug will cover the client changes needed so we don't have to keep re-signing unchanged add-ons.

Comment 5

[Daniel Veditz [:dveditz]]

This doesn't need to delay tomorrow's release -- Firefox 45 has the same problem.

The AMO re-signing pass will take some of the heat off, but will not fix any add-on that isn't served from AMO (about half of them?). We need to try to get a code fix into the inevitable 46.0.1. If the change turns out to be scarier than expected maybe Fx47 at the latest, but that's going to be very painful.

Comment 6

[Adam Roach [:abr]]

(In reply to Daniel Veditz [:dveditz] from comment #5)
> The AMO re-signing pass will take some of the heat off, but will not fix any
> add-on that isn't served from AMO (about half of them?).

We need to contact as many of those as we reasonably can and let them know that they may need to re-sign their addons and bump the version.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Is this related to the test failures here: https://bugzilla.mozilla.org/show_bug.cgi?id=1267012 ?

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Didn't mean to set those flags - that was from while I was still not sure if we should block.

Comment 9

[Andy McKay]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #7)
> Is this related to the test failures here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1267012 ?

Its a similar reason, they should be signed with a longer cert until the change to ignore it lands.

Comment 10

[Mike Kaply [:mkaply]]

Am I to understand that anything that gets signed (even today) will expire in a year?

Comment 11

[Daniel Veditz [:dveditz]]

Attachment: bug1267318.patch

David: please review this change that removes the cert expiration check from mozilla-signed packages. Please check in particular the condition noted in the comments that I'm assuming NSS/pkix will only report "expired" if there's nothing serious wrong like a completely invalid cert or corrupt content.

Comment 12

[Daniel Veditz [:dveditz]]

(In reply to Mike Kaply [:mkaply] from comment #10)
> Am I to understand that anything that gets signed (even today) will expire
> in a year?

We are changing that, too. Since the signing is part of the AMO backend it is being tracking in a github issue with the AMO server code.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8745511 [details] [diff] [review]
bug1267318.patch

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

This is a fragile solution, but I guess that's where we are right now. Also, this would benefit from tests.

::: security/apps/AppSignatureVerification.cpp
@@ +661,5 @@
> +    //
> +    // Our package format doesn't support timestamps (nor do we have a
> +    // trusted 3rd party timestamper), but since we sign all of our apps and
> +    // add-ons ourselves we can trust ourselves not to mess with the clock
> +    // on the signing systems. We also have a revocation mechanism if we

Unfortunately, for app signature verification we don't do revocation checking - AppTrustDomain::CheckRevocation and ::IsChainValid (i.e. the functions that would be doing the revocation checking) don't do anything.

@@ +666,5 @@
> +    // need it. It's OK to ignore cert expiration under these conditions.
> +    //
> +    // This is an invalid approach if
> +    //  * we issue certs to let others sign their own packages
> +    //  * NSS returns "expired" when there are "worse" problems

I would s/NSS/mozilla::pkix/ here since that's really what's returning the error code in question.
In any case, I had a look through the code and from my understanding we currently won't mask any "worse" problems by doing this (the one caveat is that AppTrustDomain::CheckValidityIsAcceptable can be bypassed, but since that does nothing anyway, that's ok for now).

Comment 14

[Daniel Veditz [:dveditz]]

(In reply to Dana Keeler [:keeler] (use needinfo?) from comment #13)
> This is a fragile solution, but I guess that's where we are right now. Also,
> this would benefit from tests.

The tests turned off in bug 1267012 will cover this once we re-enable them.

> > +    // on the signing systems. We also have a revocation mechanism if we
> 
> Unfortunately, for app signature verification we don't do revocation
> checking - AppTrustDomain::CheckRevocation and ::IsChainValid (i.e. the
> functions that would be doing the revocation checking) don't do anything.

Technically true, but the blocklist is effectively revocation. At least for add-ons--doesn't help Marketplace Apps which presumably suffer the same issue.

Comment 15

[Daniel Veditz [:dveditz]]

Attachment: bug1267318.patch with comment change

changed the comment.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 8745654 [details] [diff] [review]
bug1267318.patch with comment change

Approval Request Comment
[Feature/regressing bug #]:
  Father Time
[User impact if declined]: 
  Add-ons stop working on all branches in a few weeks, especially non-AMO
  ones we can't re-sign.
[Describe test coverage new/current, TreeHerder]:
  The tests disabled in bug 1267012 cover this if that change is backed out.
[Risks and why]: 
  Low risk
[String/UUID change made/needed]:
  None.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/488df90abd62

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/488df90abd62

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8745654 [details] [diff] [review]
bug1267318.patch with comment change

Uplift all the way. We will need this for 46.0.1 and 46.1.1esr.

Comment 20

[Vasilica Tamas, QA [:vtamas]]

I was unable to reproduce this issue on Firefox 45.0.2 (20160407164938), Firefox 46 (20160421124000) and Firefox 48.0a1 (2016-04-25) under Windows 10 64-bit, Windows 7 64-bit and Mac OS X 10.9.5 while installing https://addons.mozilla.org/addon/bugzilla-tweaks/ via both methods (mentioned in User Story and Description ).

Is there any additional step or precondition that I should take care of? Or this add-on is no longer suitable for testing?

Comment 21

[Carsten Book [:Tomcat]]

pushed as approved per Liz

https://hg.mozilla.org/releases/mozilla-aurora/rev/d6d1b8b21016
https://hg.mozilla.org/releases/mozilla-beta/rev/e0cddb21b284
https://hg.mozilla.org/releases/mozilla-esr45/rev/b27938d37c78
https://hg.mozilla.org/releases/mozilla-release/rev/e7ba7fb7f6f1

Comment 22

[Daniel Veditz [:dveditz]]

Attachment: "Copy Short Url" signed 2015-04-24

Comment 23

[Daniel Veditz [:dveditz]]

Vasilica: I updated the "user story" with new STR.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

We should also make sure that the UI works correctly for Firefox for Android.  With (or without) the patch, what do users see when the certificate expires?

Comment 25

[Daniel Veditz [:dveditz]]

Unfortunately "Copy Short URL" (attached) doesn't work in Fennec. Now that we've re-signed things we'll have to do some digging to get an old copy of one of the Fennec-affected add-ons.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

We may not need an old addon (Krupa, kevin suggested testing by faking the date on the phone)

Comment 27

[T. Bug Reporter]

Does this mean that years from now I'll be updating my copy of CoolAbandonedExtension from Version
1.01.1-signed.1-signed.1-signed.1-signed.1-signed.1-signed.1-signed.1-signed.1-signed to Version
1.01.1-signed.1-signed.1-signed.1-signed.1-signed.1-signed.1-signed.1-signed.1-signed.1-signed?
Or will someone have come up with a less brain-dead re-signing script by then?

Comment 28

[Andy McKay]

(In reply to T. Bug Reporter from comment #27)
> Does this mean that years from now I'll be updating my copy of
> CoolAbandonedExtension from Version
> 1.01.1-signed.1-signed.1-signed.1-signed.1-signed.1-signed.1-signed.1-signed.
> 1-signed to Version
> 1.01.1-signed.1-signed.1-signed.1-signed.1-signed.1-signed.1-signed.1-signed.
> 1-signed.1-signed?
> Or will someone have come up with a less brain-dead re-signing script by
> then?

No. Resigning will not be necessary in the future once this has landed.

Comment 29

[krupa raj[:krupa]]

Attachment: copy-short-android.xpi

for Android support

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 8747820 [details]
copy-short-android.xpi

This one isn't signed correctly, it's not a valid test (although if it doesn't come up "corrupt" it's a sign you've turned off signatures)

Comment 31

[u421692]

Is there a way to test this in Fennec? 
I've tested with the destkop add-on in the attachment and I got the following results: 
On Fennec 47 Beta 1 I got an error saying that the add-on appears to be corrupt.
On Fennec 47 Beta 2 the error is not displayed, but add-on is not installed since it's not for Fennec 
On Fennec 46.0.1 the error is not displayed, but add-on is not installed since it's not for Fennec

Comment 32

[Vasilica Tamas, QA [:vtamas]]

(In reply to Daniel Veditz [:dveditz] from comment #23)
> Vasilica: I updated the "user story" with new STR.

Thanks Daniel.
I managed to reproduce the initial issue (by both methods) using “Copy Short Url” from Comment 22 on Firefox 48.0a1 (2016-04-22) under Windows 10 64-bit.

Both add-ons and webextensions are successfully installed on Firefox 49.0a1 (2016-05-02/03), Firefox 48.0a2 (2016-05-03), Firefox 47.0b2 (20160502152141), Firefox 46.0.1 (20160502172042) and Firefox 45.1.1esr (20160502160818) under Windows 10 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit. 



I’ve noticed that an error is thrown in browser console while disabling some add-ons:
NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.nukeSandbox] bootstrap.js:290:0

Is this an error that we should be concerned about?

Comment 33

[Andy McKay]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #32)
> I’ve noticed that an error is thrown in browser console while disabling some
> add-ons:
> NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057
> (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.nukeSandbox]
> bootstrap.js:290:0
> 
> Is this an error that we should be concerned about?

I don't think so, but it might make sense to file a seperate bug in Toolkit > Addons Manager.

Comment 34

[CosminB]

Attachment: skyziptm_acceleration_and_compression_proxy-0.2.6-fx+an-android.xpi

I managed to install the attached add-on (which has expired date) on Nightly 49.0a1 (20161-05-04), Aurora 48.0a2 (2016-05-04), Fennec 47.0b2 and Fennec 46.0.1 on a device with Android 4.2.1 and I did not encountered any errors.

Based on this testing, I'm clearing krupa's need info request

Comment 35

[Daniel Veditz [:dveditz]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #32)
> I’ve noticed that an error is thrown in browser console while disabling some
> add-ons:
> NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057
> (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.nukeSandbox]
> bootstrap.js:290:0
> 
> Is this an error that we should be concerned about?

Not with respect to this bug. I don't know if it's something the Copy Short URL author needs to worry about for the functionality of their add-on.