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

VERIFIED FIXED in Firefox 33

Status

()

Firefox for Android
Add-on Manager
--
major
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: Flaviu Cos, Assigned: briansmith)

Tracking

({regression, reproducible})

Trunk
Firefox 33
ARM
Android
regression, reproducible
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox30 unaffected, firefox31 unaffected, firefox32 unaffected, firefox33- verified, fennec33+)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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:
(Reporter)

Comment 1

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

Updated

3 years ago
Keywords: regression
Can you try to narrow down the regression range using fxteam and mozilla-inbound builds?
(Reporter)

Comment 3

3 years ago
Last good build: 19663c22e7eb
First bad build: 3a4d57c7ffdf
 
mozilla-inbound pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=19663c22e7eb&tochange=3a4d57c7ffdf
brian@briansmith.org
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?
Severity: normal → major
tracking-fennec: --- → ?
tracking-firefox33: --- → ?
Flags: needinfo?(brian)
Keywords: reproducible
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
Note that the code that implements that check is problematic. See bug 1002586.
See Also: → bug 1002586
(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.
Assignee: nobody → brian
Flags: needinfo?(brian)

Updated

3 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 33
Created attachment 8455819 [details] [diff] [review]
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.
Attachment #8455819 - Flags: review?(dkeeler)
Blocks: 1035009
> 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 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.
Attachment #8455819 - Flags: review?(dkeeler) → review+
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.
(Reporter)

Comment 12

3 years ago
(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.
(Reporter)

Comment 13

3 years ago
The bug is also reproducible in guest mode.
https://hg.mozilla.org/mozilla-central/rev/a6389627c3a4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Blocks: 950025
Blocks: 1037432
tracking-fennec: ? → 33+
tracking-firefox33: ? → -
(Reporter)

Comment 15

3 years ago
Verified as fixed in build 33.0a1 (2014-07-21);
Device Samsung Galaxy R (Android 2.3.4);
Status: RESOLVED → VERIFIED
status-firefox33: affected → verified
You need to log in before you can comment on or make changes to this bug.