Closed
Bug 1038098
Opened 10 years ago
Closed 10 years ago
Add-ons can not be installed; 'Installation failed' toast notification is displayed
Categories
(Firefox for Android Graveyard :: Add-on Manager, defect)
Tracking
(firefox30 unaffected, firefox31 unaffected, firefox32 unaffected, firefox33- verified, fennec33+)
VERIFIED
FIXED
Firefox 33
Tracking | Status | |
---|---|---|
firefox30 | --- | unaffected |
firefox31 | --- | unaffected |
firefox32 | --- | unaffected |
firefox33 | - | verified |
fennec | 33+ | --- |
People
(Reporter: cos_flaviu, Assigned: briansmith)
References
Details
(Keywords: regression, reproducible)
Attachments
(1 file)
(deleted),
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
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•10 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•10 years ago
|
Keywords: regression
Comment 2•10 years ago
|
||
Can you try to narrow down the regression range using fxteam and mozilla-inbound builds?
Reporter | ||
Comment 3•10 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
Comment 4•10 years ago
|
||
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
Comment 5•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
(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•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Target Milestone: --- → Firefox 33
Assignee | ||
Comment 8•10 years ago
|
||
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)
Comment 9•10 years ago
|
||
> 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+
Assignee | ||
Comment 11•10 years ago
|
||
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•10 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.
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a6389627c3a4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
tracking-fennec: ? → 33+
Updated•10 years ago
|
Reporter | ||
Comment 15•10 years ago
|
||
Verified as fixed in build 33.0a1 (2014-07-21); Device Samsung Galaxy R (Android 2.3.4);
Status: RESOLVED → VERIFIED
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•