1038098 - Add-ons can not be installed; 'Installation failed' toast notification is displayed

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Add-on Manager, defect)

Description

[Flaviu Cos]

Environment: 
Device: Google Nexus 7 (Android 4.4.4);
Build: Nightly 33.0a1 (2014-07-14);

Steps to reproduce:
1. Go to addons.mozilla.org;
2. Try to install any add-on (e.g. Adblock Plus).


Expected result:
The add-on is successfully installed.

Actual result:
The add-on can not be installed. 'Installation failed' toast notification is displayed.

Notes:

Comment 1

[Flaviu Cos]

Last good build: 2014-07-09;
First bad build: 2014-07-10;
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=196d05832e12&tochange=cb75d6cfb004

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Can you try to narrow down the regression range using fxteam and mozilla-inbound builds?

Comment 3

[Flaviu Cos]

Last good build: 19663c22e7eb
First bad build: 3a4d57c7ffdf
 
mozilla-inbound pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=19663c22e7eb&tochange=3a4d57c7ffdf

Comment 4

[Aaron Train [:aaronmt]]

[e-mail]
Wed Jul 09 17:41:36 2014 +0000	3a4d57c7ffdf	Brian Smith — Bug 1035942: Decide whether to consider end-entity CN as a dnsName in CheckNameConstraints instead of in BuildCertChain, r=cviecco

0ed88d692f42	Brian Smith — Bug 1035009: Stop using CERTCertList in mozilla::pkix, r=keeler

Can we try to back-out the culprit?

Comment 5

[Aaron Train [:aaronmt]]

In console on attempt to install an add-on

E/GeckoConsole( 5727): [JavaScript Error: "NS_ERROR_ABORT: Certificate issuer is not built-in." {file: "resource://gre/modules/CertUtils.jsm" line: 168}]

E/GeckoConsole( 5727): 1405346532572	addons.xpi	WARN	Download of https://addons.mozilla.org/android/downloads/latest/413482/platform:7/addon-413482-latest.xpi?src=homepagebrowse failed: 301 MOVED PERMANENTLY

I/Gecko   ( 5727): 1405346532572	addons.xpi	WARN	Download of https://addons.mozilla.org/android/downloads/latest/413482/platform:7/addon-413482-latest.xpi?src=homepagebrowse failed: 301 MOVED PERMANENTLY

Comment 6

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

Note that the code that implements that check is problematic. See bug 1002586.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Aaron Train [:aaronmt] from comment #4)
> 3a4d57c7ffdf	Brian Smith — Bug 1035942:
> Decide whether to consider end-entity CN as a dnsName in
> CheckNameConstraints instead of in BuildCertChain, r=cviecco

Almost definitely not this one.

> 0ed88d692f42	Brian Smith — Bug 1035009: Stop using CERTCertList in
> mozilla::pkix, r=keeler

This is probably the checkin that caused the regression.

> Can we try to back-out the culprit?

Backing out my patch would slightly lower my sense of self-worth. My therapist says that lowering my sense of self-worth would be a good thing, but I'm trying to prove her wrong. So, please give me a couple of hours to write the fix for the regression.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: fix-addon-installation-on-android.patch

My patch for bug 1035009 fails to properly handle the case where the caller to CertVerifier::VerifySSLServerCert passed saveIntermediatesInPermanentDatabase=true and builtChain=nullptr. In particular, it refuses to save the intermediate certificates into the permanent database unless saveIntermediatesInPermanentDatabase=true and builtChain != nullptr. This patch corrects this so that saveIntermediatesInPermanentDatabase=true is honored even when builtChain == nullptr.

The bug I described above causes addon installation to fail because the code that checks if the certificate was issued by a built-in CA is spectacularly terrible. However, the problem that David pointed out in comment 6 is not the issue. Rather, the issue is that this code makes the invalid assumption that, given only the end-entity certificate, it will be possible for NSS to magically find the intermediate certificates needed to build a certificate chain. This is not the case, in general. However, it is usually the case because we save the intermediate certificates in the permanent database, and then NSS can find them from there.

Note that any use of nsIX509Cert.getChain or nsIX509Cert.issuer or any other thing that attempts to build/verify the same (or similar) certificate chain that we built/verified during a TLS handshake will have the same issues.

It is likely that, due to the aforementioned problem, even after this patch lands you will not be able to install an addon in private browsing mode, at least on Android, because on private browsing mode, we don't store the intermediate certificates into NSS's permanent database. As David suggests, the best thing to do is to remove this "is builtin root" code and rely on the new (as of Firefox 32) key pinning logic to do it automatically for you. However, I am not sure if the key pinning feature is enabled on Android.

I suggest that somebody test what happens when one attempts to install an addon in private browsing mode (on Desktop and on Android), and file new bugs if it does not work.

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

> As David suggests,
> the best thing to do is to remove this "is builtin root" code and rely on
> the new (as of Firefox 32) key pinning logic to do it automatically for you.
> However, I am not sure if the key pinning feature is enabled on Android.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1019255

Comment 10

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

Comment on attachment 8455819 [details] [diff] [review]
fix-addon-installation-on-android.patch

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

::: security/certverifier/CertVerifier.cpp
@@ +443,5 @@
>    if (rv != SECSuccess) {
>      return rv;
>    }
>  
> +  if (saveIntermediatesInPermanentDatabase) {

We should probably comment that callers might specify saveIntermediatesInPermanentDatabase without specifying builtChain so we don't do the same refactoring in the future.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6389627c3a4

(In reply to David Keeler (:keeler) [use needinfo?] from comment #10)
> > +  if (saveIntermediatesInPermanentDatabase) {
> 
> We should probably comment that callers might specify
> saveIntermediatesInPermanentDatabase without specifying builtChain so we
> don't do the same refactoring in the future.

I don't like to add such comments, because such concerns apply to every function. We need to be more careful, though mistakes like this will always happen occasionally, especially when tests are missing or disabled, as is the case here.

Comment 12

[Flaviu Cos]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #8)
> 
> I suggest that somebody test what happens when one attempts to install an
> addon in private browsing mode (on Desktop and on Android), and file new
> bugs if it does not work.

The bug is also reproducible using private browsing on Android; logged bug 1038573 for this.
Can not reproduce this issue using private browsing on desktop Nightly.

Comment 13

[Flaviu Cos]

The bug is also reproducible in guest mode.

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Flaviu Cos]

Verified as fixed in build 33.0a1 (2014-07-21);
Device Samsung Galaxy R (Android 2.3.4);