Closed
Bug 787155
Opened 12 years ago
Closed 11 years ago
crash in PKIX_List_GetItem (disable it on B2G)
Categories
(Core :: Security: PSM, defect, P1)
Tracking
()
People
(Reporter: wsmwk, Assigned: briansmith)
References
Details
(Keywords: crash, perf, Whiteboard: [tbird crash][b2g-crash][eta:2013-04-19][madrid])
Crash Data
Attachments
(2 files, 1 obsolete file)
49.10 KB,
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
45.14 KB,
patch
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-3e9ed381-d082-4f27-9c6d-926a22120828 . ============================================================= 0 nss3.dll PKIX_List_GetItem security/nss/lib/libpkix/pkix/util/pkix_list.c:1553 1 nss3.dll pkix_CacheCert_Lookup security/nss/lib/libpkix/pkix/util/pkix_tools.c:1072 2 nss3.dll pkix_Build_GatherCerts security/nss/lib/libpkix/pkix/top/pkix_build.c:1800 3 nss3.dll pkix_BuildForwardDepthFirstSearch security/nss/lib/libpkix/pkix/top/pkix_build.c:2377 4 nss3.dll pkix_Build_InitiateBuildChain security/nss/lib/libpkix/pkix/top/pkix_build.c:3615 5 nss3.dll PKIX_BuildChain security/nss/lib/libpkix/pkix/top/pkix_build.c:3786 6 nss3.dll CERT_PKIXVerifyCert security/nss/lib/certhigh/certvfypkix.c:2204 7 xul.dll nsNSSCertificate::hasValidEVOidTag security/manager/ssl/src/nsIdentityChecking.cpp:1189 8 xul.dll nsNSSCertificate::getValidEVOidTag security/manager/ssl/src/nsIdentityChecking.cpp:1224 9 xul.dll nsNSSCertificate::GetIsExtendedValidation security/manager/ssl/src/nsIdentityChecking.cpp:1250 firefox bp-ece0c31e-cdad-48fc-aa94-41dbf2120803 crashes in all releases of the past year. and all OS nothing especially useful in any of the crash comments afaict.
Updated•12 years ago
|
Blocks: pkix-default
Comment 1•11 years ago
|
||
I have this type of crash on SeaMonkey [1], there are many crashes on Firefox [2], so IMHO it isn't Thunderbird-specific. [1] https://crash-stats.mozilla.com/report/index/bp-e781b49c-7333-40be-85bc-f40c72121215 [2] https://crash-stats.mozilla.com/report/list?signature=PKIX_List_GetItem
Updated•11 years ago
|
Component: Security → Security: PSM
Product: Thunderbird → Core
Comment 2•11 years ago
|
||
This caused an OS restart crash on b2g upon launching the homescreen. Doing the following STR: 1. Flash the 3/29 build 2. Complete FTE and enable data 3. Unlock lockscreen 4. Switched to homescreen on the right https://crash-stats.mozilla.com/report/index/bp-f298fc2c-8eca-43db-a36e-d7ca02130329 Brian - Any ideas? Why is the homescreen experiencing an OS restart crash here? Should we nom to block leo or tef?
status-b2g18:
--- → affected
tracking-b2g18:
--- → ?
Flags: needinfo?(bsmith)
Whiteboard: [tbird crash] → [tbird crash][b2g-crash]
Assignee | ||
Comment 3•11 years ago
|
||
I am working on a patch that will remove all of this code from B2G 1.0 at least, and I have another patch that will allow us to remove this code from (at least the default configuration of) Firefox and Thunderbird.
Assignee: nobody → bsmith
Flags: needinfo?(bsmith)
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #2) > Why is the homescreen experiencing an OS restart crash > here? This code (PKIX_*) is executed during the course of validating an SSL certificate for a site that has an Extended Validation (EV) certificate. This is the code that decides, in non-B2G products, whether the lock icon should be green or gray. Since we don't make that distinction in B2G (especially for apps where there isn't even an address bar), we can disable this functionality for B2G. Not only will this work around this problem, but it will be a significant performance enhancement for these EV sites due to the savings of revocation checking overhead. > Should we nom to block leo or tef? Sure, it seems reasonable to nom this to block. I will have a patch very soon to disable this code path.
Comment 5•11 years ago
|
||
I think Brian intended to nom this as a blocker.
blocking-b2g: --- → tef?
tracking-b2g18:
? → ---
Comment 6•11 years ago
|
||
Oh, and once we have a patch, an STR showing this was the cause would be nice.
Updated•11 years ago
|
blocking-b2g: tef? → tef+
Updated•11 years ago
|
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → affected
Assignee | ||
Updated•11 years ago
|
Priority: -- → P1
Whiteboard: [tbird crash][b2g-crash] → [tbird crash][b2g-crash][eta:2013-04-15]
Target Milestone: --- → mozilla23
Comment 7•11 years ago
|
||
Hi Brian. Past 4/15 now - do you have a new ETA for this?
Whiteboard: [tbird crash][b2g-crash][eta:2013-04-15] → [tbird crash][b2g-crash][eta:2013-04-15][status: needs new ETA]
Assignee | ||
Comment 8•11 years ago
|
||
Sorry, I meant to write "2013-04-16" as the original ETA anyway. I have a patch that I need to run through try and get reviewed. This is a simple patch (#ifdefing out some unneeded code), so it should go quickly.
Whiteboard: [tbird crash][b2g-crash][eta:2013-04-15][status: needs new ETA] → [tbird crash][b2g-crash][eta:2013-04-16]
Assignee | ||
Comment 9•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=d52de52751a0 ^ This patch is a superset of the patch that will fix this bug. Pushing back ETA because cviecco has limited availability and it may take longer to get a review, once I post a patch for review, tomorrow, Pacific time.
Whiteboard: [tbird crash][b2g-crash][eta:2013-04-16] → [tbird crash][b2g-crash][eta:2013-04-19]
Updated•11 years ago
|
Whiteboard: [tbird crash][b2g-crash][eta:2013-04-19] → [tbird crash][b2g-crash][eta:2013-04-19][madrid]
Updated•11 years ago
|
Target Milestone: mozilla23 → Madrid (19apr)
Assignee | ||
Comment 10•11 years ago
|
||
This patch fixes the bug by ensuring that libpkix never runs in B2G (PKIX_List_GetItem is part of libpkix). The only thing we use libpkix for is the EV indicator, and B2G doesn't have an EV indicator anyway. Not only will this fix the crash, but it will be a "free" performance improvement for EV sites because (a) we don't do the extra certificate verification to see if the cert is EV, (b) we don't do the extra OCSP checks that are part of that extra certificate verification, and (c) we don't have to do the "GetPreviousCert" trip through the main thread's event loop to do the caching of the EV status.
Attachment #739380 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 11•11 years ago
|
||
I filed NSS bug 863534 for fixing this more generally in NSS.
Updated•11 years ago
|
Whiteboard: [tbird crash][b2g-crash][eta:2013-04-19][madrid] → [tbird crash][b2g-crash][eta:2013-04-19][madrid][awaiting review]
Comment 12•11 years ago
|
||
I thought we had a flag to switch PKIX usage on and off (nsNSSComponent::globalConstFlagUsePKIXVerification), why is there so much changes needed in the patch then?
Updated•11 years ago
|
Summary: crash in PKIX_List_GetItem → crash in PKIX_List_GetItem (disable it on B2G)
Comment 13•11 years ago
|
||
Comment on attachment 739380 [details] [diff] [review] Do not use libpkix in B2G at all, to avoid crash in PKIX_List_GetItem How this patch fixes this crash https://crash-stats.mozilla.com/report/index/5ef6e795-c9f4-4ede-932a-b22fe2130419 ?
Comment 14•11 years ago
|
||
Comment on attachment 739380 [details] [diff] [review] Do not use libpkix in B2G at all, to avoid crash in PKIX_List_GetItem Review of attachment 739380 [details] [diff] [review]: ----------------------------------------------------------------- This fixes the only one crash ever reported on B2G: https://crash-stats.mozilla.com/report/list?product=B2G&query_search=signature&query_type=contains&query=PKIX_List_GetItem&reason_type=contains&date=04%2F19%2F2013%2015%3A55%3A21&range_value=2&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=PKIX_List_GetItem ::: security/manager/ssl/src/nsIdentityChecking.cpp @@ +1308,5 @@ > > nsresult > nsNSSCertificate::getValidEVOidTag(SECOidTag &resultOidTag, bool &validEV) > { > +#ifdef NSS_NO_LIBPKIX Looks like you cannot get here according the previous #ifndef condition. Leftover? Since you don't declare the method for no-libpkix too. @@ +1382,5 @@ > > outDottedOid = oid_str; > PR_smprintf_free(oid_str); > } > +#endif #else outDottedOid.Truncate(); or return NS_ERROR_FAILURE; #endif ::: security/manager/ssl/src/nsNSSCertHelper.cpp @@ +2097,5 @@ > return rv; > > if (!validEV) > ev_oid_tag = SEC_OID_UNKNOWN; > +#endif Nit: Just put this #endif above one line? ::: security/manager/ssl/src/nsNSSCertificate.cpp @@ +1277,5 @@ > + return nsrv; > + RefPtr<nsCERTValInParamWrapper> survivingParams; > + nsrv = inss->GetDefaultCERTValInParam(survivingParams); > + if (NS_FAILED(nsrv)) > + return nsrv; This check seems to be important to me for both compilation variations. It's on purpose to check we can instantiate the component prior any NSS calls here (PSM panic). ::: security/manager/ssl/src/nsNSSCertificateDB.cpp @@ +582,5 @@ > + return nsrv; > + RefPtr<nsCERTValInParamWrapper> survivingParams; > + nsrv = inss->GetDefaultCERTValInParam(survivingParams); > + if (NS_FAILED(nsrv)) > + return nsrv; Same concern about checking nss component here. I'd rather leave it as is. More places here in this file. Also GetDefaultCERTValInParam doesn't seems not to PKIX_List_GetItem at all. Maybe I don't see the reason, just explain in the code. ::: security/manager/ssl/src/nsSSLStatus.cpp @@ +6,5 @@ > > #include "nsSSLStatus.h" > #include "plstr.h" > #include "nsIClassInfoImpl.h" > +#include "nsIIdentityInfo.h" Why needed?
Attachment #739380 -
Flags: review?(honzab.moz) → review+
Updated•11 years ago
|
Whiteboard: [tbird crash][b2g-crash][eta:2013-04-19][madrid][awaiting review] → [tbird crash][b2g-crash][eta:2013-04-19][madrid][awaiting review response from bsmith]
Assignee | ||
Comment 15•11 years ago
|
||
Comment on attachment 739380 [details] [diff] [review] Do not use libpkix in B2G at all, to avoid crash in PKIX_List_GetItem Review of attachment 739380 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the quick review, Honza. There are a few layers to this patch. Perhaps I should have broken it up into fewer patches. First, the most important change is the change to nsIdentityChecking.cpp to ensure that we never do the EV checks, because EV checks always use libpkix, regardless of the value of globalConstFlagUsePKIXVerification. This change alone would be enough to fix this bug, AFAICT. Second, all the other callers of CERT_PKIXVerifyCert are #ifdefed out just to be extra careful. Third, since we're not doing EV validation any more for B2G (because there is no UI for it yet), we might as well #ifdef out all of the extra stuff that was added just to support EV validation. This is why all the DefaultCERTValInParam stuff and the changes to nsNSSIOLayer.cpp were done. Fourth, there is another part to this patch, which I will post in a follow-up bug, that makes it possible to build NSS without libpkix at all, to save some space in NSS on B2G. (libpkix is a fairly large part of NSS, in terms of object code size and in terms of lines of code and complexity.) This is another reason I #ifdef'd out every call to CERT_PKIXVerifyCert, because otherwise linking to the minimized NSS would fail. ::: security/manager/ssl/src/nsIdentityChecking.cpp @@ +1308,5 @@ > > nsresult > nsNSSCertificate::getValidEVOidTag(SECOidTag &resultOidTag, bool &validEV) > { > +#ifdef NSS_NO_LIBPKIX Good catch. Fixed. @@ +1382,5 @@ > > outDottedOid = oid_str; > PR_smprintf_free(oid_str); > } > +#endif Fixed by doing outDottedOid.Truncate() at the start of the function. ::: security/manager/ssl/src/nsNSSCertHelper.cpp @@ +2097,5 @@ > return rv; > > if (!validEV) > ev_oid_tag = SEC_OID_UNKNOWN; > +#endif I think it is clearer the way I wrote it, so I am leaving it as-is. ::: security/manager/ssl/src/nsNSSCertificate.cpp @@ +1277,5 @@ > + return nsrv; > + RefPtr<nsCERTValInParamWrapper> survivingParams; > + nsrv = inss->GetDefaultCERTValInParam(survivingParams); > + if (NS_FAILED(nsrv)) > + return nsrv; The do_GetService for nsINSSComponent was only added when the globalConstFlagUsePKIXVerification option was added, so it should only be needed for the libpkix case. If this is a problem, then it would be a problem in many other methods of this class that also assume that PSM/NSS has been initialized. I think the best thing to do is check that PSM was instantiated in nsNSSCertificate::Create so that we don't have to worry about initializing PSM/NSS in any nsNSSCertificate method. I filed a bug for that in 863860.
Assignee | ||
Comment 16•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9405d359a22
Comment 17•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f9405d359a22
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 18•11 years ago
|
||
This will need a branch-specific patch for the b2g18 uplift.
Flags: needinfo?(bsmith)
Whiteboard: [tbird crash][b2g-crash][eta:2013-04-19][madrid][awaiting review response from bsmith] → [tbird crash][b2g-crash][eta:2013-04-19][madrid]
Comment 19•11 years ago
|
||
This broke my build on osx 10.7.5, the problem is pretty obvious: /Users/shanec/moz/m-c/security/manager/ssl/src/nsNSSCertificateDB.cpp:1362:73: error: use of undeclared identifier 'nsrv' nsCOMPtr<nsINSSComponent> inss = do_GetService(kNSSComponentCID, &nsrv); ^ /Users/shanec/moz/m-c/security/manager/ssl/src/nsNSSCertificateDB.cpp:1364:16: error: use of undeclared identifier 'nsrv' return nsrv;
Comment 20•11 years ago
|
||
Comment on attachment 739380 [details] [diff] [review] Do not use libpkix in B2G at all, to avoid crash in PKIX_List_GetItem >@@ -1349,34 +1343,45 @@ nsNSSCertificateDB::FindCertByEmailAddre >+#ifndef NSS_NO_LIBPKIX > } > else { >+ nsCOMPtr<nsINSSComponent> inss = do_GetService(kNSSComponentCID, &nsrv); >+ if (!inss) >+ return nsrv; >+ RefPtr<nsCERTValInParamWrapper> survivingParams; >+ nsresult nsrv = inss->GetDefaultCERTValInParam(survivingParams); >+ if (NS_FAILED(nsrv)) >+ return nsrv; > CERTValOutParam cvout[1]; > cvout[0].type = cert_po_end; That is using nsrv before it is defined.
Assignee | ||
Comment 21•11 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #20) > That is using nsrv before it is defined. Fixed in bug 864633. It turns out that I made a mistake and defined NSS_NO_LIBPKIX everywhere. (In reply to Ryan VanderMeulen [:RyanVM] from comment #18) > This will need a branch-specific patch for the b2g18 uplift. I will merge the patch for bug 864633 into this patch, and merge it into b2g18.
Flags: needinfo?(bsmith)
Comment 22•11 years ago
|
||
Brian, it's getting pretty late in the game for tef blockers. Can you do the uplift soon please?
Flags: needinfo?(bsmith)
Assignee | ||
Comment 23•11 years ago
|
||
cviecco, this was a non-trivial merge due to many conflicting changes. Please sanity-check it for me.
Attachment #747649 -
Flags: review?(cviecco)
Flags: needinfo?(bsmith)
Comment 24•11 years ago
|
||
Comment on attachment 747649 [details] [diff] [review] Patch merged to mozilla-b2g18 + fix for bug 864633 Review of attachment 747649 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. EV is avoided.
Attachment #747649 -
Flags: review?(cviecco) → review+
Comment 25•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/018993e4de3c https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/18c9efb9a30b
Comment 26•11 years ago
|
||
Backed out for busting desktop builds. https://hg.mozilla.org/releases/mozilla-b2g18/rev/52bc8679ad01 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/06bf6a703734
Updated•11 years ago
|
Keywords: branch-patch-needed
Comment 27•11 years ago
|
||
Attachment #747649 -
Attachment is obsolete: true
Comment 28•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b6a4c1387ea1 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3223dcb52dbe
Comment 29•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6a8c8e993feb
Comment 30•11 years ago
|
||
Was the change on the way the intermediate certificates are validated intended? https://uapush-nv.srv.openwebdevice.com/about (signed with a Versign intermediate certificate that isn't supplied by the server) worked before this patch (and still works if I revert the changes) but it doesn't anymore. It fails with "The certificate is not trusted because no issuer chain was provided." We can fix that server side, but the thing is that the server works correctly on Firefox desktop, IE, and "old" B2G builds.
Updated•11 years ago
|
Flags: needinfo?(bsmith)
Assignee | ||
Comment 31•11 years ago
|
||
(In reply to Antonio Manuel Amaya Calvo from comment #30) > Was the change on the way the intermediate certificates are validated > intended? It's complicated. I will try to be precise: 1. The behavior you see is expected because Gecko doesn't try to fetch intermediate certificates through the AIA mechanism. 2. It is *unexpected* that there is any change in behavior here though, since your server isn't even EV. Is it possible that you are noticing the effects of a race condition? In particular, is it possible that when it "worked" your device had already made a connection to another server that did supply the intermediate, before your connection to https://uapush-nv.srv.openwebdevice.com/about? If so, that first connection that supplied the intermediate cert would have caused Gecko to cache the intermediate such that the subsequent connection to https://uapush-nv.srv.openwebdevice.com/about would succeed.
Flags: needinfo?(bsmith)
Assignee | ||
Comment 32•11 years ago
|
||
(In reply to Antonio Manuel Amaya Calvo from comment #30) > We can fix that server side, but the thing is that the server works > correctly on Firefox desktop, IE, and "old" B2G builds. I tried it in Nightly desktop AND Firefox Beta desktop with a *new* profile and I got the same error in both: uapush-nv.srv.openwebdevice.com uses an invalid security certificate. The certificate is not trusted because no issuer chain was provided. (Error code: sec_error_unknown_issuer)
Comment 33•11 years ago
|
||
(In reply to Brian Smith (:bsmith) from comment #32) > (In reply to Antonio Manuel Amaya Calvo from comment #30) > > We can fix that server side, but the thing is that the server works > > correctly on Firefox desktop, IE, and "old" B2G builds. > > I tried it in Nightly desktop AND Firefox Beta desktop with a *new* profile > and I got the same error in both: > > uapush-nv.srv.openwebdevice.com uses an invalid security certificate. > The certificate is not trusted because no issuer chain was provided. > (Error code: sec_error_unknown_issuer) I just tried with a 20.0 with a new profile and you're right, it failed... and then a little while after it worked, without me visiting any site. I think https://versioncheck.addons.mozilla.org/ is the culprit of that :). B2G, though, was a fresh install (compiled after doing a backout of this patch, flashed and tried directly)... maybe some of the sites it autoconnects uses the same CA too and I just was too slow checking. I didn't knew the intermediate CAs supplied by other sites were cached and reused. Ok, since this is an expected (yet strange) behavior, we'll just fix up the server (as we were going to do anyway :) ) and consider this closed. Thanks Brian!
See Also: → 1149279
You need to log in
before you can comment on or make changes to this bug.
Description
•