Closed Bug 1267318 (armagadd-on) Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox46 + verified
firefox47 + verified
firefox48 + verified
firefox49 + verified
firefox-esr38 --- unaffected
firefox-esr45 46+ verified

People

(Reporter: jorgev, Assigned: dveditz)

References

Details

User Story

If I download an add-on it should continue to work unless it is blocklisted.

The AMO copy of the original add-on used has been re-signed and updated, the original STR will not work. I've attached an old copy of "Copy Short URL" that has an invalid signature. You may want to disable add-on updates while testing these because this add-on _will_ update to a fixed copy at some point. You can test in two ways:

Basic Install.
--------------
We're past the expiration date so no clock messing required
1. Try to install attachment 8746160 [details]

broken behavior: won't install due to invalid signature
fixed behavior: installs fine.

NOTE: the console error message Jorge reports will come up in both cases. That's a vestige of the old jar-signing code in nsJar.cpp (still used by Tbird and seamonkey in place of this new-style signing).

Failure as users will experience it
-----------------------------------
1. set clock back to April 22, 2016
2. install attachment 8746160 [details]
3. shut down Firefox
4. move clock back to today
5. restart Firefox.

broken behavior: some time after start (2-5 minutes? not sure what the delay value we used) you get an infobar saying an add-on was disabled

fixed behavior: hard to test a negative, but add-on never disabled.


Don't forget to re-enable your add-on updates unless you're using a throw-away test profile.

Attachments

(3 files, 2 obsolete files)

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.
Assignee: aswan → nobody
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.
Assignee: nobody → aswan
Assignee: aswan → nobody
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.
User Story: (updated)
User Story: (updated)
Does this need to block the release or can this be resolved by changes to AMO?
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.
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.
Depends on: 1267361
(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.
Didn't mean to set those flags - that was from while I was still not sure if we should block.
(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.
Blocks: 1267642
Am I to understand that anything that gets signed (even today) will expire in a year?
Blocks: 1267012
Attached patch bug1267318.patch (obsolete) — Splinter Review
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.
Attachment #8745511 - Flags: review?(dkeeler)
(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 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).
Attachment #8745511 - Flags: review?(dkeeler) → review+
(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.
changed the comment.
Assignee: nobody → dveditz
Attachment #8745511 - Attachment is obsolete: true
Attachment #8745654 - Flags: review+
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.
Attachment #8745654 - Flags: approval-mozilla-release?
Attachment #8745654 - Flags: approval-mozilla-esr45?
Attachment #8745654 - Flags: approval-mozilla-beta?
Attachment #8745654 - Flags: approval-mozilla-aurora?
Keywords: checkin-needed
Alias: armagadd-on
https://hg.mozilla.org/mozilla-central/rev/488df90abd62
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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.
Attachment #8745654 - Flags: approval-mozilla-release?
Attachment #8745654 - Flags: approval-mozilla-release+
Attachment #8745654 - Flags: approval-mozilla-esr45?
Attachment #8745654 - Flags: approval-mozilla-esr45+
Attachment #8745654 - Flags: approval-mozilla-beta?
Attachment #8745654 - Flags: approval-mozilla-beta+
Attachment #8745654 - Flags: approval-mozilla-aurora?
Attachment #8745654 - Flags: approval-mozilla-aurora+
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?
Flags: needinfo?(jorge)
Attachment #8746160 - Attachment mime type: application/x-install → application/x-xpinstall
Vasilica: I updated the "user story" with new STR.
User Story: (updated)
Flags: needinfo?(jorge)
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?
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.
We may not need an old addon (Krupa, kevin suggested testing by faking the date on the phone)
Flags: needinfo?(krupa.mozbugs)
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?
(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.
Attached file copy-short-android.xpi (obsolete) —
for Android support
Flags: needinfo?(krupa.mozbugs)
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)
Attachment #8747820 - Attachment is obsolete: true
Flags: qe-verify+
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
Flags: needinfo?(krupa.mozbugs)
(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?
(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.
Depends on: 1270100
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
Flags: needinfo?(krupa.mozbugs)
Flags: qe-verify+
(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.
Flags: needinfo?(dveditz)
See Also: → armagadd-on-2.0
You need to log in before you can comment on or make changes to this bug.