Comment on attachment 802469 [details] [diff] [review] psm-workaround-for-verifycert (v2) Review of attachment 802469 [details] [diff] [review]: ----------------------------------------------------------------- r- for a bit of cleanup and because I think the comment could be more clear. ::: security/manager/ssl/src/CertVerifier.cpp @@ +101,5 @@ > * there being any differnce in results between CERT_VerifyCert and > * CERT_VerifyCertificate. > */ > + > +#ifdef BUG_910438_FIXED Instead of using #ifdefs, why don't we just file a bug to revert this change when bug 910438 is fixed and mozilla-central is using the version of NSS it shipped with? @@ +106,4 @@ > rv = CERT_VerifyCert(CERT_GetDefaultCertDB(), cert, true, > certUsageSSLServer, time, pinArg, verifyLog); > +#else > + /* BUG_910438: CERT_VerifyCert returns differently on error than on success This comment is a bit unclear. Bug 910438 comment 6 seemed clear to me, so maybe adapt that for use here. @@ +114,5 @@ > + rv = CERT_VerifyCert(CERT_GetDefaultCertDB(), cert, true, > + certUsageSSLServer, time, pinArg, nullptr); > + PRErrorCode error = PR_GetError(); > + > + PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: Did legacy sslservercheck in 'classic' cert=%p pinarg=%p verifylog=%p rv=%d\n",cert, pinArg, verifyLog, rv)); I'm not sure you need this log line. @@ +116,5 @@ > + PRErrorCode error = PR_GetError(); > + > + PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: Did legacy sslservercheck in 'classic' cert=%p pinarg=%p verifylog=%p rv=%d\n",cert, pinArg, verifyLog, rv)); > + if (verifyLog && rv != SECSuccess) { > + no need for this blank line @@ +118,5 @@ > + PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: Did legacy sslservercheck in 'classic' cert=%p pinarg=%p verifylog=%p rv=%d\n",cert, pinArg, verifyLog, rv)); > + if (verifyLog && rv != SECSuccess) { > + > + CERT_VerifyCert(CERT_GetDefaultCertDB(), cert, true, > + certUsageSSLServer, time, pinArg, verifyLog); indentation @@ +124,5 @@ > + MOZ_ASSERT(verifyLog->head); > + PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: log is not empty cert=%p", cert)); > + > + // Just in case, restore the error to the first error reported. > + PR_SetError(error,0); error, 0

Comment on attachment 802592 [details] [diff] [review] thin js call for verifycert (v2) Review of attachment 802592 [details] [diff] [review]: ----------------------------------------------------------------- Needs a bit of cleanup. ::: security/manager/ssl/public/nsIX509CertDB.idl @@ +303,5 @@ > + /** > + * Obtain the verification chain for a cert given a particular usage. > + * On success the call returns the verified chain and whether the cert > + * is considered for EV. > + * On failure a null array is returned (REVIEWERS: array of size 0 better?) Is it even possible to return a null array? Anyway, an array of size 0 sounds good. @@ +307,5 @@ > + * On failure a null array is returned (REVIEWERS: array of size 0 better?) > + * and an array with the errors found as NSS error codes. > + * > + * @param cert Obtain the stored trust of this certificate > + * @param usage a integer representing the usage from NSS Are these usages defined in an idl somewhere? It would be nice to either add them or have a comment pointing to them. @@ +320,5 @@ > + in bool localOnly, > + out bool hasEVPolicy, > + out ACString errorList > + //out unsigned long errCount, > + //[array, size_is(errCount)] out long errorLog So, what's up with these being commented out? ::: security/manager/ssl/src/nsNSSCertificateDB.cpp @@ +1730,5 @@ > + > + > +NS_IMETHODIMP > +nsNSSCertificateDB::GetChainForUsage(nsIX509Cert *cert, > + uint32_t usage, indentation for all of these @@ +1733,5 @@ > +nsNSSCertificateDB::GetChainForUsage(nsIX509Cert *cert, > + uint32_t usage, > + bool localOnly, > + bool *_hasEVPolicy, > + nsACString & _errorList, trailing space here and elsewhere @@ +1735,5 @@ > + bool localOnly, > + bool *_hasEVPolicy, > + nsACString & _errorList, > + nsIArray **_rvChain > + ){ ) on the same line as the last argument, { on the beginning of the next line @@ +1743,5 @@ > + NS_ENSURE_ARG_POINTER(_hasEVPolicy); > + NS_ENSURE_ARG_POINTER(_rvChain); > + *_rvChain = nullptr; > + > + /*get preconditions*/ space after /* and before */ @@ +1745,5 @@ > + *_rvChain = nullptr; > + > + /*get preconditions*/ > + nsNSSShutDownPreventionLock locker; > + if (isAlreadyShutDown()) {} around conditional body here and elsewhere @@ +1755,5 @@ > + ScopedCERTCertificate nsscert(pipCert->GetCert()); > + > + > + RefPtr<CertVerifier> certVerifier(GetDefaultCertVerifier()); > + NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED); NS_ERROR_FAILURE, maybe? @@ +1758,5 @@ > + RefPtr<CertVerifier> certVerifier(GetDefaultCertVerifier()); > + NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED); > + > + > + PLArenaPool *log_arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE); let's be consistent with naming style - logArena, etc. @@ +1765,5 @@ > + NS_ERROR("PORT_NewArena failed"); > + return NS_ERROR_OUT_OF_MEMORY; > + } > + > + CERTVerifyLog * verify_log = PORT_ArenaZNew(log_arena, CERTVerifyLog); no space after * here @@ +1771,5 @@ > + NS_ERROR("PORT_ArenaZNew failed"); > + return NS_ERROR_OUT_OF_MEMORY; > + } > + CERTVerifyLogContentsCleaner verify_log_cleaner(verify_log); > + verify_log->arena = log_arena; Hmmm - does the cleaner for verify_log not also take care of log_arena? @@ +1779,5 @@ > + SECStatus srv; > + nsresult rv; > + CERTCertList *pkixNssChain = nullptr; > + > + /* We dont fill this in one pass as there have been But if we're doing this, then why have the other patch? @@ +1794,5 @@ > + PRErrorCode error = PR_GetError(); > + > + > + if (IS_SEC_ERROR(srv) || !pkixNssChain) { > + //XX or error ? @@ +1813,5 @@ > + > + MOZ_ASSERT(verify_log->head); > + nsAutoCString errorList; > + > + CERTVerifyLogNode *i_node; maybe just "node" @@ +1815,5 @@ > + nsAutoCString errorList; > + > + CERTVerifyLogNode *i_node; > + int i; > + for (i_node = verify_log->head,i=0; i_node; i_node = i_node->next,i++) { spaces after commas @@ +1817,5 @@ > + CERTVerifyLogNode *i_node; > + int i; > + for (i_node = verify_log->head,i=0; i_node; i_node = i_node->next,i++) { > + //char string_err[20]; > + if ( i!=0 ) { (i != 0) @@ +1849,5 @@ > + nsCOMPtr<nsIX509Cert> cert = nsNSSCertificate::Create(node->cert); > + array->AppendElement(cert, false); > + } > + *_rvChain = array; > + NS_IF_ADDREF(*_rvChain); We can probably just addref directly, since we're guaranteed *_rvChain points to something.

Comment on attachment 802593 [details] [diff] [review] test-thin-certverify Review of attachment 802593 [details] [diff] [review]: ----------------------------------------------------------------- Looks like a good approach. My main concerns are typos and inconsistent style (e.g. nssErrorsService vs nss_sec_error_base). ::: security/manager/ssl/tests/unit/test_getchainforusage.js @@ +27,5 @@ > +var ee_usage1 = 'Client,Server,Sign,Encrypt'; > + > + > +let errname2value = { > + 'bad_signature' :nss_sec_error_base + 10, Seems like this slightly overlaps in functionality with getXPCOMStatusFromNSS - maybe we should unify the two somehow? @@ +147,5 @@ > + > + certdb.getChainForUsage(cert, usage, 1, hasEVPolicy, errorList); > + //do_print("err_string="+errorList.value); > + let errorArray = (errorList.value.split(",")).map(convert); //map(parseInt); > + if (cert_name.indexOf('expired') == -1) { Doesn't this mean you're only checking certs with "expired" in the name?

Thanks for feddback/reviews. (In reply to David Keeler (:keeler) from comment #10) > Comment on attachment 802593 [details] [diff] [review] > test-thin-certverify > > Review of attachment 802593 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks like a good approach. My main concerns are typos and inconsistent > style (e.g. nssErrorsService vs nss_sec_error_base). > > ::: security/manager/ssl/tests/unit/test_getchainforusage.js > @@ +27,5 @@ > > +var ee_usage1 = 'Client,Server,Sign,Encrypt'; > > + > > + > > +let errname2value = { > > + 'bad_signature' :nss_sec_error_base + 10, > > Seems like this slightly overlaps in functionality with > getXPCOMStatusFromNSS - maybe we should unify the two somehow? That sounds promising, the goal of that map is to have human readable names but I agree we a common way to do it could be better. > > @@ +147,5 @@ > > + > > + certdb.getChainForUsage(cert, usage, 1, hasEVPolicy, errorList); > > + //do_print("err_string="+errorList.value); > > + let errorArray = (errorList.value.split(",")).map(convert); //map(parseInt); > > + if (cert_name.indexOf('expired') == -1) { > > Doesn't this mean you're only checking certs with "expired" in the name? actually the opposite, as time is checked first and returns early the verifier never performs any extra checks. so I skip the expired one first before I load the cas. All the remaning certs should return 'unkown issuer'.

(In reply to David Keeler (:keeler) from comment #8) > Comment on attachment 802469 [details] [diff] [review] > psm-workaround-for-verifycert (v2) > > Review of attachment 802469 [details] [diff] [review]: > ----------------------------------------------------------------- > > r- for a bit of cleanup and because I think the comment could be more clear. > > ::: security/manager/ssl/src/CertVerifier.cpp > @@ +101,5 @@ > > * there being any differnce in results between CERT_VerifyCert and > > * CERT_VerifyCertificate. > > */ > > + > > +#ifdef BUG_910438_FIXED > > Instead of using #ifdefs, why don't we just file a bug to revert this change > when bug 910438 is fixed and mozilla-central is using the version of NSS it > shipped with? I prefer to keep these bugs of dependencies inside the code as it makes it clear why you have a really obtuse solution to a problem instead of the obvious one. And once the bug is fixed, it is trivial for anyone to remove the workaround. Other than that, agreed with all > > @@ +106,4 @@ > > rv = CERT_VerifyCert(CERT_GetDefaultCertDB(), cert, true, > > certUsageSSLServer, time, pinArg, verifyLog); > > +#else > > + /* BUG_910438: CERT_VerifyCert returns differently on error than on success > > This comment is a bit unclear. Bug 910438 comment 6 seemed clear to me, so > maybe adapt that for use here. > > @@ +114,5 @@ > > + rv = CERT_VerifyCert(CERT_GetDefaultCertDB(), cert, true, > > + certUsageSSLServer, time, pinArg, nullptr); > > + PRErrorCode error = PR_GetError(); > > + > > + PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: Did legacy sslservercheck in 'classic' cert=%p pinarg=%p verifylog=%p rv=%d\n",cert, pinArg, verifyLog, rv)); > > I'm not sure you need this log line. > > @@ +116,5 @@ > > + PRErrorCode error = PR_GetError(); > > + > > + PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: Did legacy sslservercheck in 'classic' cert=%p pinarg=%p verifylog=%p rv=%d\n",cert, pinArg, verifyLog, rv)); > > + if (verifyLog && rv != SECSuccess) { > > + > > no need for this blank line > > @@ +118,5 @@ > > + PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: Did legacy sslservercheck in 'classic' cert=%p pinarg=%p verifylog=%p rv=%d\n",cert, pinArg, verifyLog, rv)); > > + if (verifyLog && rv != SECSuccess) { > > + > > + CERT_VerifyCert(CERT_GetDefaultCertDB(), cert, true, > > + certUsageSSLServer, time, pinArg, verifyLog); > > indentation > > @@ +124,5 @@ > > + MOZ_ASSERT(verifyLog->head); > > + PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: log is not empty cert=%p", cert)); > > + > > + // Just in case, restore the error to the first error reported. > > + PR_SetError(error,0); > > error, 0

Comment on attachment 823625 [details] [diff] [review] thin js call for verifycert (v3) Review of attachment 823625 [details] [diff] [review]: ----------------------------------------------------------------- It seems like a lot of this is copied from nsNSSCertificate.cpp - can we refactor the code at all? (Or is this going to end up in the standalone verifier and can't be shared?) ::: security/manager/ssl/public/nsIX509CertDB.idl @@ +316,5 @@ > + * @return A chain of certificates if the cert was valid for that usage > + */ > + nsIArray getChainForUsage(in nsIX509Cert cert, > + in uint32_t usage, > + in bool localOnly, aCert, aUsage, aLocalOnly, etc. @@ +319,5 @@ > + in uint32_t usage, > + in bool localOnly, > + out bool hasEVPolicy, > + out ACString errorList > + ); Nit: I think it's more common for the closing parenthesis to go on the same line as the last argument. ::: security/manager/ssl/src/nsNSSCertificateDB.cpp @@ +57,5 @@ > #endif > > static NS_DEFINE_CID(kNSSComponentCID, NS_NSSCOMPONENT_CID); > > +NSSCleanupAutoPtrClass_WithParam(PLArenaPool, PORT_FreeArena, FalseParam, false) Does it make sense to define a ScopedPLArenaPool in ScopedNSSTypes.h instead? @@ +1671,5 @@ > + > +NS_IMETHODIMP > +nsNSSCertificateDB::GetChainForUsage(nsIX509Cert *cert, > + uint32_t usage, > + bool localOnly, aCert, aUsage, aLocalOnly, etc. @@ +1674,5 @@ > + uint32_t usage, > + bool localOnly, > + bool *_hasEVPolicy, > + nsACString & _errorList, > + nsIArray **_rvChain Maybe aHasEVPolicyOut, aErrorListOut, etc. @@ +1675,5 @@ > + bool localOnly, > + bool *_hasEVPolicy, > + nsACString & _errorList, > + nsIArray **_rvChain > + ){ Nit: ") {" (also, move this to the same line as the last argument) @@ +1683,5 @@ > + NS_ENSURE_ARG_POINTER(_hasEVPolicy); > + NS_ENSURE_ARG_POINTER(_rvChain); > + *_rvChain = nullptr; > + > + /*get preconditions*/ This comment is unclear to me. @@ +1719,5 @@ > + if (!verifyLog) { > + NS_ERROR("PORT_ArenaZNew failed"); > + return NS_ERROR_OUT_OF_MEMORY; > + } > + CERTVerifyLogContentsCleaner verify_log_cleaner(verifyLog); Again, can we do this with a single scoped type instead? @@ +1734,5 @@ > + certverify, so we will call it once on success and twice for error > + */ > + srv = certVerifier->VerifyCert(nsscert, > + usage, PR_Now(), > + nullptr, //XXX fixme What's the issue that needs to be fixed here? @@ +1742,5 @@ > + nullptr); > + PRErrorCode error = PR_GetError(); > + > + > + if (srv!=SECSuccess || !pkixNssChain) { spaces: "srv != SECSuccess" @@ +1744,5 @@ > + > + > + if (srv!=SECSuccess || !pkixNssChain) { > + //XX or error > + PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("Getting usage chain for is fail \"%s\"\n", nsscert->nickname)); Maybe "Failed to get usage chain for \"%s\"\n" @@ +1762,5 @@ > + nsAutoCString errorList; > + > + CERTVerifyLogNode *node; > + int i; > + for (node = verifyLog->head,i=0; node; node = node->next,i++) { spaces @@ +1763,5 @@ > + > + CERTVerifyLogNode *node; > + int i; > + for (node = verifyLog->head,i=0; node; node = node->next,i++) { > + if ( i!=0 ) { spaces @@ +1771,5 @@ > + if(node->error == error) { > + error_found = true; > + } > + } > + _errorList=errorList; spaces @@ +1786,5 @@ > + > + ScopedCERTCertList nssChain(pkixNssChain); > + > + nsCOMPtr<nsIMutableArray> array = > + do_CreateInstance(NS_ARRAY_CONTRACTID, &rv); indent

Comment on attachment 823625 [details] [diff] [review] thin js call for verifycert (v3) Review of attachment 823625 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/nsNSSCertificateDB.cpp @@ +57,5 @@ > #endif > > static NS_DEFINE_CID(kNSSComponentCID, NS_NSSCOMPONENT_CID); > > +NSSCleanupAutoPtrClass_WithParam(PLArenaPool, PORT_FreeArena, FalseParam, false) maybe but I would add it after this bug is landed. (once there is a gain to have), @@ +1734,5 @@ > + certverify, so we will call it once on success and twice for error > + */ > + srv = certVerifier->VerifyCert(nsscert, > + usage, PR_Now(), > + nullptr, //XXX fixme It is related to bug BUG_910438 he is afraid there are other cases where CERT_VerifyCert returns bad results. We discussed this and he wants this dual call here even with the workaround in CertCerifier.cpp.

Comment on attachment 824869 [details] [diff] [review] thin js call for verifycert (v4) Review of attachment 824869 [details] [diff] [review]: ----------------------------------------------------------------- As discussed on IRC, let's refactor the code this introduces that is similar to what's in nsNSSCertificate.cpp. ::: security/manager/ssl/public/nsIX509CertDB.idl @@ +311,5 @@ > + * @param usage a integer representing the usage from NSS > + * @param localOnly prevent network activity for revocation > + * @param hasEVPolicy bool that signified that the cert was an EV cert > + * @param errCount the number of errors found > + * @param errorLog and array of errors represented by their nss error value. The comments now don't match the argument names in the function declaration. @@ +318,5 @@ > + nsIArray getChainForUsage(in nsIX509Cert aCert, > + in uint32_t aUsage, > + in bool aLocalOnly, > + out bool aOutHasEVPolicy, > + out ACString aOutErrorList); Nit: since these arguments are already annotated with "out", I don't think their names need to include "Out" in the idl file.

Comment on attachment 825367 [details] [diff] [review] thin js call for verifycert (v5) Review of attachment 825367 [details] [diff] [review]: ----------------------------------------------------------------- In general, it would be great if you made sure patches are ready to land stylistically before asking for review (the main things are nits like naming conventions, indentation, and spaces around &*()+=, etc.) Anyway, this is almost ready to land - just a few fixups. ::: security/manager/ssl/src/nsNSSCertificate.h @@ +84,5 @@ > > nsNSSCertList(CERTCertList *certList = nullptr, bool adopt = false); > > static CERTCertList *DupCertList(CERTCertList *aCertList); > + static nsresult CopyCertListTonsIArray(CERTCertList *aCertList, The capitalization here is a bit unfortunate. Maybe CopyCertListToNSIArray? Eh - that's not much better. Maybe ask on #developers or a mailing list? @@ +85,5 @@ > nsNSSCertList(CERTCertList *certList = nullptr, bool adopt = false); > > static CERTCertList *DupCertList(CERTCertList *aCertList); > + static nsresult CopyCertListTonsIArray(CERTCertList *aCertList, > + nsIArray **_rvChain); indentation ::: security/manager/ssl/src/nsNSSCertificateDB.cpp @@ +1673,5 @@ > +nsNSSCertificateDB::GetChainForUsage(nsIX509Cert *aCert, > + uint32_t aUsage, > + bool aLocalOnly, > + bool *aHasEVPolicy, > + nsACString & aErrorList, &aErrorList @@ +1711,5 @@ > + NS_ERROR("PORT_NewArena failed"); > + return NS_ERROR_OUT_OF_MEMORY; > + } > + > + CERTVerifyLog * verifyLog = PORT_ArenaZNew(logArena, CERTVerifyLog); *verifyLog @@ +1720,5 @@ > + CERTVerifyLogContentsCleaner verify_log_cleaner(verifyLog); > + verifyLog->arena = logArena; > + > + SECOidTag evOidPolicy; > + *aHasEVPolicy= false; aHasEVPolicy = @@ +1727,5 @@ > + CERTCertList *pkixNssChain = nullptr; > + CertVerifier::Flags flags = aLocalOnly ? CertVerifier::FLAG_LOCAL_ONLY : 0; > + > + /* Bsmith wants the call to have the same parameters usually used with > + certverify, so we will call it once on success and twice for error I don't think this comment explains well enough what the issue here is. In particular, it should be descriptive enough so we don't have to have the vague "// fixme" comments below. Also, use // style comments. @@ +1740,5 @@ > + PRErrorCode error = PR_GetError(); > + > + if (srv != SECSuccess) { > + PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, > + ("Failed to get usage chain for is fail \"%s\"\n", Again, this isn't a very clear log comment. Maybe "Failed to get usage chain for \"%s\"\n" would be better. @@ +1756,5 @@ > + > + bool error_found = false; > + > + MOZ_ASSERT(verifyLog->head); > + nsAutoCString errorList; Why do we have to use a local variable and then copy it to the out variable? Can't we just use aErrorList directly? (After maybe truncating it first) @@ +1780,5 @@ > + MOZ_ASSERT(pkixNssChain); > + if (evOidPolicy != SEC_OID_UNKNOWN) { > + *aHasEVPolicy = true; > + } > + ScopedCERTCertList nssChain(pkixNssChain); Nit: I would have a blank line before this one and no blank line after it.

Comment on attachment 802593 [details] [diff] [review] test-thin-certverify Review of attachment 802593 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_getchainforusage.js @@ +1,4 @@ > +"use strict"; > +/* > + * The purpose of this test is to verify that we correctly detect bad signatures on tampered certificates. > + * Evenstually, we should also be verifying that the error we return is the correct error. It looks like this is an attempt to rewrite test_cert_signatures.js to use the CertVerifier::VerifyCert wrapper that you created. I think that is fine and well. But, this should be a patch to change how test_cert_signatures.js works, not a new test. @@ +23,5 @@ > + > + > +var ca_usage1 = 'SSL CA'; > +var ca_usage2 = 'Client,Server,Sign,Encrypt,SSL CA,Status Responder'; > +var ee_usage1 = 'Client,Server,Sign,Encrypt'; dead code? @@ +32,5 @@ > + 'expired' :nss_sec_error_base + 11, > + 'revoked_certificate' :nss_sec_error_base + 12, > + 'unknown_issuer' :nss_sec_error_base + 13, > + 'usages_invalid' :nss_sec_error_base + 38, > + 'inadequate_key_usage':nss_sec_error_base + 90, See bug 934327. @@ +40,5 @@ > + 'certificateUsageSSLClient' : 0x01, > + 'certificateUsageSSLServer' : 0x02, > + 'certificateUsageSSLCA' : 0x08, > + 'certificateUsageEmailSigner' : 0x10 > +}; 1. Why not this?: const certificateUsageSSLClient = 0x01; ... 2. Put this is in head_psm.js so we can reuse it in other tests. @@ +80,5 @@ > + 'p384-invalid-int-usages': [], > + 'ee-p384-invalid-int-usages' : ['inadequate_key_usage'], > + > + 'rsa-invalid-int-usages-perturbed-ee' : [], > + 'ee-rsa-invalid-int-usages-perturbed-ee' : ['inadequate_key_usage','bad_signature'], I do not understand why it is good to do this with a dictionary. AFAICT, based on my experience with using other similarly-written tests to debug problems in insanity::pkix, is that these these dictionary-based tests are extremely difficult to debug. Conversely, tests that are organized like test_ocsp_stapling.js (a helper function that does all the repeated setup, and then one call to that function for each test case with the differing inputs as parameters) is very easy to debug. @@ +187,5 @@ > + } > + } > + > + > + if (gDebug.isDebugBuild) { I think that this EV testing should go into the patch for bug 927016, not here.

Comment on attachment 825367 [details] [diff] [review] thin js call for verifycert (v5) Review of attachment 825367 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/public/nsIX509CertDB.idl @@ +298,5 @@ > * @param aName name of the cert for display purposes. > */ > void addCert(in ACString certDER, in string aTrust, in string aName); > + > + Update the UUID of the interface. @@ +317,5 @@ > + nsIArray getChainForUsage(in nsIX509Cert aCert, > + in uint32_t aUsage, > + in bool aLocalOnly, > + out bool aHasEVPolicy, > + out ACString aErrorList); I am a little surprised that this function isn't named VerifyCert or similar. Isn't this wrapper supposed to be basically a proxy from JS to C++ to enable us to call CertVerifer::VerifyCert from JavaScript? If so, I would expect the signature to look something like this: typedef int32_t PRErrorCode; // NOT uint32_t!!! See certt.h typedef int64_t SECCertificateUsage; const SECCertificateUsage certificateUsageSSLClient = 0x01; const SECCertificateUsage certificateUsageSSLServer = 0x02; ... typedef unsigned long Flags; const Flags FLAG_LOCAL_ONLY = 1; // the time parameter is implicitly PR_Now() nsIArray verifyCertNow(in nsIX509Cert aCert, in SECCertificateUsage aUsage, optional in Flags flags, optional out bool aHasEVPolicy); I don't see why we need aErrorList. The only time we call CertVerifier::VerifyCert in real code with a verify log is when we're doing cert error overrides. For all other cases, all we really care about is the return value of CertVerifier::VerifyCert, not any error log. In the case of cert error overrides, the important thing is that we get the decision to override or not override the error correct. The exact contents of the error log do not matter much. So, as I said before, I suggest we punt on that aspect. Similarly, I don't see why we go through the effort of creating the nsIArray of certificates, considering we don't have any tests that need that array. I suggest that we drop that for now and add it back once we actually have a test that needs to test the constructed cert chain. That will simplify this code so it can land sooner. Then we can make the return value be a PRErrorCode, and we can write tests like: do_check_eq(SEC_ERROR_EXPIRED_CERTIFICATE, certDB.verifyCertNow(cert, certDB.certificateUsageSSLServer)); ... which is very easy to write, read, and understand. In general, YAGNI.

Brian I think I will going to need the chain, particularly when we start adding tests for cross-signing side effects (think pinning). What about this one?

Comment on attachment 828747 [details] [diff] [review] thin js call for verifycert (v6) Review of attachment 828747 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty reasonable to me. ::: security/manager/ssl/public/nsIX509CertDB.idl @@ +303,5 @@ > + /* > + * Obtain the verificatin result for a cert given a particular usage. > + * On success the call returns 0, a chain of the chain used to verify > + * and whether the cert is good for EV usage. > + * On failure the call returns the error code for the verication failure Which kind of error code? a PRErrorCode? If so, the return type should be SECStatus. I suggest adding a note that this function should NOT be used by extensions AND should NOT be used internally within Gecko. It is for tests only, because it does I/O on the main thread. @@ +317,5 @@ > + int64_t verifyCertNow(in nsIX509Cert aCert, > + in uint64_t aUsage, > + in bool aLocalOnly, > + out nsIX509CertList verifiedChain, > + out bool aHasEVPolicy); Please use typedefs and provide named constants like I suggested in my previous review. XPIDL can do typedefs and named constants. Let me know if you have trouble getting them to work. ::: security/manager/ssl/src/nsNSSCertificateDB.cpp @@ +1671,5 @@ > + uint64_t aUsage, > + bool aLocalOnly, > + nsIX509CertList **verifiedChain, > + bool *aHasEVPolicy, > + int64_t *_retval) Please follow the Mozilla coding convention in new code: "nsIX509Cert* aCert" not "nsIX509Cert *aCert" @@ +1673,5 @@ > + nsIX509CertList **verifiedChain, > + bool *aHasEVPolicy, > + int64_t *_retval) > +{ > + NS_ENSURE_ARG(aCert); NS_ENSURE_ARG_POINTER @@ +1676,5 @@ > +{ > + NS_ENSURE_ARG(aCert); > + NS_ENSURE_ARG_POINTER(aHasEVPolicy); > + NS_ENSURE_ARG_POINTER(verifiedChain); > + NS_ENSURE_ARG(_retval); NS_ENSURE_ARG_POINTER @@ +1677,5 @@ > + NS_ENSURE_ARG(aCert); > + NS_ENSURE_ARG_POINTER(aHasEVPolicy); > + NS_ENSURE_ARG_POINTER(verifiedChain); > + NS_ENSURE_ARG(_retval); > + I suggest you NULL out the out parameters (verifiedChain, aHasEVPolicy, _retval) here. @@ +1681,5 @@ > + > + nsNSSShutDownPreventionLock locker; > + if (isAlreadyShutDown()) { > + return NS_ERROR_NOT_AVAILABLE; > + } nit: new line here. @@ +1689,5 @@ > + nsCOMPtr<nsINSSComponent> inss = do_GetService(PSM_COMPONENT_CONTRACTID, &rv); > + if (NS_FAILED(rv)) { > + return NS_ERROR_NOT_AVAILABLE; > + } > + inss->EnsureIdentityInfoLoaded(); Why do we need to call EnsureIdentityInfoLoaded here? Isn't the existing call to EnsureIdentityInfoLoaded in nsNSSCertificate::hasValidEVOidTag sufficient? @@ +1692,5 @@ > + } > + inss->EnsureIdentityInfoLoaded(); > +#endif > + > + nsCOMPtr<nsIX509Cert2> pipCert = do_QueryInterface(aCert); nit: please don't use the name "pip" in any new code. It doesn't mean anything to anybody any more. @@ +1696,5 @@ > + nsCOMPtr<nsIX509Cert2> pipCert = do_QueryInterface(aCert); > + if (!pipCert) { > + return NS_ERROR_FAILURE; > + } > + ScopedCERTCertificate nsscert(pipCert->GetCert()); nit: nssCert. @@ +1702,5 @@ > + RefPtr<CertVerifier> certVerifier(GetDefaultCertVerifier()); > + NS_ENSURE_TRUE(certVerifier, NS_ERROR_FAILURE); > + > + SECOidTag evOidPolicy; > + *aHasEVPolicy= false; NIT: whitespace. @@ +1704,5 @@ > + > + SECOidTag evOidPolicy; > + *aHasEVPolicy= false; > + > + SECStatus srv; NIT: declare variables as close to where they are initialized as possible. @@ +1705,5 @@ > + SECOidTag evOidPolicy; > + *aHasEVPolicy= false; > + > + SECStatus srv; > + CERTCertList *pkixNssChain = nullptr; NIT: I don't understand the name "pkixNssChain". What is the "pkix" for? @@ +1710,5 @@ > + CertVerifier::Flags flags = aLocalOnly ? CertVerifier::FLAG_LOCAL_ONLY : 0; > + > + srv = certVerifier->VerifyCert(nsscert, > + aUsage, PR_Now(), > + nullptr, //Assume no context nit: whitespace after "//" @@ +1719,5 @@ > + > + PRErrorCode error = PR_GetError(); > + > + nsCOMPtr<nsIX509CertList> nssCertList; > + //This adopts the list nit: space after "//". @@ +1722,5 @@ > + nsCOMPtr<nsIX509CertList> nssCertList; > + //This adopts the list > + nssCertList = new nsNSSCertList(pkixNssChain, true); > + *verifiedChain = nssCertList; > + NS_ADDREF(*verifiedChain); Why not just: NS_ADDREF(*verifiedChain = new nsNSSCertList(pkixNSSChain, true)); nit: trailing whitespace. @@ +1725,5 @@ > + *verifiedChain = nssCertList; > + NS_ADDREF(*verifiedChain); > + > + if (evOidPolicy != SEC_OID_UNKNOWN) { > + *aHasEVPolicy = true; We should only set aHasEVPolicy = true if srv == SECSuccess, right? @@ +1733,5 @@ > + *_retval = 0; > + } else { > + *_retval = error; > + } > + whitespace.

Comment on attachment 829468 [details] [diff] [review] test-thin-certverify-3 Review of attachment 829468 [details] [diff] [review]: ----------------------------------------------------------------- I still don't think we need to have a specific test for this. Instead, let's just write the tests for which we created this function in the first place. And/or, modify one of the existing tests to use this function instead of using the horrible getUsagesArray stuff. ::: security/manager/ssl/tests/unit/test_verifycertnow.js @@ +1,4 @@ > +"use strict"; > +/* > + * The purpose of this test is to verify that we correctly detect bad signatures on tampered certificates. > + * Evenstually, we should also be verifying that the error we return is the correct error. Wrap lines to 80 chars.

Comment on attachment 828747 [details] [diff] [review] thin js call for verifycert (v6) Review of attachment 828747 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/public/nsIX509CertDB.idl @@ +317,5 @@ > + int64_t verifyCertNow(in nsIX509Cert aCert, > + in uint64_t aUsage, > + in bool aLocalOnly, > + out nsIX509CertList verifiedChain, > + out bool aHasEVPolicy); Regarding the consts. I cannot have uint64_t consts and there are collision in the names: CertificateUsageSSLServer is OK sertificateUsageSSLServer is NOT OK. Add them as long consts? (your call). ::: security/manager/ssl/src/nsNSSCertificateDB.cpp @@ +1689,5 @@ > + nsCOMPtr<nsINSSComponent> inss = do_GetService(PSM_COMPONENT_CONTRACTID, &rv); > + if (NS_FAILED(rv)) { > + return NS_ERROR_NOT_AVAILABLE; > + } > + inss->EnsureIdentityInfoLoaded(); no, cerverufy calls getFirstEVPolicy which does not initialize the ev certs. @@ +1722,5 @@ > + nsCOMPtr<nsIX509CertList> nssCertList; > + //This adopts the list > + nssCertList = new nsNSSCertList(pkixNssChain, true); > + *verifiedChain = nssCertList; > + NS_ADDREF(*verifiedChain); 'cannot convert ‘nsNSSCertList’ to ‘nsIX509CertList*’ in assignment' ( and two lines before: nsNSSCertificate.h:92:12: error: ‘virtual nsNSSCertList::~nsNSSCertList()’ is private) @@ +1725,5 @@ > + *verifiedChain = nssCertList; > + NS_ADDREF(*verifiedChain); > + > + if (evOidPolicy != SEC_OID_UNKNOWN) { > + *aHasEVPolicy = true; OK. moved inside the next if block and added assertion on the block.

new version: diff on comments: 1. consts now in head_psm.js as discussed via irc. 2. NS_ADDREF(*verifiedChain = new nsNSSCertList(pkixNSSChain, true) not done due to compilation errors. 3. EnsureIdentityInfoLoaded is still called as verifycert calls do not call it.

Comment on attachment 831141 [details] [diff] [review] thin js call for verifycert (v7.1) Review of attachment 831141 [details] [diff] [review]: ----------------------------------------------------------------- Please make sure you address all the review comments below. ::: security/manager/ssl/public/nsIX509CertDB.idl @@ +304,5 @@ > + * 1. It can create IO on the main thread. > + * 2. It is in constant change, so in/out can change at any release. > + * > + * Obtain the verificatin result for a cert given a particular usage. > + * On success the call returns 0, a chain of the chain used to verify s/ a chain of the chain used to verify/the chain built during verification/ @@ +306,5 @@ > + * > + * Obtain the verificatin result for a cert given a particular usage. > + * On success the call returns 0, a chain of the chain used to verify > + * and whether the cert is good for EV usage. > + * On failure the call returns the PRerror code for the verication failure s/PRerror code/PRErrorCode/ s/verication/verification/ @@ +317,5 @@ > + * @return 0 if success or the value or the error code for the verification > + * failure > + */ > + int32_t verifyCertNow(in nsIX509Cert aCert, > + in uint64_t aUsage, SECCertificateUsage is int64_t, not uint64_t. I suggest we document the types as follows: int32_t /*PRErrorCode*/ ... in uint64_t /*SECCertificateUsage*/ aUsage, ::: security/manager/ssl/src/nsNSSCertificateDB.cpp @@ +1674,5 @@ > + uint64_t aUsage, > + bool aLocalOnly, > + nsIX509CertList** verifiedChain, > + bool* aHasEVPolicy, > + int32_t* _retval) Document the real types here too. @@ +1678,5 @@ > + int32_t* _retval) > +{ > + NS_ENSURE_ARG_POINTER(aCert); > + NS_ENSURE_ARG_POINTER(aHasEVPolicy); > + NS_ENSURE_ARG_POINTER(verifiedChain); Should we have NS_ENSURE_TRUE(!*verifiedChain)? @@ +1701,5 @@ > +#endif > + > + nsCOMPtr<nsIX509Cert2> x509Cert = do_QueryInterface(aCert); > + if (!x509Cert) { > + return NS_ERROR_FAILURE; NS_ERROR_INVALID_ARG @@ +1728,5 @@ > + // This adopts the list > + nssCertList = new nsNSSCertList(locker, resultChain, true); > + NS_ENSURE_TRUE(nssCertList, NS_ERROR_FAILURE); > + *verifiedChain = nssCertList; > + NS_ADDREF(*verifiedChain); Please use nssCertList.forget(verifiedChain); In general, try to avoid NS_ADDREF and other manual memory management. Maybe it is better to put this nssCertList.forget(verifiedChain) just before the "return NS_OK." Then we don't have to think about what happens when the NS_ENSURE_TRUE below returns early. @@ +1736,5 @@ > + *aHasEVPolicy = true; > + } > + *_retval = 0; > + } else { > + NS_ENSURE_TRUE(evOidPolicy == SEC_OID_UNKNOWN, NS_ERROR_FAILURE); Add NS_ENSURE_TRUE(error != 0, NS_ERROR_FAILURE);

clearing needinfo since I did the review.

Keeping r+ from bsmith