Closed Bug 813418 Opened 7 years ago Closed 7 years ago

Centralize certificate validation

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: briansmith, Assigned: cviecco)

References

Details

Attachments

(3 files, 55 obsolete files)

78.74 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
87.46 KB, patch
Details | Diff | Splinter Review
1.44 KB, patch
cviecco
: review+
Details | Diff | Splinter Review
Now, there are many places where we call CERT_VerifyCert and CERT_PKIXVerifyCert. When we want to modify the behavior of all certificate validations, we have to modify many callers. And, this is difficult to test. Instead, we should create a wrapper function around CERT_VerifyCert/CERT_PKIXVerifyCert that every certificate validation will call.
Blocks: 672811
No longer depends on: 672811
Attachment #690966 - Flags: feedback?(cviecco)
Attached patch Use CertVerifier for EV (obsolete) — Splinter Review
Attachment #690969 - Flags: feedback?(cviecco)
Comment on attachment 690966 [details] [diff] [review]
Centralize cert verification [WIP]

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

Yay I found this excellent. We might want to add other optional output params to have is really be the only place for validation. I have a patch queue based on this one to address all of this.
Comment on attachment 690966 [details] [diff] [review]
Centralize cert verification [WIP]

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

Yay I found this excellent. We might want to add other optional output params to have is really be the only place for validation. I have a patch queue based on this one to address all of this.
Attachment #690966 - Flags: feedback?(cviecco) → feedback+
Comment on attachment 690969 [details] [diff] [review]
Use CertVerifier for EV

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

I think it is better if the callers are oblivious of ev validation and the certverify actually returns the validated oid policy if such thing was found. The interface requiring input of EV params goes agains of what we are trying to do (remove as much input flexibility) from the validation callers as possible.
Attachment #690969 - Flags: feedback?(cviecco) → feedback-
So here is a try log with centralized verification, including ev verification (and single verification) without enabling pkix.

https://tbpl.mozilla.org/?tree=Try&rev=5a3f10ea3c21

The pkix enabled push log will be recorded later.
Blocks: 744204
Attached patch centralized verify (p1) (obsolete) — Splinter Review
From bsmith,
Comment on attachment 701998 [details] [diff] [review]
centralized verify (p1)

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

So I would say this seems OK. Just a couple of traling spaces.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +659,5 @@
>  {
> +    RefPtr<CertVerifier> certVerifier(GetDefaultCertVerifier());
> +    if (!certVerifier) {
> +      PR_SetError(PR_INVALID_STATE_ERROR, 0);
> +      return SECFailure; 

trailing space.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1243,5 @@
>        break;
>  
>      case CERT_USAGE_VerifyCA:
>        nss_usage = certificateUsageVerifyCA;
> +      break;  

traling space
Attached patch part 2: centralize EV (obsolete) — Splinter Review
Attachment #690969 - Attachment is obsolete: true
Attached patch part4: use verifychain (obsolete) — Splinter Review
Attached patch part 5: getting verify-log (obsolete) — Splinter Review
Attached file part 6: removing verifycertnow (obsolete) —
Attached patch part 6: removing verifycertnow (obsolete) — Splinter Review
Attachment #702057 - Attachment is obsolete: true
Comment on attachment 701998 [details] [diff] [review]
centralized verify (p1)

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

So I would say this seems OK. Just a couple of traling spaces.

This patch also checks OCSP for the chain in all cases, to have the same behaviour as classic, we need to remove chain ocsp checks for NON ev certificates.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +659,5 @@
>  {
> +    RefPtr<CertVerifier> certVerifier(GetDefaultCertVerifier());
> +    if (!certVerifier) {
> +      PR_SetError(PR_INVALID_STATE_ERROR, 0);
> +      return SECFailure; 

trailing space.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1243,5 @@
>        break;
>  
>      case CERT_USAGE_VerifyCA:
>        nss_usage = certificateUsageVerifyCA;
> +      break;  

traling space
Comment on attachment 690966 [details] [diff] [review]
Centralize cert verification [WIP]

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

Good as base to build upon. Requires other patches to actually provide completeness/correctness. But is an excellent start.
Attachment #690966 - Flags: review+
Comment on attachment 690966 [details] [diff] [review]
Centralize cert verification [WIP]

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

Oops wrong review
Attachment #690966 - Flags: review+
Comment on attachment 701998 [details] [diff] [review]
centralized verify (p1)

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

Space nits and a potential better use of a scoped variable.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +1158,1 @@
>  

Maybe keep using the ScopedCertCertificate here?
Attachment #701998 - Flags: review+
Attached patch part 7: remove verifycertnow (obsolete) — Splinter Review
Attachment #702058 - Attachment is obsolete: true
Attachment #702053 - Attachment is obsolete: true
Attached file part5 :getting chain from certverify (obsolete) —
Attachment #702051 - Attachment is obsolete: true
Attachment #702050 - Attachment is obsolete: true
To have same behaviour as current code
Attachment #703976 - Attachment is obsolete: true
Attached file part2: centralize ev verfication (obsolete) —
Attachment #702049 - Attachment is obsolete: true
Attachment #704057 - Attachment is obsolete: true
Attachment #704055 - Attachment is obsolete: true
Attachment #704053 - Attachment is obsolete: true
Attachment #705037 - Attachment is obsolete: true
setup as real patch
Attachment #704052 - Attachment is obsolete: true
Attachment #704056 - Attachment is obsolete: true
Comment on attachment 701998 [details] [diff] [review]
centralized verify (p1)

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

Really, my only comments are about unnecessary whitespace and some places where you've got "/* XXX something or other */" and I wasn't sure if you were going to take care of that in this patch, another patch in this bug, or another bug entirely.

::: security/manager/ssl/src/nsCERTValInParamWrapper.cpp
@@ +33,4 @@
>  }
>  
> +SECStatus
> +CertVerifier::VerifyCert(CERTCertificate * cert, 

Trailing space.

@@ +132,5 @@
> +  cvin[2].type = cert_pi_date;
> +  cvin[2].value.scalar.time = time;
> +  cvin[3].type = cert_pi_end;
> +
> +  CERTValOutParam cvout[2];

Why [2]? Don't we just need 1?

::: security/manager/ssl/src/CertVerifier.h
@@ +24,5 @@
> +  static const Flags FLAG_LOCAL_ONLY;
> +
> +  // policy == SEC_OID_UNKNOWN means "do not verify for policy."
> +  // trustAnchors == nullptr means "determine trust based on NSS trust database."
> +  SECStatus VerifyCert(CERTCertificate * cert, 

Trailing space.

@@ +39,5 @@
> +
> +  bool IsOCSPDownloadEnabled() const { return mOCSPDownloadEnabled; }
> +
> +private:
> +  CertVerifier(missing_cert_download_config ac, crl_download_config cdc,

It looks like this first argument doesn't follow the naming conventions of the rest of the arguments.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +474,5 @@
>      NS_ERROR("GetDefaultCERTValInParam failed");
>      PR_SetError(defaultErrorCodeToReport, 0);
>      return nullptr;
>    }
>    

Some unnecessary whitespace you could remove while you're here...

@@ +484,2 @@
>  
> +  if (true) { // XXX: if not libpkix

I assume this is going to get fixed later?

@@ +495,5 @@
> +      return nullptr; // PORT_ArenaZNew set error code
> +    }
> +    CERTVerifyLogContentsCleaner verify_log_cleaner(verify_log);
> +    verify_log->arena = log_arena;
> +  

Unnecessary whitespace.

@@ +565,3 @@
>      }
> +  } else {
> +    // XXX set errorCodeTrust, errorCodeMismatch, errorCodeExpired, collected_errors

So... does this happen in a later patch?

@@ +920,5 @@
>      if (nsc) {
>        bool dummyIsEV;
>        nsc->GetIsExtendedValidation(&dummyIsEV); // the nsc object will cache the status
>      }
>      

As long as you're removing the next line, you could deal with this unnecessary whitespace.

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +503,1 @@
>   

Remove this unnecessary space while you're here?

@@ +528,5 @@
>    for (i=0; i < numcerts; i++) {
>      rawArray[i] = &certCollection->rawCerts[i];
>    }
>  
> +  srv = CERT_ImportCerts(certdb, certUsageEmailRecipient, numcerts, rawArray, 

Trailing space.

@@ +602,5 @@
>      }
>      for (i=0; i < certChain->len; i++) {
>        rawArray[i] = &certChain->certs[i];
>      }
> +    CERT_ImportCerts(certdb, certUsageEmailRecipient, certChain->len, 

Trailing space.

@@ +1292,5 @@
>  NS_IMETHODIMP
>  nsNSSCertificateDB::FindCertByEmailAddress(nsISupports *aToken, const char *aEmailAddress, nsIX509Cert **_retval)
>  {
>    nsNSSShutDownPreventionLock locker;
>    

Whitespace.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1103,1 @@
>        aiaDownloadEnabled ? 

Trailing spaces on the lines you didn't change...

@@ +1988,5 @@
>    *aErrorCode = 0;
>    *aPrincipal = nullptr;
>  
>    nsNSSShutDownPreventionLock locker;
> +  SEC_PKCS7ContentInfo * p7_info = nullptr; 

Trailing space here and the line below.
Attachment #701998 - Flags: feedback+
Comment on attachment 705032 [details] [diff] [review]
part 2: centralize EV verification

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

::: security/manager/ssl/src/CertVerifier.cpp
@@ +40,5 @@
>    MOZ_COUNT_DTOR(CertVerifier);
>  }
>  
>  SECStatus
>  CertVerifier::VerifyCert(CERTCertificate * cert, 

Remove trailing space while you're here?

@@ +164,5 @@
>          : CERT_REV_MI_NO_OVERALL_INFO_REQUIREMENT)
>      ;
>  
> +  CERTValInParam cvin[6];
> +  size_t i = 3;

Maybe move the declaration of i closer to where it's used?

@@ +173,5 @@
>    cvin[1].value.pointer.revocation = &rev;
>    cvin[2].type = cert_pi_date;
>    cvin[2].value.scalar.time = time;
> +
> +  size_t evParamLocation = i;

I see what you're doing here, but maybe just use 3 instead of i?
I would also use const (this value isn't supposed to change, right?).

@@ +204,5 @@
> +    rv = CERT_PKIXVerifyCert(cert, usage, cvin, cvout, pinArg);
> +  }
> +
> +  if (trustAnchors) {
> +    //do cleanup , need to error check too

I don't see any error checking here...?

::: security/manager/ssl/src/CertVerifier.h
@@ +24,5 @@
>    static const Flags FLAG_LOCAL_ONLY;
>  
>    // policy == SEC_OID_UNKNOWN means "do not verify for policy."
>    // trustAnchors == nullptr means "determine trust based on NSS trust database."
>    SECStatus VerifyCert(CERTCertificate * cert, 

Maybe removing all unnecessary whitespace from PSM/NSS should be a bug on its own...

::: security/manager/ssl/src/nsIdentityChecking.cpp
@@ +1213,5 @@
>    resultOidTag = SEC_OID_UNKNOWN;
>  
> +  SECStatus rv = certVerifier->VerifyCert(mCert,
> +                                          certificateUsageSSLServer, PR_Now(),
> +                                          nullptr /* XXX pinarg*/,

I'm assuming this (the pinarg) gets taken care of later.
Attachment #705032 - Flags: feedback+
Comment on attachment 705455 [details] [diff] [review]
part 3: relax dv chain verification

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

::: security/manager/ssl/src/CertVerifier.cpp
@@ +131,5 @@
>  
>      // no local crl and don't know where to get it from? ignore
>      | CERT_REV_M_SKIP_TEST_ON_MISSING_SOURCE
> +    ;
> +  strictCRLRevocationFlags = commonCRLRevocationFlags 

There's some unnecessary spaces at the end of a few lines around here.

@@ +156,5 @@
>  
>      // ocsp success is sufficient
>      | CERT_REV_M_STOP_TESTING_ON_FRESH_INFO
>      ;
> +  strictOCSPRevocationFlags = commonOCSPRevocationFlags 

Some more whitespace.
Attachment #705455 - Flags: feedback+
Comment on attachment 705036 [details] [diff] [review]
part4: calculate verification chain in verifycert

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

::: security/manager/ssl/src/CertVerifier.cpp
@@ +93,5 @@
> +                                          verify_log, nullptr);
> +    }
> +    if (rv == SECSuccess && validationChain ) {
> +      SECCertUsage enumUsage = certUsageSSLClient;
> +      if( usage == certificateUsageSSLServer ) {

"if (usage == ..."

or maybe do this as an inline-if:

SECCertUsage enumUsage = (usage == certificateUsageSSLServer) ? certUsageSSLServer : certUsageSSLClient;

I guess that's kind-of long...

@@ +231,5 @@
>      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: verifycert: looks like an EV verify! \n"));
>    }
>  
> +  i=0;
> +  int validationChainLocation=0;

"i = 0", "validationChainLocation = 0"
Also, should validationChainLocation be size_t?

@@ +257,5 @@
>      cvin[evParamLocation].type = cert_pi_end;
>      rv = CERT_PKIXVerifyCert(cert, usage, cvin, cvout, pinArg);
>    }
>  
> +  //this section needs some revision for error reporting/detecting

In a follow-up? Or will this patch be revised?

@@ +272,5 @@
> +      *validationChain = cvout[validationChainLocation].value.pointer.chain;
> +      if (issuerCert) {
> +        PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: I might need to  adding issuer to tail for display\n"));
> +        //to reuse rv ot not to reuse, that is the question.
> +        if(!CERT_CompareCerts(issuerCert, cert)){

"if (...) {"

@@ +274,5 @@
> +        PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: I might need to  adding issuer to tail for display\n"));
> +        //to reuse rv ot not to reuse, that is the question.
> +        if(!CERT_CompareCerts(issuerCert, cert)){
> +          PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: verifycert:  adding issuer to tail for display\n"));
> +          rv = CERT_AddCertToListTail(*validationChain, 

Unnecessary space at the end.

@@ +281,5 @@
> +          //issuerCert = nullptr;
> +        }
> +      }
> +    }
> +    else{

"} else {" all on one line, I think.

@@ +283,5 @@
> +      }
> +    }
> +    else{
> +      //no chain found, add the cert itself as a chain?, only on success?
> +      if ( rv == SECSuccess ){

"if (rv == SECSuccess) {"

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +663,5 @@
>        return SECFailure; 
>      }
>  
>      SECOidTag *evOidPolicy = NULL;
> +  

Unnecessary whitespace.
Attachment #705036 - Flags: feedback+
Comment on attachment 705039 [details] [diff] [review]
part 5: getting chain from verifycert

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

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +901,5 @@
> +    certList = verifyCertChain;
> +  }
> +  else{
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: SSLauth failed to get chain from verifycert\n"));
> +    certList = CERT_GetCertChainFromCert(cert, PR_Now(), certUsageSSLCA); 

Trailing space.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +818,3 @@
>    ScopedCERTCertList nssChain;
> +
> +  {

Is there supposed to be an if here? (I guess I don't see the reason for the scope here...)

@@ +824,5 @@
> +    NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED);
> +    CERTCertList *pkixNssChain = nullptr;
> +
> +    //try as server...
> +    if (!pkixNssChain ) {

Isn't this always true?

@@ +825,5 @@
> +    CERTCertList *pkixNssChain = nullptr;
> +
> +    //try as server...
> +    if (!pkixNssChain ) {
> +      //mozilla::psm::TransportSecurityInfo infoObject;

Delete commented-out code?

@@ +831,5 @@
> +      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: PKIX attempting chain(server) for '%s'\n", mCert->nickname));
> +      srv = certVerifier->VerifyCert(mCert,
> +                                     certificateUsageSSLServer, PR_Now(),
> +                                     nullptr, /*XXX fixme*/
> +                                     0, 

Trailing space here and a couple more some lines down.

@@ +840,5 @@
> +      //try as something else?
> +    }
> +    for (int usage = certificateUsageSSLClient; 
> +         usage < certificateUsageAnyCA && !pkixNssChain;
> +         usage = usage<<1) {

I'm not sure we want to rely on this working this way... maybe have an array of allowed usages and loop through that?

@@ +857,5 @@
> +    if (!pkixNssChain) {
> +       //do fallback.
> +       PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: getchain :CertVerify failed to get chain for '%s'\n", mCert->nickname));
> +       nssChain = CERT_GetCertChainFromCert(mCert, PR_Now(), certUsageSSLClient);
> +    }

"} else {" all on one line, I think.

@@ +865,5 @@
> +    } 
> +
> +  }
> +
> +  //nssChain = CERT_GetCertChainFromCert(mCert, PR_Now(), certUsageSSLClient);

Delete commented-out code?
Attachment #705039 - Flags: feedback+
Comment on attachment 705108 [details] [diff] [review]
part 6: getting verifylog from verifycert

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

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ -477,5 @@
>    PLArenaPool *log_arena = nullptr;
>    PLArenaPoolCleanerFalseParam log_arena_cleaner(log_arena);
>    CERTVerifyLog * verify_log = nullptr;
>  
> -  if (true) { // XXX: if not libpkix

Ok, now this makes sense.

@@ +493,3 @@
>  
> +  srv = certVerifier->VerifyCert(cert, certificateUsageSSLServer, now,
> +                                   infoObject, 0, nullptr, nullptr, verify_log);

Looks like this indentation doesn't line up - the second line needs to move two spaces to the left.
Attachment #705108 - Flags: feedback+
Comment on attachment 704051 [details] [diff] [review]
part 7: remove verifycertnow

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

Yay for deleting things!

::: security/manager/ssl/src/nsUsageArrayHelper.cpp
@@ +205,5 @@
>    NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED);
>  
>    PRTime now = PR_Now();
>    CertVerifier::Flags flags = localOnly ? CertVerifier::FLAG_LOCAL_ONLY : 0;
>    

As long as you're removing things, maybe clean up this unnecessary whitespace too?
Attachment #704051 - Flags: feedback+
Comment on attachment 701998 [details] [diff] [review]
centralized verify (p1)

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

Thank you Keeler. (lets hope this will not eat my comments again)

::: security/manager/ssl/src/nsCERTValInParamWrapper.cpp
@@ +132,5 @@
> +  cvin[2].type = cert_pi_date;
> +  cvin[2].value.scalar.time = time;
> +  cvin[3].type = cert_pi_end;
> +
> +  CERTValOutParam cvout[2];

Yes. Is fixed on subsequent parts

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +484,2 @@
>  
> +  if (true) { // XXX: if not libpkix

Yes (mostly). This gets finllay fixed by bug 699874 (which depends on this) and is already coded.

@@ +565,3 @@
>      }
> +  } else {
> +    // XXX set errorCodeTrust, errorCodeMismatch, errorCodeExpired, collected_errors

halft in later parts and half In a leter bug (699874)
Attached patch part 1: centralized verify (obsolete) — Splinter Review
Attached patch part 1: centralized verify (obsolete) — Splinter Review
Addressing all spacing coments for new lines. (old lines with traling spaces left as is.
Attachment #706176 - Attachment is obsolete: true
removed all space issues of new lines.
Attachment #706421 - Attachment is obsolete: true
Attachment #706436 - Attachment is obsolete: true
Comment on attachment 705032 [details] [diff] [review]
part 2: centralize EV verification

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

::: security/manager/ssl/src/CertVerifier.cpp
@@ +164,5 @@
>          : CERT_REV_MI_NO_OVERALL_INFO_REQUIREMENT)
>      ;
>  
> +  CERTValInParam cvin[6];
> +  size_t i = 3;

Yes, agreed.

@@ +173,5 @@
>    cvin[1].value.pointer.revocation = &rev;
>    cvin[2].type = cert_pi_date;
>    cvin[2].value.scalar.time = time;
> +
> +  size_t evParamLocation = i;

const yes. but no 3, as I expect the value to change as we add more mandatory parameters to the checks

@@ +204,5 @@
> +    rv = CERT_PKIXVerifyCert(cert, usage, cvin, cvout, pinArg);
> +  }
> +
> +  if (trustAnchors) {
> +    //do cleanup , need to error check too

After going back to the code, there is no way to check if cleanup was successful, the function is a void function
Attachment #705032 - Attachment is obsolete: true
Attachment #706467 - Attachment is obsolete: true
Attachment #706511 - Attachment is obsolete: true
Comment on attachment 705039 [details] [diff] [review]
part 5: getting chain from verifycert

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

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +901,5 @@
> +    certList = verifyCertChain;
> +  }
> +  else{
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: SSLauth failed to get chain from verifycert\n"));
> +    certList = CERT_GetCertChainFromCert(cert, PR_Now(), certUsageSSLCA); 

And bad else!

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +818,3 @@
>    ScopedCERTCertList nssChain;
> +
> +  {

agreed not anymore, much cruft left on this one.

@@ +824,5 @@
> +    NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED);
> +    CERTCertList *pkixNssChain = nullptr;
> +
> +    //try as server...
> +    if (!pkixNssChain ) {

Yes

@@ +825,5 @@
> +    CERTCertList *pkixNssChain = nullptr;
> +
> +    //try as server...
> +    if (!pkixNssChain ) {
> +      //mozilla::psm::TransportSecurityInfo infoObject;

ACK

@@ +840,5 @@
> +      //try as something else?
> +    }
> +    for (int usage = certificateUsageSSLClient; 
> +         usage < certificateUsageAnyCA && !pkixNssChain;
> +         usage = usage<<1) {

I am on the fence on this one. Yes an array could make it cleaner in some ways, but I do not want to maitain another constant.

Will keep as is and wait for bsmith/mayhammer comments

@@ +865,5 @@
> +    } 
> +
> +  }
> +
> +  //nssChain = CERT_GetCertChainFromCert(mCert, PR_Now(), certUsageSSLClient);

ACK
Attachment #706513 - Attachment is obsolete: true
forgot to qrefresh.
Attachment #706567 - Attachment is obsolete: true
forgot to mark as patch.
Attachment #706468 - Flags: review?(bsmith)
Attachment #706474 - Flags: review?(bsmith)
Just to remove the part5 that was not marked as obsolete,
Attachment #706568 - Attachment is obsolete: true
Attachment #706562 - Flags: review?(bsmith)
Attachment #706570 - Flags: review?(bsmith)
Consider this code:

  rv = CERT_PKIXVerifyCert(cert, usage, cvin, cvout, pinArg);
  if (rv != SECSuccess && doingEvValidation) {
    //Do fallback for Ev (remove ev constraints)
    *evOidPolicy = SEC_OID_UNKNOWN;
    cvin[evParamLocation].type = cert_pi_end;
    rv = CERT_PKIXVerifyCert(cert, usage, cvin, cvout, pinArg);
  }

in the classic (non-libpkix) mode, when doingEVValidation, the second call to CERT_PKIXVerifyCert should be a call to CERT_VerifyCert[ificate] + CERT_GetCertChainFromCert, not a call to CERT_PKIXVerifyCert.

The code currently has a control flow like this:

    IF not using libpkix AND not doing EV validation THEN
       IF SSL THEN
          rv = CERT_VerifyCert(...);
       ELSE
          rv = CERT_VerifyCertificate(...);
       END IF
       return rv;
    ENDIF

    IF doing EV validation THEN
       configure things for EV validation
    ELSE
       configure things for non-EV validation
    ENDIF
    rv = CERT_PKIXVerifyCert(...);
    IF rv != SECSuccess and doing EV validation THEN
       reconfigure for non-EV validation
       rv = CERT_PKIXVerifyCert(...)
    END IF

I think the control flow should instead be:

    IF doing EV validation OR in libpkix-by-default mode THEN
        Configure cvin/cvout for EV validation
    END IF
    
    IF doing EV validation THEN
       rv = CERT_PKIXVerifyCert(...)
       IF rv == SECSuccess THEN
          nrv = CopyResultsFromCvOut(...);
          IF NS_FAILED(nrv) THEN
             PR_SetError(...);
             rv = SECFailure;
          END IF;
          return rv;
       END IF
    END IF

    IF in classic (non-libpkix mode) THEN
       IF ssl usage THEN
          rv = CERT_VerifyCert(...);
       ELSE
          rv = CERT_VerifyCertificate(...);
       END IF
       IF rv == SECSuccess THEN
          ...
          ... CERT_GetCertChainFromCert ...
          ...
       END IF
       return rv;
    endif

    reconfigure cvin/cvout for non-EV validation
    rv = CERT_PKIXVerifyCert(...);
    IF rv != SECsuccess THEN
       return rv;
    END IF

    nrv = CopyResultsFromCvOut(...);
    IF NS_FAILED(nrv) THEN
        PR_SetError(...);
        returnb SECFailure;
    END IF;

    return SECSuccess;

Also, I did not check this too closely, but when you deal with cvin/cvout, make sure that you are moving things to Scoped* types in the appropriate places so as to avoid memory leaks. For example, if the first call to CERT_PKIXVerifyCert fails, then it might be the case that cvout contains some results that need to be freed.

When you re-post this for review, please go ahead and combine all the patches that touch CertVerifer.cpp (including the part I wrote). I found that the combined patch is easier to review because there are too many inter-dependencies between the various parts.
Comment on attachment 706570 [details] [diff] [review]
part 5 :getting chain from certverify (post-keeler)

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

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +813,5 @@
>    NS_ENSURE_ARG(_rvChain);
>    nsresult rv;
>    /* Get the cert chain from NSS */
>    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("Getting chain for \"%s\"\n", mCert->nickname));
> +

I suggest that, instead of modifying GetChain(), we just leave it as it is, until we can remove it. We are not supposed to do any certificate database operations on the main thread, so this synchronous getChain() method eventually needs to be removed. Also, this method is misleading and somewhat meaningless because in order to get a good chain, you would need to tell getChain what you consider a good chain--which usage, etc.

The two existing callers in Gecko/Firefox are viewCertDetails.js and pippki.js/exportToFile.

I suggest that we (eventually, not in this bug) change pippki.js/exportToFile to take a nsIX509CertList as a parameter, and then just have it export that cert list without modification.

We know that eventually we need to have the cert details dialog box take as input a cert chain to display, so that we can ensure that we display the exact cert chain we constructed during the SSL handshake. That means that the opener of the viewCertDetails dialog box should be responsible for constructing the chain if it doesn't have one.

In particular, eventually, when certManager.js needs to call exportToFile or when it needs to show the cert details dialog box, then it first should use the asynchronous cert validation API mentioned above to construct a cert chain.

Note that, in particular, the usage parameter for the cert validation will be different depending on what tab of the certificate manager dialog is open. For example, it will want to validate the certificate for CA usages for the "Authorities" tab, for SSL usages for the "Servers" tab, and for "client authentication" for the "Your certificates/People" tabs.

So, ideally, this getChain method should be replaced with a method (on nsIX509CertDB, not nsIX509Cert) that is basically a scriptable, asynchronous XPCOM wrapper around CertVerifier, something like:

[scriptable, function]
interface nsIX509CertVerificationCallback {
   void verifyCertFinished(nsresult rv,
                           optional in nsIX509CertList list);
};

interface nsIX509CertDB {
  ...
  void verifyCert(in nsIX509Cert cert,
                  in nsX509CertUsage usage,
                  in PRTime time,
                  in nsIX509CertVerificationCallback);
  ...
};

The question is what to do about that in the interim. I think that, if making getChain asynchronous is too much work, then doing something like what you propose below may be OK in the interim. But, at a minimum, we should pass the LOCAL_ONLY flag to VerifyCert here so that we don't do OCSP fetches on the main thread. Lets talk about it on Wednesday.
No longer blocks: 672811
Depends on: 672811
Thank you for taking a look at this.

Agreed and done: put as many Scoped Types as possible.

Some disagreement:
I agree that the current control flow doesn't handle properly the case of fallback of ev in classic mode. However I want to deviate from your suggested control flow as the cleanup section for libpkix mode is more complex than the 'classic control' flow:

Here is my suggestion:

    IF not using libpkix AND not doing EV validation THEN
       return doClassicVerify(...)
    ENDIF

    IF doing EV validation THEN
       configure things for EV validation
    ELSE
       configure things for non-EV validation
    ENDIF
    rv = CERT_PKIXVerifyCert(...);
    IF rv != SECSuccess and doing EV validation THEN
       mark output as non-ev
       IF (not using libpkix)
          return doClassicVerify(...)
       ENDIF 
       reconfigure for non-EV validation
       rv = CERT_PKIXVerifyCert(...)
    END IF
    HandlePKIX-cvin-cvout-errcase


This still the correct behaviour and is simpler to code :)
ACtually after coding it, with a forward goto, your code path is not that bad!
Attached patch part 1: centralized verify (obsolete) — Splinter Review
Attachment #690966 - Attachment is obsolete: true
Attachment #701998 - Attachment is obsolete: true
Attachment #705036 - Attachment is obsolete: true
Attachment #705455 - Attachment is obsolete: true
Attachment #706456 - Attachment is obsolete: true
Attachment #706468 - Attachment is obsolete: true
Attachment #706474 - Attachment is obsolete: true
Attachment #706562 - Attachment is obsolete: true
Attachment #706468 - Flags: review?(bsmith)
Attachment #706474 - Flags: review?(bsmith)
Attachment #706562 - Flags: review?(bsmith)
Comment on attachment 708372 [details] [diff] [review]
part 1: centralized verify

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

Style issues:

* Comments should usually start with space followed by a capital letter. If the comment is a sentence, it should end in a period. Abbreviations like EV, CRLs, etc. should be capitalized.

* Output parameters should be labeled "/*out*/ ReturnType * parameterName" and optional parameters should be "/*optional out*/ ReturnTyime * parameterName", in both the header and in the source file.

* variable names and parameters are camelCase, not separated_with_underscores.

* Do we need to prefix log messages with "pipnss:"? I thought NSPR logging prefixes things for us?

::: security/manager/ssl/src/nsCERTValInParamWrapper.cpp
@@ +46,5 @@
> +                  const SECCertificateUsage usage,
> +                  const PRTime time,
> +                  nsIInterfaceRequestor * pinArg,
> +                  CERTCertList **validationChain,
> +                  CERTVerifyLog *verify_log)

s/verify_log/verifyLog/

Document which parameters are output parameters using /*out*/, e.g.:

/*out*/ CERTCertList **validationChain,
/*out*/ CERTVerifyLog *verify_log)

@@ +60,5 @@
> +                         certUsageSSLServer, time, pinArg, verify_log);
> +  } else {
> +    rv = CERT_VerifyCertificate(CERT_GetDefaultCertDB(), cert, true,
> +                                        usage, time, pinArg,
> +                                        verify_log, nullptr);

indention.

@@ +66,5 @@
> +  if (rv == SECSuccess && validationChain) {
> +    SECCertUsage enumUsage = certUsageSSLClient;
> +    if (usage == certificateUsageSSLServer) {
> +      enumUsage = certUsageSSLServer;
> +    }

Map certificateUsage to the exactly corresponding certUsage, instead of defaulting to certUsageSSLClient.

@@ +67,5 @@
> +    SECCertUsage enumUsage = certUsageSSLClient;
> +    if (usage == certificateUsageSSLServer) {
> +      enumUsage = certUsageSSLServer;
> +    }
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: verifycert: getting chain in 'classic' \n"));

s/pipnss: verifycert:/VerifyCert:/

@@ +85,5 @@
> +                         const Flags flags,
> +                         CERTCertList **validationChain,
> +                         SECOidTag *evOidPolicy,
> +                         CERTVerifyLog *verify_log)
> +{

MOZ_ASSERT(cert);

@@ +99,4 @@
>    }
> +  *evOidPolicy = SEC_OID_UNKNOWN;
> +
> +  //do ev checking only for sslserver usage

// Do EV validation for all SSL server usage, and only for SSL server usage.

@@ +100,5 @@
> +  *evOidPolicy = SEC_OID_UNKNOWN;
> +
> +  //do ev checking only for sslserver usage
> +  if (usage == certificateUsageSSLServer) {
> +    SECStatus nrv = getFirstEVPolicy(cert, *evOidPolicy);

We should not set *evOidPolicy until after we've verified that the certificate validation has succeeded. That way, it is clearer that we don't accidentally leave *evOidPolicy set in the case where EV validation fails but DV validation succeeds.

(More generally, avoid setting an output parameter until you're sure that the value is correct/safe to set.)

@@ +106,5 @@
> +      trustAnchors = getRootsForOid(*evOidPolicy);
> +    }
> +    if (trustAnchors) {
> +      doingEvValidation = true;
> +    }

} else {

@@ +126,5 @@
> +  rev.leafTests.number_of_preferred_methods =
> +  rev.chainTests.number_of_preferred_methods = 1;
> +
> +  rev.leafTests.number_of_defined_methods =
> +  rev.chainTests.number_of_defined_methods = cert_revocation_method_ocsp +1;

" + 1"

@@ +133,5 @@
> +  uint64_t relaxedCRLRevocationFlags;
> +  uint64_t commonCRLRevocationFlags;
> +  uint64_t strictOCSPRevocationFlags;
> +  uint64_t relaxedOCSPRevocationFlags;
> +  uint64_t commonOCSPRevocationFlags;

This code that sets the revocation flags has become way too complicated--especially the way the flags are reset when we do the DV case.

I suggest that you factor out all the flag setting into a separate method that takes all the relevant inputs as parameters and then configures the CERTRevocationFlags based on those inputs. Then, you can call the new function once for the EV case and then again for the DV case. I think that would make things much easier to understand.

And/or, instead of splitting things up as "strict" vs. "relaxed", perhaps they should be separated into EV vs DV.

@@ +204,3 @@
>      ;
>  
> +  CERTValInParam cvin[6];

Add:
<blank line>
// Parameters used for both EV and DV validation

@@ +209,5 @@
> +  cvin[1].type = cert_pi_revocationFlags;
> +  cvin[1].value.pointer.revocation = &rev;
> +  cvin[2].type = cert_pi_date;
> +  cvin[2].value.scalar.time = time;
> +

// Parameters used only for EV validation.

@@ +212,5 @@
> +  cvin[2].value.scalar.time = time;
> +
> +  size_t i = 3;
> +  const size_t evParamLocation = i;
> +

remove blank line.

@@ +219,5 @@
> +    cvin[i].value.arraySize = 1;
> +    cvin[i].value.array.oids = evOidPolicy;
> +    ++i;
> +  }
> +  if (trustAnchors) {

Isn't it always the case that these are all always true or always false?:

*evOidPolicy != SEC_OID_UNKNOWN
doingEVValidation
trustAnchors

If so, then doingEVValidation is redundant and can be removed.

@@ +226,5 @@
> +    ++i;
> +  }
> +  cvin[i].type = cert_pi_end;
> +
> +  if(trustAnchors && *evOidPolicy != SEC_OID_UNKNOWN) {

whitespace

@@ +231,5 @@
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: verifycert: looks like an EV verify! \n"));
> +  }
> +
> +  i = 0;
> +  size_t validationChainLocation = 0;

No need to initialize validationChainLocation here.

@@ +233,5 @@
> +
> +  i = 0;
> +  size_t validationChainLocation = 0;
> +  CERTValOutParam cvout[4];
> +  if (verify_log) {

verifyLogLocation = i;

@@ +245,5 @@
> +    cvout[i].type = cert_po_certList;
> +    cvout[i].value.pointer.cert = NULL;
> +    cvout[i+1].type = cert_po_trustAnchor;
> +    cvout[i+1].value.pointer.chain = NULL;
> +    i+=2;

whitespace.

@@ +287,5 @@
> +  // skip ev parameters
> +  cvin[evParamLocation].type = cert_pi_end;
> +
> +  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: verifycert: calling CERT_PKIXVerifyCert(dv) \n"));
> +  rv = CERT_PKIXVerifyCert(cert, usage, cvin, cvout, pinArg);

You need to save PR_GetError() here and restore it before returning.

@@ +290,5 @@
> +  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: verifycert: calling CERT_PKIXVerifyCert(dv) \n"));
> +  rv = CERT_PKIXVerifyCert(cert, usage, cvin, cvout, pinArg);
> + 
> +pkix_done:
> +  if (validationChain) {

We should not return the validation chain on validation failure, because there is really no sensible chain to return. So, this should be:

  if (rv == SECSuccess && validationChain) {
  }

But, you should also make sure that you correctly free the validation chain in the rv != SECSuccess chain in case libpkix set it.

::: security/manager/ssl/src/CertVerifier.h
@@ +22,5 @@
> +
> +  typedef unsigned int Flags;
> +  static const Flags FLAG_LOCAL_ONLY;
> +
> +  // policy == SEC_OID_UNKNOWN means "do not verify for policy."

There is no policy parameter.

@@ +23,5 @@
> +  typedef unsigned int Flags;
> +  static const Flags FLAG_LOCAL_ONLY;
> +
> +  // policy == SEC_OID_UNKNOWN means "do not verify for policy."
> +  // trustAnchors == nullptr means "determine trust based on NSS trust database."

There is no trustAnchors parameter

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +474,1 @@
>      NS_ERROR("GetDefaultCERTValInParam failed");

Update message (Do a recursive search for "GetDefaultCERTValInParam").

@@ +484,2 @@
>  
> +  if (true) { // XXX: if not libpkix

I didn't review this part yet. Is it possible to fold the other changes to this file into this patch too?

::: security/manager/ssl/src/nsIdentityChecking.cpp
@@ +1214,5 @@
>  
> +  SECStatus rv = certVerifier->VerifyCert(mCert,
> +                                          certificateUsageSSLServer, PR_Now(),
> +                                          nullptr /* XXX pinarg*/,
> +                                          0, nullptr,  &resultOidTag);

whitespace

@@ +1219,2 @@
>  
> +  if (rv != SECSuccess){

whitespace.

@@ +1222,2 @@
>    }
> +  if (resultOidTag != SEC_OID_UNKNOWN){

whitespace

@@ +1257,5 @@
>    *aIsEV = false;
>  
>    if (mCachedEVStatus != ev_status_unknown) {
>      *aIsEV = (mCachedEVStatus == ev_status_valid);
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: NSSCertificate::GEtisExtval IS cached! \n"));

What is GEtisExtval?

@@ +1262,3 @@
>      return NS_OK;
>    }
> +  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: NSSCertificate::GEtisExtval NOT cached! \n"));

What is GEtisExtval?
Attachment #708372 - Attachment is obsolete: true
Comment on attachment 708372 [details] [diff] [review]
part 1: centralized verify

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

::: security/manager/ssl/src/nsCERTValInParamWrapper.cpp
@@ +46,5 @@
> +                  const SECCertificateUsage usage,
> +                  const PRTime time,
> +                  nsIInterfaceRequestor * pinArg,
> +                  CERTCertList **validationChain,
> +                  CERTVerifyLog *verify_log)

Done

@@ +60,5 @@
> +                         certUsageSSLServer, time, pinArg, verify_log);
> +  } else {
> +    rv = CERT_VerifyCertificate(CERT_GetDefaultCertDB(), cert, true,
> +                                        usage, time, pinArg,
> +                                        verify_log, nullptr);

Done

@@ +66,5 @@
> +  if (rv == SECSuccess && validationChain) {
> +    SECCertUsage enumUsage = certUsageSSLClient;
> +    if (usage == certificateUsageSSLServer) {
> +      enumUsage = certUsageSSLServer;
> +    }

Done

@@ +67,5 @@
> +    SECCertUsage enumUsage = certUsageSSLClient;
> +    if (usage == certificateUsageSSLServer) {
> +      enumUsage = certUsageSSLServer;
> +    }
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: verifycert: getting chain in 'classic' \n"));

Done

@@ +85,5 @@
> +                         const Flags flags,
> +                         CERTCertList **validationChain,
> +                         SECOidTag *evOidPolicy,
> +                         CERTVerifyLog *verify_log)
> +{

ACK.

@@ +100,5 @@
> +  *evOidPolicy = SEC_OID_UNKNOWN;
> +
> +  //do ev checking only for sslserver usage
> +  if (usage == certificateUsageSSLServer) {
> +    SECStatus nrv = getFirstEVPolicy(cert, *evOidPolicy);

Made some changes to it. Now is only set twice:
at the begginig assuming it is dv and at after ev if it succeeds

@@ +106,5 @@
> +      trustAnchors = getRootsForOid(*evOidPolicy);
> +    }
> +    if (trustAnchors) {
> +      doingEvValidation = true;
> +    }

done

@@ +126,5 @@
> +  rev.leafTests.number_of_preferred_methods =
> +  rev.chainTests.number_of_preferred_methods = 1;
> +
> +  rev.leafTests.number_of_defined_methods =
> +  rev.chainTests.number_of_defined_methods = cert_revocation_method_ocsp +1;

ack

@@ +133,5 @@
> +  uint64_t relaxedCRLRevocationFlags;
> +  uint64_t commonCRLRevocationFlags;
> +  uint64_t strictOCSPRevocationFlags;
> +  uint64_t relaxedOCSPRevocationFlags;
> +  uint64_t commonOCSPRevocationFlags;

I clean it up a bit, lets take another look in the next patch

@@ +219,5 @@
> +    cvin[i].value.arraySize = 1;
> +    cvin[i].value.array.oids = evOidPolicy;
> +    ++i;
> +  }
> +  if (trustAnchors) {

removed doingEVValidation.

@@ +287,5 @@
> +  // skip ev parameters
> +  cvin[evParamLocation].type = cert_pi_end;
> +
> +  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: verifycert: calling CERT_PKIXVerifyCert(dv) \n"));
> +  rv = CERT_PKIXVerifyCert(cert, usage, cvin, cvout, pinArg);

Can you elaborate on this?

@@ +290,5 @@
> +  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: verifycert: calling CERT_PKIXVerifyCert(dv) \n"));
> +  rv = CERT_PKIXVerifyCert(cert, usage, cvin, cvout, pinArg);
> + 
> +pkix_done:
> +  if (validationChain) {

reorganized to make more sense, but I was already doing what you said with restpect to the generation of the validation chain.

::: security/manager/ssl/src/CertVerifier.h
@@ +22,5 @@
> +
> +  typedef unsigned int Flags;
> +  static const Flags FLAG_LOCAL_ONLY;
> +
> +  // policy == SEC_OID_UNKNOWN means "do not verify for policy."

remoded

@@ +23,5 @@
> +  typedef unsigned int Flags;
> +  static const Flags FLAG_LOCAL_ONLY;
> +
> +  // policy == SEC_OID_UNKNOWN means "do not verify for policy."
> +  // trustAnchors == nullptr means "determine trust based on NSS trust database."

removed
Blocks: 836552
Attached file part 1: centralized verify (obsolete) —
Attachment #705108 - Attachment is obsolete: true
Attachment #708461 - Attachment is obsolete: true
Attached patch part 1: centralized verify (obsolete) — Splinter Review
Attachment #709171 - Attachment is obsolete: true
Attached patch part 1: centralized verify (obsolete) — Splinter Review
Attachment #709175 - Attachment is obsolete: true
Attachment #709211 - Flags: review?(bsmith)
Comment on attachment 709211 [details] [diff] [review]
part 1: centralized verify

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

Also check for line length > ~80 characters and the normal whitespace issues around parameters and operators.

::: security/manager/ssl/src/nsCERTValInParamWrapper.cpp
@@ +46,5 @@
> +                  const SECCertificateUsage usage,
> +                  const PRTime time,
> +                  nsIInterfaceRequestor * pinArg,
> +                  /*out*/ CERTCertList **validationChain,
> +                  /*out*/ CERTVerifyLog *verifyLog)

/*optional out*/ for both, right?

@@ +102,5 @@
> +      case certificateUsageAnyCA:
> +        enumUsage = certUsageAnyCA;
> +        break;
> +       default:
> +          enumUsage = certUsageSSLClient;

we should fail on unknown cert usage instead of defaulting to certUsageSSLClient.

@@ +121,5 @@
> +                         nsIInterfaceRequestor * pinArg,
> +                         const Flags flags,
> +                         CERTCertList **validationChain,
> +                         SECOidTag *evOidPolicy,
> +                         CERTVerifyLog *verifyLog)

label optional & output parameters with the /*optional*/ and /*out*/ comments.

@@ +137,2 @@
>    }
> +  *evOidPolicy = SEC_OID_UNKNOWN;

This would be clearer as:

if (evOidPolicy) {
  *evOidPolicy = SEC_OID_UNKNOWN;
}

Then, AFAICT, we don't need dummyEVPolicy at all.

@@ +144,5 @@
> +      if (evPolicy != SEC_OID_UNKNOWN ) {
> +        trustAnchors = getRootsForOid(evPolicy);
> +      }
> +      if (!trustAnchors) {
> +        evPolicy = SEC_OID_UNKNOWN;

if getRootsForOid returns nullptr then that means we ran out of memory and we should fail.

Also, I found two bugs in getRootsForOid: it doesn't check the result of CERT_AddCertToListTail or getRootsForOidFromExternalRootsFile.

@@ +165,5 @@
> +  rev.leafTests.number_of_preferred_methods =
> +  rev.chainTests.number_of_preferred_methods = 1;
> +
> +  rev.leafTests.number_of_defined_methods =
> +  rev.chainTests.number_of_defined_methods = cert_revocation_method_ocsp + 1;

This would be clearer as "= 2" since we use "2" elsewhere.

@@ +170,5 @@
> +
> +  uint64_t evCRLRevocationFlags;
> +  uint64_t commonCRLRevocationFlags;
> +  uint64_t evOCSPRevocationFlags;
> +  uint64_t commonOCSPRevocationFlags;

declare where initialized (e.g. uint64_t evCRLRevocationFlags = ...)

@@ +172,5 @@
> +  uint64_t commonCRLRevocationFlags;
> +  uint64_t evOCSPRevocationFlags;
> +  uint64_t commonOCSPRevocationFlags;
> +
> +  commonCRLRevocationFlags =

This doesn't seem to match the current logic as far as what is common to DV and EV.

The current DV logic for CRLs is:

  CERT_REV_M_TEST_USING_THIS_METHOD
| CERT_REV_M_IGNORE_IMPLICIT_DEFAULT_SOURCE
| CERT_REV_M_CONTINUE_TESTING_ON_FRESH_INFO
| CERT_REV_M_IGNORE_MISSING_FRESH_INFO
| CERT_REV_M_SKIP_TEST_ON_MISSING_SOURCE
| (mCRLDownloabedEnabled ?
   CERT_REV_M_ALLOW_NETWORK_FETCHING : CERT_REV_M_FORBID_NETWORK_FETCHING)
;

The current EV logic for CRLs is:

  CERT_REV_M_TEST_USING_THIS_METHOD
| (mOCSPDownloadEnabled ? CERT_REV_M_ALLOW_NETWORK_FETCHING
                        : CERT_REV_M_FORBID_NETWORK_FETCHING)
| CERT_REV_M_ALLOW_IMPLICIT_DEFAULT_SOURCE
| CERT_REV_M_REQUIRE_INFO_ON_MISSING_SOURCE
| CERT_REV_M_IGNORE_MISSING_FRESH_INFO
| CERT_REV_M_STOP_TESTING_ON_FRESH_INFO;

(Yes, the EV logic is currently testing mOCSPDownloadEnabled, not mCRLDownloadEnabled.)

Thus, it seems like what you are calling the "common" flags are not really common. I know that this doesn't make much sense. But, we should sort that out in another bug. For now, why don't you just cut/paste the existing code that sets the revocation flags for EV into this section, and then cut/paste the existing code that sets the revocation flags for DV into the DV section. Later, we can refactor things (and change the behavior) to figure out what is common between EV and DV.

@@ +193,2 @@
>      // crl download based on parameter
> +    | ((mCRLDownloadEnabled && !localOnly) ?

This is one example where this patch is changing the behavior significantly from what is currently being done.

@@ +254,5 @@
> +    cvin[i].value.arraySize = 1;
> +    cvin[i].value.array.oids = &evPolicy;
> +    ++i;
> +  }
> +  if (trustAnchors) {

Your code relies on trustAnchors will only be non-null when evPolicy != SEC_OID_UNKNOWN, and it will ALWAYS be non-null when evPolicy != SEC_OID_UNKNOWN. So, you can merge these two if statements together.

(Also, if trustAnchors was not NULL during a DV validation, then you would run into trouble when you set cvin[evParamLocation] = cert_pi_end below).

@@ +261,5 @@
> +    ++i;
> +  }
> +  cvin[i].type = cert_pi_end;
> +
> +  if (trustAnchors && evPolicy != SEC_OID_UNKNOWN) {

trustAnchors will only be non-null when evPolicy != SEC_OID_UNKNOWN so these conditions are redundant.

I suggest that you move this logging to happen in the (evPolicy != SEC_OID_UNKNOWN) section below.

@@ +267,5 @@
> +  }
> +
> +  i = 0;
> +  size_t validationChainLocation = 0;
> +  CERTValOutParam cvout[4];

I suggest that you initialize cvout before cvin, so that you can combine the two (evPolicy != SEC_OID_UNKNOWN) sections into the (evPolicy != SEC_OID_UKNOWN) section below. That would make this code easier to understand.

@@ +294,5 @@
> +    if (rv == SECSuccess) {
> +      *evOidPolicy = evPolicy;
> +      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: successful CERT_PKIXVerifyCert(ev) \n"));
> +      goto pkix_done;
> +    }

You need to destroy cvout[validationChainLocation].value.pointer.chain and verifyLog here, because of the early return when we call ClassicVerifyCert.

@@ +296,5 @@
> +      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: successful CERT_PKIXVerifyCert(ev) \n"));
> +      goto pkix_done;
> +    }
> +  }
> +  if (!localOnly && !nsNSSComponent::globalConstFlagUsePKIXVerification){

Why the check for localOnly here? Classic validation must be done whenever we're not doing EV if !nsNSSComponent::globalConstFlagUsePKIXVerification, right?

@@ +381,5 @@
> +      }
> +    } else {
> +      // Validation was a fail, clean up if needed
> +      if (cvout[validationChainLocation].value.pointer.chain) {
> +        CERT_DestroyCertList(cvout[validationChainLocation].value.pointer.chain);

It seems like there are some error cases in the logic above (e.g. when CERT_AddCertToListTail fails) where we want to destroy the validationChain and/or cvout[validationChainLocation].value.pointer.chain, where we currently aren't doing so.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +659,2 @@
>  
> +    SECOidTag *evOidPolicy = NULL;

s/NULL/nullptr/

@@ +661,4 @@
>  
> +    SECStatus rv = certVerifier->VerifyCert(peerCert,
> +                                            certificateUsageSSLServer, PR_Now(),
> +                                            pinarg, 0, nullptr , evOidPolicy);

This call will now do up to two validations. If the cert is a valid EV cert, it will do one; if it is not a valid EV cert then it will do two. That would be OK, except...

@@ +914,5 @@
>  
>    if (rv == SECSuccess) {
>      if (nsc) {
>        bool dummyIsEV;
>        nsc->GetIsExtendedValidation(&dummyIsEV); // the nsc object will cache the status

This call will ALSO do up to two validations, since it ends up calling certVerifier->VerifyCert.

To avoid a performance regression, you need to make it so that after the call to certVerifier->VerifyCert, you cache the EV OID into nsc like GetIsExtendedValidation does. For example, you could create a new method "VerifyCert" that is like GetIsExtendedValidation, but which returns a tri-state: { invalid, DV, EV }. If you make a new method on nsNSSCertificate then *don't* add it to any of the nsIX509Cert* interfaces.
Attachment #709211 - Flags: review?(bsmith) → review-
Attachment #705039 - Attachment is obsolete: true
Attachment #706570 - Attachment is obsolete: true
Attachment #706573 - Attachment is obsolete: true
Attachment #706570 - Flags: review?(bsmith)
Attached patch part 3: remove verifycertnow (obsolete) — Splinter Review
Attachment #704051 - Attachment is obsolete: true
Assignee: bsmith → cviecco
Attached patch part 1: centralized verify (v2) (obsolete) — Splinter Review
Comments adressed .. missing use -> do we cleanup or not the verify log on fail?
Attachment #709211 - Attachment is obsolete: true
Attached patch part 1: centralized verify (v2) (obsolete) — Splinter Review
Attachment #710845 - Attachment is obsolete: true
Comment on attachment 710855 [details] [diff] [review]
part 1: centralized verify (v2)

With the exception of the verifyLog cleanup (since is an append only I think is ok to reuse), all other issues are addressed.
Attachment #710855 - Flags: review?(bsmith)
Comment on attachment 710855 [details] [diff] [review]
part 1: centralized verify (v2)

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

Also remember to clean up trailing whitespace and other whitespace-related issues on lines you changed.

::: security/manager/ssl/src/nsCERTValInParamWrapper.cpp
@@ +125,5 @@
> +                         /*optional out*/ CERTCertList **validationChain,
> +                         /*optional out*/ SECOidTag *evOidPolicy,
> +                         /*optional out*/ CERTVerifyLog *verifyLog)
> +{
> +  MOZ_ASSERT(cert);

Nit: do you think it's OK to MOZ_ASSERT(cert) or should we just play it safe and do if (!cert) { PORT_SetError(SEC_ERROR_INVALID_ARGS); return SECFailure; }?

@@ +131,3 @@
>  
> +  ScopedCERTCertList trustAnchors;
> +  SECStatus rv;

Declare ev and localOnly as close to where they are first used as possible.

@@ +135,1 @@
>  

We should initialize *validationChain to nullptr here, if validatoinChain isn't null.

@@ +137,5 @@
> +    *evOidPolicy = SEC_OID_UNKNOWN;
> +  }
> +  // Do ev checking only for sslserver usage
> +  if (usage == certificateUsageSSLServer) {
> +    SECStatus nrv = getFirstEVPolicy(cert, evPolicy);

SECStatus variables should not be named nrv because nrv is a convention used for nsresult variables. (Conversely, nsresult variables should never have the name srv).

@@ +254,5 @@
> +        *evOidPolicy = evPolicy;
> +      }
> +      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, 
> +             ("VerifyCert: successful CERT_PKIXVerifyCert(ev) \n"));
> +      goto pkix_done;

Please add a comment like: // XXX remove this goto when we remove the ClassicVerifyCert code path.

@@ +258,5 @@
> +      goto pkix_done;
> +    }
> +    // There SHOULD not be a validation chain on failure, as this would create a
> +    // memory leak.
> +    MOZ_ASSERT_IF(validationChain, !*validationChain);

Are you sure there's never a case where libpkix starts creating the validation chain and then gives up without deleting this? It seems safer to just do something like:

if (validationChain && *validationChain) {
   CERT_DestroyCertList(*validationChain);
   *validationChain = nullptr;
}

@@ +259,5 @@
> +    }
> +    // There SHOULD not be a validation chain on failure, as this would create a
> +    // memory leak.
> +    MOZ_ASSERT_IF(validationChain, !*validationChain);
> +  }

blank line here.

@@ +260,5 @@
> +    // There SHOULD not be a validation chain on failure, as this would create a
> +    // memory leak.
> +    MOZ_ASSERT_IF(validationChain, !*validationChain);
> +  }
> +  if (!nsNSSComponent::globalConstFlagUsePKIXVerification){

Add a comment here about why it is OK to ignore localOnly in classic mode.

@@ +263,5 @@
> +  }
> +  if (!nsNSSComponent::globalConstFlagUsePKIXVerification){
> +    // Note: There is no need to cleanup the verify log as the log is append
> +    // only, at worse (on failure of classic mode) the log will contain
> +    // duplicate errors.

Unfortunately we cannot use this trick. Consider the case where the EV validation failed because an intermediate cert in the chain was REVOKED. The classic verification may build a chain that doesn't use that revoked certificate, but which may have some other error. In that case, you don't want the REVOKED error from the EV validation to be mixed in with the other errors from the classic verification because then the classic verification errors will no longer be overridable.

BUT GOOD NEWS: There is only one caller that tries to get the verify log--SSLServerCertVerification. And, we also know that we have a lot of doubts about the accuracy and completeness of the information of the verify log from CERT_PKIXVerifyCert. So, I propose that we only return the verify log during classic verification, and always leave it untouched during libpkix-based verification. Then in SSLServerCertVerification, we can add a nsNSSComponent::globalConstFlagUsePKIXVerification that uses the verifyLog for classic mode and which uses the "verify at expiration time" trick for PKIX mode. If we do that, we just need to add an "XXX remove the verifyLog parameter when we remove the classic mode" comment to the header file.

@@ +334,5 @@
> +
> +  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: calling CERT_PKIXVerifyCert(dv) \n"));
> +  rv = CERT_PKIXVerifyCert(cert, usage, cvin, cvout, pinArg);
> +
> +pkix_done:

if rv != SECSuccess then we cannot assume anything about the output parameters from CERT_PKIXVerifyCert except the error log (and even then, we don't have a lot of confidence in the accuracy of the information in the error log). So, the only thing we can do is destroy them if they are non-null. In particular, we shouldn't attempt to return a validation chain to the user if validation failed. I actually think that in some cases libpkix will return an incomplete chain, and in the error case it will almost definitely return the *worst* chain it can construct (the last one it tried), if it returns one at all. (Sorry I didn't notice this in the last round.)
Attachment #710855 - Flags: review?(bsmith) → review-
Comment on attachment 710855 [details] [diff] [review]
part 1: centralized verify (v2)

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

Thanks for the review. 
I think I have three issues pending before I submit a new revision:
1. keep the assert for debug build on the validate chain?
2. Cleanup of log is enough?
  2.1. Should I fold the fix for libpkix in this patchset?
3. Want me to rewrite the cleanup so that is more clear?

::: security/manager/ssl/src/nsCERTValInParamWrapper.cpp
@@ +254,5 @@
> +        *evOidPolicy = evPolicy;
> +      }
> +      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, 
> +             ("VerifyCert: successful CERT_PKIXVerifyCert(ev) \n"));
> +      goto pkix_done;

With the current structure the goto needs to stay. For the success case.

@@ +258,5 @@
> +      goto pkix_done;
> +    }
> +    // There SHOULD not be a validation chain on failure, as this would create a
> +    // memory leak.
> +    MOZ_ASSERT_IF(validationChain, !*validationChain);

I think to leave an assert for the debug builds. But since I am still not familiar for mozilla code this is my proposal (removing the assert is OK by me, but I would prefer to kn):

if (validationChain && *validationChain) {
  MOZ_ASSERT(false,"certPKIXVerifyCert returned failure AND a validationChain");
  CERT_DestroyCertList(*validationChain);
  *validationChain = nullptr;
}

@@ +263,5 @@
> +  }
> +  if (!nsNSSComponent::globalConstFlagUsePKIXVerification){
> +    // Note: There is no need to cleanup the verify log as the log is append
> +    // only, at worse (on failure of classic mode) the log will contain
> +    // duplicate errors.

At first sight this very sensible alternative, BUT then we would have to replicate the logic for the input parameters in another part of the code.

What do you think of
if EV failure{
 1. destroy log contents?
}

Do you want to fold the verifylog fixes into this patchset?

@@ +334,5 @@
> +
> +  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: calling CERT_PKIXVerifyCert(dv) \n"));
> +  rv = CERT_PKIXVerifyCert(cert, usage, cvin, cvout, pinArg);
> +
> +pkix_done:

Is this not what I am doing here? you want the structure so that it is more clear?
Attached patch part 1: centralized verify (v3) (obsolete) — Splinter Review
Attachment #710855 - Attachment is obsolete: true
Attached patch part 1: centralized verify (v3) (obsolete) — Splinter Review
Attachment #712581 - Attachment is obsolete: true
Attachment #712583 - Flags: review?(bsmith)
Comment on attachment 712583 [details] [diff] [review]
part 1: centralized verify (v3)

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

::: security/manager/ssl/src/nsCERTValInParamWrapper.cpp
@@ +140,5 @@
> +    *validationChain = nullptr;
> +  }
> +  if (evOidPolicy) {
> +    *evOidPolicy = SEC_OID_UNKNOWN;
> +  }

Nit: newline here.

Nit: Should we fail when (usage != certificateUsageSSLServer && evOidPolicy) because we don't return the EV policy when the usage is different?

Generally, parameter checking / initialization should go as early in the file as possible, so it would be good to move the initialization of *validationChain and *evOidPolicy above the declaration of trustAnchors.

@@ +144,5 @@
> +  }
> +  // Do ev checking only for sslserver usage
> +  if (usage == certificateUsageSSLServer) {
> +    SECStatus srv = getFirstEVPolicy(cert, evPolicy);
> +    if (srv == SECSuccess){

whitespace

@@ +145,5 @@
> +  // Do ev checking only for sslserver usage
> +  if (usage == certificateUsageSSLServer) {
> +    SECStatus srv = getFirstEVPolicy(cert, evPolicy);
> +    if (srv == SECSuccess){
> +      if (evPolicy != SEC_OID_UNKNOWN ) {

whitespace.

@@ +149,5 @@
> +      if (evPolicy != SEC_OID_UNKNOWN ) {
> +        trustAnchors = getRootsForOid(evPolicy);
> +      }
> +      if (!trustAnchors) {
> +        PR_SetError(PR_OUT_OF_MEMORY_ERROR, 0);

This PR_SetError isn't necessary, is it? getRootsForOid should be setting the error code itself, and I think it does (indirectly through CERT_NewCertList).

@@ +172,5 @@
> +  if (validationChain) {
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: setting up validation chain outparam.\n"));
> +    validationChainLocation = i;
> +    cvout[i].type = cert_po_certList;
> +    cvout[i].value.pointer.cert = NULL;

nullptr

@@ +174,5 @@
> +    validationChainLocation = i;
> +    cvout[i].type = cert_po_certList;
> +    cvout[i].value.pointer.cert = NULL;
> +    cvout[i+1].type = cert_po_trustAnchor;
> +    cvout[i+1].value.pointer.chain = NULL;

nullptr

@@ +175,5 @@
> +    cvout[i].type = cert_po_certList;
> +    cvout[i].value.pointer.cert = NULL;
> +    cvout[i+1].type = cert_po_trustAnchor;
> +    cvout[i+1].value.pointer.chain = NULL;
> +    i+=2;

whitespace.

Below, you use ++i;...++i; instead of i+=2 for similar code. I think that pattern of two increments is clearer because we have to keep track of how much we're incrementing i to avoid buffer overflows in cvout/cvin.

@@ +215,5 @@
> +
> +  if (evPolicy != SEC_OID_UNKNOWN) {
> +    // EV setup!
> +    // This flags are not quite correct, but it is what we have now, so keeping
> +    // them identical for bug landing purposes. Should be fixed later!

Include the bug number of the follow-up bug in the comment.

@@ +219,5 @@
> +    // them identical for bug landing purposes. Should be fixed later!
> +    uint64_t revMethodFlags =
> +      CERT_REV_M_TEST_USING_THIS_METHOD
> +      | (mOCSPDownloadEnabled ? CERT_REV_M_ALLOW_NETWORK_FETCHING
> +                                         : CERT_REV_M_FORBID_NETWORK_FETCHING)

alignment

@@ +224,5 @@
> +      | CERT_REV_M_ALLOW_IMPLICIT_DEFAULT_SOURCE
> +      | CERT_REV_M_REQUIRE_INFO_ON_MISSING_SOURCE
> +      | CERT_REV_M_IGNORE_MISSING_FRESH_INFO
> +      | CERT_REV_M_STOP_TESTING_ON_FRESH_INFO;
> + 

trailing whitespace.

@@ +265,5 @@
> +
> +    if (validationChain && *validationChain) {
> +      //There SHOULD not be a validation chain on failure, asserion here for
> +      // the debug builds AND a fallback for production builds
> +      MOZ_ASSERT(false,

MOZ_NOT_REACHED

@@ +271,5 @@
> +      CERT_DestroyCertList(*validationChain);
> +      *validationChain = nullptr;
> +    }
> +
> +    if (verifyLog){

whitespace.

@@ +276,5 @@
> +      // Cleanup the log so that it is ready the the next validation
> +      CERTVerifyLogNode *i_node;
> +      for (i_node = verifyLog->head; i_node; i_node = i_node->next) {
> +         //destroy cert if any.
> +         if (i_node->cert){

whitespace.

@@ +288,5 @@
> +    }
> +
> +  }
> +
> +  if (!nsNSSComponent::globalConstFlagUsePKIXVerification){

whitespace.

@@ +290,5 @@
> +  }
> +
> +  if (!nsNSSComponent::globalConstFlagUsePKIXVerification){
> +    // Note: we do not care about the localonly flag (currently) as the
> +    // caller that wants localonly should disable and reenable the fetching.

I would add a statement such as "localOnly is only used when libPKIX is enabled."

Also, s/Note:/XXX:/ since we know this is pretty bogus.

Also capitalization of "localonly" is wrong twice.

@@ +351,5 @@
>      // avoiding the network is good, let's try local first
>      CERT_REV_MI_TEST_ALL_LOCAL_INFORMATION_FIRST
>  
>      // is overall revocation requirement strict or relaxed?
> +    | ((mRequireRevocationInfo || mOCSPStrict) ?

Why add the mOCSPStrict condition here?

@@ +352,5 @@
>      CERT_REV_MI_TEST_ALL_LOCAL_INFORMATION_FIRST
>  
>      // is overall revocation requirement strict or relaxed?
> +    | ((mRequireRevocationInfo || mOCSPStrict) ?
> +       CERT_REV_MI_REQUIRE_SOME_FRESH_INFO_AVAILABLE: CERT_REV_MI_NO_OVERALL_INFO_REQUIREMENT)

whitespace.

@@ +357,3 @@
>      ;
>  
> +  // Skip ev parameters

s/ev/EV/

@@ +364,5 @@
> +
> +pkix_done:
> +  if (validationChain) {
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: validation chain requested\n"));
> +    ScopedCERTCertificate issuerCert(cvout[validationChainLocation+1].value.pointer.cert);

1. Doesn't the declaration of issuerCert need to be outside the "if validationChain" condition so that we don't leak cvout[validationChainLocation+1].value.pointer.cert when the caller isn't asking for a validation chain?

2. Instead of cvout[validationChainLocation+1], we should use cvout[trustAnchorLocation], and set trustAnchorLocation appropriately above. This way, there is less risk of failure if we change the ordering of the cvout array.

3. Wouldn't "trustAnchor" be a better name than "issuerCert"? To me, "issuerCert" indicates the immediate issuer of cert.

@@ +373,5 @@
> +        *validationChain = cvout[validationChainLocation].value.pointer.chain;
> +        if (issuerCert) {
> +          // we should only add the issuer to the chain if it is not already
> +          // present. On CA cert checking, the issuer is the same cert, so in
> +          // that case we do not add the cert to the chain.

I do not understand the conditions under which libpkix is returning a various combinations of cert chain or trust anchor. This should be documented in certt.h's documentation. The current documentation is:

   cert_po_trustAnchor     = 2, /* Return the trust anchor for the chain that
				 * was validated. Returned in 
				 * value.pointer.cert, this value is only 
				 * returned on SECSuccess. */
   cert_po_certList        = 3, /* Return the entire chain that was validated.
				 * Returned in value.pointer.certList. If no 
				 * chain could be constructed, this value 
				 * would be NULL. */

So, my understanding is that certList and trustAnchor should both never be null when CERT_PKIXVerifyCert returns SECSuccess, and if either are null it is a bug in libpkix.

The only case I can see where that would be different is when we are validating a trust anchor itself. In that case, should the trust anchor be returned in the cert chain, or as the trust anchor, or in both?

@@ +381,5 @@
> +            rv = CERT_AddCertToListTail(*validationChain,
> +                                        CERT_DupCertificate(issuerCert));
> +            if (rv != SECSuccess) {
> +              CERT_DestroyCertList(*validationChain);
> +              *validationChain=nullptr;

whitespace.

@@ +386,5 @@
> +            }
> +          }
> +        }
> +      } else {
> +        // no chain found but cert is valid. Trusted cert, add itself to chain

Is the issue here is that libpkix won't return a chain when we're validating a trust anchor?

@@ +387,5 @@
> +          }
> +        }
> +      } else {
> +        // no chain found but cert is valid. Trusted cert, add itself to chain
> +        *validationChain = CERT_NewCertList ();

whitespace.

@@ +389,5 @@
> +      } else {
> +        // no chain found but cert is valid. Trusted cert, add itself to chain
> +        *validationChain = CERT_NewCertList ();
> +        if (*validationChain) {
> +          rv = CERT_AddCertToListTail(*validationChain,  CERT_DupCertificate(cert));

whitespace

@@ +392,5 @@
> +        if (*validationChain) {
> +          rv = CERT_AddCertToListTail(*validationChain,  CERT_DupCertificate(cert));
> +          if (rv != SECSuccess) {
> +            CERT_DestroyCertList(*validationChain);
> +            *validationChain=nullptr;

whitespace.

::: security/manager/ssl/src/CertVerifier.h
@@ +20,5 @@
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CertVerifier)
> +
> +  typedef unsigned int Flags;
> +  static const Flags FLAG_LOCAL_ONLY;

Add a comment that says that this is ignored in classic (non-libpkix) mode.
Attachment #712583 - Flags: review?(bsmith)
Comment on attachment 712583 [details] [diff] [review]
part 1: centralized verify (v3)

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

::: security/manager/ssl/src/nsCERTValInParamWrapper.cpp
@@ +365,5 @@
> +pkix_done:
> +  if (validationChain) {
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: validation chain requested\n"));
> +    ScopedCERTCertificate issuerCert(cvout[validationChainLocation+1].value.pointer.cert);
> +

RE: #1: Never mind, I see that we don't ask libpkix to output a trust anchor unless validationChain is not null.

RE #2: For the same reason, I can see why you use validationChainLocation+1. If you don't want to add trustAnchorLocation, at least add a comment saying where you configure cvout that says that the cert_po_trustAnchor must always be immediately after validationChainLocation.
<bsmith> relyea rsleevi: I am curious about cert_po_trustAnchor and cert_po_certList

<bsmith> In particular, what should they be set to on a successful validation of a trust anchor certificate?

<rsleevi__> wan-teh 'fixed' that some time ago
<rsleevi__> let me see what the answer was
<rsleevi__> bsmith: https://code.google.com/p/chromium/issues/detail?id=115978
<rsleevi__> NSS bug was https://bugzilla.mozilla.org/show_bug.cgi?id=726134
<rsleevi__> bsmith: So if you verified verisign, you'd get a null cert_po_trustAnchor

<bsmith> So, there should be no situation in which a NULL or empty cert_po_certList is returned if CERT_PKIXVerifyCert returns SECSuccess, right?
* Notify: keeler is online (irc.mozilla.org).

<rsleevi__> That is, in Chromium code, we certainly operate on that assumption
<rsleevi__> looks like we do a null check
<rsleevi__> just later on
<rsleevi__> but I think that's just my usual paranoia
<rsleevi__> I don't think it should return null
<rsleevi__> we'd be seeing HTTPS failures if it did, at least in our code
<rsleevi__> so you should be good
<rsleevi__> other than that, you'd likely have to root around the sources

So, we should return SECFailure (with an appropriate PR_SetError() if CERT_PKIXVerifyCert fails to return a cert chain. And, we should add a MOZ_NOT_REACHED for that condition, so that we get a crash/stacktrace in debug builds to isolate any libpkix bug that causes that condition to arise.

This should considerably simplify the logic, I think.
Attached patch test (for ev only) (obsolete) — Splinter Review
Comment on attachment 712583 [details] [diff] [review]
part 1: centralized verify (v3)

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

Fixed expect for the MOZ_NOT_REACHED

::: security/manager/ssl/src/nsCERTValInParamWrapper.cpp
@@ +265,5 @@
> +
> +    if (validationChain && *validationChain) {
> +      //There SHOULD not be a validation chain on failure, asserion here for
> +      // the debug builds AND a fallback for production builds
> +      MOZ_ASSERT(false,

This is the only thing that bothers me. I looks like this becomes undefined un production builds as it calls builtin_not_reaches which is implementation specific,
Attached patch test (for ev only) (obsolete) — Splinter Review
Attachment #725574 - Attachment is obsolete: true
Attached patch part 1: centralized verify (v4) (obsolete) — Splinter Review
Attachment #712583 - Attachment is obsolete: true
Attachment #709748 - Attachment is obsolete: true
Attached patch part 3: remove verifycertnow (obsolete) — Splinter Review
Attachment #709750 - Attachment is obsolete: true
Attachment #725629 - Flags: review?(bsmith)
Attachment #725631 - Flags: review?(bsmith)
Comment on attachment 725629 [details] [diff] [review]
test (for ev only)

I think keeler should do the first pass on this one.
Attachment #725629 - Flags: review?(dkeeler)
Comment on attachment 725629 [details] [diff] [review]
test (for ev only)

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

I have some questions about the debug/non-debug behavior of this patch. Also, some nit-picky formatting issues in the mochitest.

::: security/manager/ssl/src/nsIdentityChecking.cpp
@@ +1078,5 @@
>  
>      entry.cert = CERT_FindCertByIssuerAndSN(nullptr, &ias);
> +
> +#ifdef DEBUG
> +    // The debug CA info is at position 0, and is NOT on the NSS root db

Should you temporarily add it to the db for the duration of the test? Also, I suggest doing something more like this:

#ifdef DEBUG
if (iEV != 0) {
#endif
NS_ASSERTION(entry.cert, "Could not find EV root in NSS storage");
#ifdef DEBUG
}
#endif

Well, I guess they're about the same in terms of readability, but I thought it might make it more clear why this was here.
Also, it doesn't look like iEV is defined in this function.

::: security/manager/ssl/tests/mochitest/bugs/Makefile.in
@@ +15,5 @@
>          test_bug480509.html \
>          test_bug483440.html \
>          test_bug484111.html \
> +	test_ev_validation.html \
> +	test_ev_validation_child.html \

I think we want spaces, not tabs for these

::: security/manager/ssl/tests/mochitest/bugs/test_ev_validation.html
@@ +23,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +
> +  var wnd = window.open("https://ev-valid.example.com/tests/security/ssl/bugs/test_ev_validation_child.html");
> +  window.addEventListener("message", function(event) {
> +     if(event.origin == "https://ev-valid.example.com"){

Here and elsewhere I think there should be some more spaces - for instance, after "if" and before "{"

@@ +28,5 @@
> +       if(SpecialPowers.isDebugBuild){
> +         is(event.data, "EV", "Child was EV valid.");
> +       }
> +       else{
> +         is(event.data, "secure", "Child was secure only.");

I'm not sure I understand why we can't also test EV on non-debug builds. I mean, I see that you've special-cased debug builds in nsIdentityChecking.cpp, but why is that even necessary?

::: security/manager/ssl/tests/mochitest/bugs/test_ev_validation_child.html
@@ +4,5 @@
> + <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> + <script type="text/javascript">
> +
> + function finish(state) {
> +   try{

I think the style is usually to have a space before any "{"

@@ +11,5 @@
> +   }
> +   catch(e){
> +   }
> +   ok(true,"posted message");
> +   SimpleTest.finish();

If the try block succeeds, won't this never call SimleTest.finish? Also, maybe there should be some sort of failure message in the catch block.

@@ +16,5 @@
> + }
> +
> + function onWindowLoad()
> + {
> + var ui = SpecialPowers.wrap(window)

I would probably indent this

@@ +28,5 @@
> +   var isBroken = ui &&
> +     (ui.state & SpecialPowers.Ci.nsIWebProgressListener.STATE_IS_BROKEN);
> +   var isEV = ui &&
> +     (ui.state & SpecialPowers.Ci.nsIWebProgressListener.STATE_IDENTITY_EV_TOPLEVEL);
> + 

whitespace

@@ +46,5 @@
> +
> + </script>
> +</head>
> +
> +<body  onload="onWindowLoad()">

There's two spaces here between "body" and "onload"
Attachment #725629 - Flags: review?(dkeeler) → review-
Comment on attachment 725629 [details] [diff] [review]
test (for ev only)

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

::: security/manager/ssl/src/nsIdentityChecking.cpp
@@ +1078,5 @@
>  
>      entry.cert = CERT_FindCertByIssuerAndSN(nullptr, &ias);
> +
> +#ifdef DEBUG
> +    // The debug CA info is at position 0, and is NOT on the NSS root db

I had hoped that I did not need to add this condition. The cert is added as part of the testing certs, but it seems like the addition is late or the pp-printout is not correct and the added cert is not found at this time.

Regarding the #ifdefs I had it originally as you wrote it, but I tought it was mot clear to have it in the coditions. I you think is better with two ifdefs I will change it.

The iEV is declared in the body of the for loop (line 1065 of the modified file)

::: security/manager/ssl/tests/mochitest/bugs/test_ev_validation.html
@@ +23,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +
> +  var wnd = window.open("https://ev-valid.example.com/tests/security/ssl/bugs/test_ev_validation_child.html");
> +  window.addEventListener("message", function(event) {
> +     if(event.origin == "https://ev-valid.example.com"){

Agreed, that is why I asked you before brian :). Thanks

@@ +28,5 @@
> +       if(SpecialPowers.isDebugBuild){
> +         is(event.data, "EV", "Child was EV valid.");
> +       }
> +       else{
> +         is(event.data, "secure", "Child was secure only.");

Even tough users now plenty of rope to hang themselves via changes to their prefs. I think we should not give them more rope than what is already there. Users already can be SSL-MITM by importing custom CAs (but no EV inidicadors are possible), with this patch EV MIM will be possible with two conditions 1. Use of a debug build AND importing of the testing CA (which is public). I think the risk of adding EV indicators to MITM in optmized builds is somthing that is not worth it. In the worse case, the EV indicator Positive presence would not be catched by the automated testing in optimized builds, which is a minor coverage loss.

::: security/manager/ssl/tests/mochitest/bugs/test_ev_validation_child.html
@@ +11,5 @@
> +   }
> +   catch(e){
> +   }
> +   ok(true,"posted message");
> +   SimpleTest.finish();

Yes, the opener should close this window. I changed the conditions, but for this part I am more than willing to get more suggestions.
Here is the issue:
1. I need this html file to use specialpowers (to access the ui indicators)
2. If I add SimpleTest.js I not only get special powers but this is expected to be a test.
3. I therefore need a simple -> ignore this file when running stand-alon clause ( when window.opener is not defined).

(another option would be to put this condition at the begginig of the JS onWindowload.....)

@@ +46,5 @@
> +
> + </script>
> +</head>
> +
> +<body  onload="onWindowLoad()">

ack
Attachment #725629 - Attachment is obsolete: true
Attachment #725629 - Flags: review?(bsmith)
Attachment #734882 - Flags: review?
Attachment #734882 - Flags: review? → review?(bsmith)
Comment on attachment 725631 [details] [diff] [review]
part 1: centralized verify (v4)

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

Looks very good now. Please address the comments below with a new patch that builds on top of this one (i.e. don't qrefresh your changes into this patch), so that I can more easily review the inter-diff. (The bugzilla automatic inter-diff tool is almost totally broken, unfortunately.)

Tomorrow I will review the tests. We need to land this and the tests together.

::: security/manager/ssl/src/nsCERTValInParamWrapper.cpp
@@ +122,5 @@
> +                         const SECCertificateUsage usage,
> +                         const PRTime time,
> +                         nsIInterfaceRequestor * pinArg,
> +                         const Flags flags,
> +                         /*optional out*/ CERTCertList **validationChain,

Should this be a ScopedCERTCertList * instead? It seems like that would be more convenient for callers. (See below.)

@@ +149,5 @@
> +      if (evPolicy != SEC_OID_UNKNOWN) {
> +        trustAnchors = getRootsForOid(evPolicy);
> +      }
> +      if (!trustAnchors) {
> +        return SECFailure;

Do you think that we should fail here, or should we just keep going without doing any EV validation?

@@ +217,5 @@
> +
> +  if (evPolicy != SEC_OID_UNKNOWN) {
> +    // EV setup!
> +    // This flags are not quite correct, but it is what we have now, so keeping
> +    // them identical for bug landing purposes. Should be fixed later!

I suggest removing this comment. It just confuses the reader and makes him worry about what, exactly, is wrong. If there is a specific problem then mention it (along with the follow-up bug number) in an XXX comment here.

@@ +220,5 @@
> +    // This flags are not quite correct, but it is what we have now, so keeping
> +    // them identical for bug landing purposes. Should be fixed later!
> +    uint64_t revMethodFlags =
> +      CERT_REV_M_TEST_USING_THIS_METHOD
> +      | (mOCSPDownloadEnabled ? CERT_REV_M_ALLOW_NETWORK_FETCHING

We should respect localOnly here too, right?

@@ +333,5 @@
>      | CERT_REV_M_SKIP_TEST_ON_MISSING_SOURCE
>  
> +    // if ocsp is required stop on lack of freshness
> +    | (mOCSPStrict ?
> +       CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO : CERT_REV_M_IGNORE_MISSING_FRESH_INFO)

Nit: move this up above "// if app has a default OCSP responder..." to minimize the diff.

@@ +340,5 @@
>      | CERT_REV_M_STOP_TESTING_ON_FRESH_INFO
> +
> +    // ocsp enabled controls network fetching, too
> +    | ((mOCSPDownloadEnabled && !localOnly) ?
> +        CERT_REV_M_ALLOW_NETWORK_FETCHING : CERT_REV_M_FORBID_NETWORK_FETCHING)

Nit: move this below "// use OCSP" to minimize the diff.

@@ +369,5 @@
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert: validation chain requested\n"));
> +    ScopedCERTCertificate trustAnchor(cvout[validationTrustAnchorLocation].value.pointer.cert);
> +
> +    if (rv == SECSuccess) {
> +      if (! cvout[validationChainLocation].value.pointer.chain) {

Need PR_SetError() here.

@@ +382,5 @@
> +        if (!CERT_CompareCerts(trustAnchor, cert)) {
> +          PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert:  adding issuer to tail for display\n"));
> +          // note: rv is reused to catch errors on cert creation!
> +          rv = CERT_AddCertToListTail(*validationChain,
> +                                      CERT_DupCertificate(trustAnchor));

You leak the copy of trustAnchor you created here, if the result != SECSuccess.

@@ +395,5 @@
> +      if (cvout[validationChainLocation].value.pointer.chain) {
> +        CERT_DestroyCertList(cvout[validationChainLocation].value.pointer.chain);
> +      }
> +    }
> +  }

blank line here.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +486,5 @@
>    if (!log_arena) {
>      NS_ERROR("PORT_NewArena failed");
>      return nullptr; // PORT_NewArena set error code
>    }
> +  verify_log = PORT_ArenaZNew(log_arena, CERTVerifyLog);

Nit: Combine the declaration with the initialization here.

@@ +495,5 @@
>    CERTVerifyLogContentsCleaner verify_log_cleaner(verify_log);
>    verify_log->arena = log_arena;
>  
> +  srv = certVerifier->VerifyCert(cert, certificateUsageSSLServer, now,
> +                                   infoObject, 0, nullptr, nullptr, verify_log);

Nit: indention of second line is off.

@@ +520,5 @@
>      collected_errors |= nsICertOverrideService::ERROR_MISMATCH;
>      errorCodeMismatch = SSL_ERROR_BAD_CERT_DOMAIN;
>    }
>  
> +  if (verify_log) {

This condition is always true, so you can revert this chunk back to what it was before.

@@ +559,3 @@
>      }
> +  } else {
> +    // XXX set errorCodeTrust, errorCodeMismatch, errorCodeExpired, collected_errors

Do not need to worry about this since verifyLog is always true.

@@ +872,5 @@
>        }
>      }
>    }
>  
> +  CERTCertList *verifyCertChain = nullptr;

You seem to be ignoring this value. I think you you intended to replace the call to CERT_GetCertChainFromCert below with the usage of this. Seems like you should just rename this to certList and then remove the line that calls CERT_GetCertChainFromCert below.

Also, shouldn't this be a ScopedCERTCertList instead of a bare pointer? That way we don't have to worry about it leaking. This is why I think that VerifyChain should take a ScopedCERTCertList* instead of a CERTCertList**.

@@ +887,5 @@
>    RefPtr<nsSSLStatus> status(infoObject->SSLStatus());
>    RefPtr<nsNSSCertificate> nsc;
>  
>    if (!status || !status->mServerCert) {
> +    if( rv == SECSuccess ){

nit: whitespace.

@@ +897,4 @@
>    }
>  
>    ScopedCERTCertList certList(CERT_GetCertChainFromCert(cert, PR_Now(),
>                                                          certUsageSSLCA));

just delete this now?

::: security/manager/ssl/src/nsIdentityChecking.cpp
@@ +1038,5 @@
>  #endif
>    return certList;
>  }
>  
> +}}

} } // namespace mozilla::psm.

@@ +1185,5 @@
>  
>    return SECFailure;
>  }
>  
> +}}

} } // namespace mozilla::psm

@@ +1246,4 @@
>    }
> +  if (resultOidTag != SEC_OID_UNKNOWN) {
> +    validEV = true;
> +  }

What is the purpose of isApprovedForEV and why don't we need to call it? It seems to be redundant with the getRootsForOid().

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +578,5 @@
>      ScopedCERTCertificateList certChain;
>  
>      if (!alert_and_skip) {
> +      certChain = CERT_CertChainFromCert(node->cert, certUsageEmailRecipient,
> +                                         false);

Seems like this should be using the certchain returned from VerifyCert instead of computing a new one.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1993,5 @@
>    *aErrorCode = 0;
>    *aPrincipal = nullptr;
>  
>    nsNSSShutDownPreventionLock locker;
> +  ScopedSEC_PKCS7ContentInfo p7_info;

unrelated change?

@@ -2026,5 @@
>                                  DecryptionAllowedCallback);
>  
> -  if (!p7_info) {
> -    return NS_ERROR_FAILURE;
> -  }

unrelated (and wrong?) change?
Attachment #725631 - Flags: review?(bsmith) → review-
Comment on attachment 734882 [details] [diff] [review]
test (for ev only) v2

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

Camilo, one thing that isn't clear off the bat about this testing patch:

Does this patch test the case where the EE cert has an EV OID (e.g. VeriSign's EV OID) but the root isn't trusted for that OID? I.e. does this test the fallback from EV to DV, as opposed to just the path that skips over EV validation to go straight to DV? It would be great to test that.
(In reply to Brian Smith (:bsmith) from comment #91)
> Comment on attachment 734882 [details] [diff] [review]
> test (for ev only) v2
> 
> Review of attachment 734882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Camilo, one thing that isn't clear off the bat about this testing patch:
> 
> Does this patch test the case where the EE cert has an EV OID (e.g.
> VeriSign's EV OID) but the root isn't trusted for that OID? I.e. does this
> test the fallback from EV to DV, as opposed to just the path that skips over
> EV validation to go straight to DV? It would be great to test that.

Yes it does. The ev-invalid cert is signed by the non-ev cert (Issuer: "CN=Temporary Certificate Authority,O=Mozilla Testing,OU=Profile Guided Optimization" ) with the policy of ( Policy Name: OID.1.3.6.1.4.1.13769.666.666.666.1.500.9.1 ) of the testing ev-ca ( Issuer: "E=charlatan@testing.example.com,OID.2.5.4.41=ev-test-ca,CN=intermediate3,OU=Security Engineering,O=Mozilla - EV debug test CA,L=Mountain View,ST=CA,C=US" )
Comment on attachment 725635 [details] [diff] [review]
part 3: remove verifycertnow

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

Good work.

I think the plan is to remove this method, after we figure out what to do with the certificate viewer, since its design doesn't make sense. (A certificate can have more than one chain, and the chain any application should care about is the one that it received during the original validation it did, not a possibly-different one that we built later.)

r+ with comments addressed.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +829,3 @@
>    ScopedCERTCertList nssChain;
> +  SECStatus srv;
> +  nssChain = NULL ;

nullptr, whitespace.

@@ +830,5 @@
> +  SECStatus srv;
> +  nssChain = NULL ;
> +  RefPtr<CertVerifier> certVerifier(GetDefaultCertVerifier());
> +  NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED);
> +  CERTCertList *pkixNssChain = nullptr;

More evidence that CertVerifier::VerifyCert should take a ScopedCERTCertList* intstead of a CERTCertList*.

@@ +832,5 @@
> +  RefPtr<CertVerifier> certVerifier(GetDefaultCertVerifier());
> +  NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED);
> +  CERTCertList *pkixNssChain = nullptr;
> +
> +  // We want to test all usages, starting with server.

Comment could be improved. We start with "server" because most of the time Firefox users care about web server certs.

@@ +835,5 @@
> +
> +  // We want to test all usages, starting with server.
> +  srv = certVerifier->VerifyCert(mCert,
> +                                 certificateUsageSSLServer, PR_Now(),
> +                                 nullptr, /*XXX fixme*/

I guess we need a bug about passing in a pinarg to many cert validations that are currently missing one. (The pinarg is how we prompt the user for the master password if we need it, which we shouldn't normally.)

@@ +840,5 @@
> +                                 CertVerifier::FLAG_LOCAL_ONLY,
> +                                 &pkixNssChain);
> +  for (int usage = certificateUsageSSLClient;
> +       usage < certificateUsageAnyCA && !pkixNssChain;
> +       usage = usage<<1) {

whitespace around "<<"

@@ +841,5 @@
> +                                 &pkixNssChain);
> +  for (int usage = certificateUsageSSLClient;
> +       usage < certificateUsageAnyCA && !pkixNssChain;
> +       usage = usage<<1) {
> +    if ( usage == certificateUsageSSLServer) {

whitespace.

@@ +853,5 @@
> +                                   &pkixNssChain);
> +  }
> +
> +  if (!pkixNssChain) {
> +    // do fallback, for compat, should be removed later

I think the comment could be improved. The reason we're doing this is so that we can show as much of a chain to the user as possible, in the case where there was a problem with the cert chain (e.g. untrusted root).

::: security/manager/ssl/src/nsUsageArrayHelper.cpp
@@ +197,5 @@
>  
>    if (outArraySize < max_returned_out_array_size)
>      return NS_ERROR_FAILURE;
>  
> +  if (!nsNSSComponent::globalConstFlagUsePKIXVerification && localOnly) {

We should have a follow-up bug to remove this chunk after we've switched off of classic.

@@ +250,5 @@
>    result = check(result, suffix, certVerifier,
>                   certificateUsageAnyCA, now, flags, count, outUsages);
>  #endif
>  
> +  if (!nsNSSComponent::globalConstFlagUsePKIXVerification && localOnly) {

Also mark this with the same XXX: Bug XXXXXX comment.
Attachment #725635 - Flags: review+
Attachment #725634 - Attachment is obsolete: true
Attachment #735457 - Flags: review?(bsmith)
Attachment #735457 - Attachment is obsolete: true
Attachment #735457 - Flags: review?(bsmith)
Attachment #736560 - Flags: review?(bsmith)
Comment on attachment 736560 [details] [diff] [review]
part 2: getting chain from verify cert + cleanup (v2)

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

Look out for trailing whitespace.

::: security/manager/ssl/src/CertVerifier.cpp
@@ +382,5 @@
>          // that case we do not add the cert to the chain.
>          if (!CERT_CompareCerts(trustAnchor, cert)) {
>            PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert:  adding issuer to tail for display\n"));
>            // note: rv is reused to catch errors on cert creation!
> +          CERTCertificate *tempCert = CERT_DupCertificate(trustAnchor);

nit: ScopedCERTCertificate takes care of this automatically.

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +581,5 @@
>        DisplayCertificateAlert(ctx, "NotImportingUnverifiedCert", certToShow);
>        continue;
>      }
>  
> +    ScopedCERTCertList certChain(verifyCertChain);

Please do this as early as possible (i.e. right after calling VerifyCert) to reduce the odds that we will leak verifyCertChain. In fact, don't we leak just three lines above with the "continue;"?

Also, AFAICT we don't need the alert_and_skip variable anymore.

@@ +593,3 @@
>      /*
> +     * VerifyCert returns a CERTCertList, import expects an array of
> +     * SECItem pointers. Create the SECItem Pointers from the node's 

trailing whitespace.

@@ +774,5 @@
>        DisplayCertificateAlert(ctx, "NotImportingUnverifiedCert", certToShow);
>        continue;
>      }
>  
> +    ScopedCERTCertList certChain(verifyCertChain);

Same applies here. Seems to leak verifyCertChain on the "continue;" line. and probably don't need alert_and_skip any more.

I really think that we should just change the signature of VerifyCert to take a ScopedCERTCertList*. This would have avoided both of these bugs. Please do this.

@@ +776,5 @@
>      }
>  
> +    ScopedCERTCertList certChain(verifyCertChain);
> +
> +    int chainLen=0;

whitespace.

@@ +804,3 @@
>                              rawArray,  nullptr, true, true, nullptr);
>  
>      PORT_Free(rawArray);

Please factor this duplicate count/create array/import cert/destroy array logic out into a function.
Comment on attachment 736560 [details] [diff] [review]
part 2: getting chain from verify cert + cleanup (v2)

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

r+ with changes requested in the previous comment.
Attachment #736560 - Flags: review?(bsmith) → review+
Comment on attachment 734882 [details] [diff] [review]
test (for ev only) v2

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

If possible, please try to write some notes about how you created the test certs. In the future, include a script that can re-create the new certs that are added along with the patch.
Attachment #734882 - Flags: review?(bsmith) → review+
Blocks: 862091
Because Android likes to be contrary, rather than having that crash, which it has intermittently anyway, it did https://tbpl.mozilla.org/php/getParsedLog.php?id=21837341&tree=Mozilla-Inbound instead, timing out in test_ev_validation.html in mochitest-8.
Attachment #725631 - Attachment is obsolete: true
Attachment #725635 - Attachment is obsolete: true
Attachment #736560 - Attachment is obsolete: true
Comment on attachment 742615 [details] [diff] [review]
silly patch for solve errors in some debug builds

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

::: security/manager/ssl/src/CertVerifier.cpp
@@ +181,5 @@
> +    //++i;
> +    validationTrustAnchorLocation = i + 1;
> +    cvout[validationTrustAnchorLocation].type = cert_po_trustAnchor;
> +    cvout[validationTrustAnchorLocation].value.pointer.chain = nullptr;
> +    i = i + 2;

This change isn't necessary, right?

@@ +255,4 @@
>      MOZ_ASSERT(trustAnchors);
>      cvin[i].type = cert_pi_trustAnchors;
>      cvin[i].value.pointer.chain = trustAnchors;
> +    i++;

Ditto: I think these changes are not necessary.

@@ +386,5 @@
>          // that case we do not add the cert to the chain.
>          if (!CERT_CompareCerts(trustAnchor, cert)) {
>            PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert:  adding issuer to tail for display\n"));
>            // note: rv is reused to catch errors on cert creation!
> +          CERTCertificate *tempCert = CERT_DupCertificate(trustAnchor);

Shouldn't this be:

ScopedCERTCertificate tempCert(CERT_DupCertificate(trustAnchor));
rv = CERT_AddCertToListTail(*validationChain, tempCert);
if (rv == SECSuccess) {
  tempCert.forget();
} else {
  ...
Attachment #742615 - Flags: review-
Comment on attachment 742615 [details] [diff] [review]
silly patch for solve errors in some debug builds

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

Yes this changes should not exist.

::: security/manager/ssl/src/CertVerifier.cpp
@@ +181,5 @@
> +    //++i;
> +    validationTrustAnchorLocation = i + 1;
> +    cvout[validationTrustAnchorLocation].type = cert_po_trustAnchor;
> +    cvout[validationTrustAnchorLocation].value.pointer.chain = nullptr;
> +    i = i + 2;

Yes, they should not be needed but I was getting weird execution paths on debug builds without disabling optimizations on Ubuntu 12.04 64 bit (gcc 4.6). That is why this bundle of changes is called silly patches.

To reproduce weird behavior:
1. open ff,
2. go to addons.mozilla.com (ev) site. 
3. Close ff
4. WTH (bad termination).

via gdb I noticed that the i was being incremented before line 183!

@@ +386,5 @@
>          // that case we do not add the cert to the chain.
>          if (!CERT_CompareCerts(trustAnchor, cert)) {
>            PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert:  adding issuer to tail for display\n"));
>            // note: rv is reused to catch errors on cert creation!
> +          CERTCertificate *tempCert = CERT_DupCertificate(trustAnchor);

And this I was getting bad cleanups, so I deciced to revert to the version before we used ScopedCert and yay clean shutdown.
Comment on attachment 742615 [details] [diff] [review]
silly patch for solve errors in some debug builds

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

Yes this changes should not exist.

::: security/manager/ssl/src/CertVerifier.cpp
@@ +181,5 @@
> +    //++i;
> +    validationTrustAnchorLocation = i + 1;
> +    cvout[validationTrustAnchorLocation].type = cert_po_trustAnchor;
> +    cvout[validationTrustAnchorLocation].value.pointer.chain = nullptr;
> +    i = i + 2;

Yes, they should not be needed but I was getting weird execution paths on debug builds without disabling optimizations on Ubuntu 12.04 64 bit (gcc 4.6). That is why this bundle of changes is called silly patches.

To reproduce weird behavior:
1. open ff,
2. go to addons.mozilla.com (ev) site. 
3. Close ff
4. WTH (bad termination).

via gdb I noticed that the i was being incremented before line 183!

@@ +386,5 @@
>          // that case we do not add the cert to the chain.
>          if (!CERT_CompareCerts(trustAnchor, cert)) {
>            PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("VerifyCert:  adding issuer to tail for display\n"));
>            // note: rv is reused to catch errors on cert creation!
> +          CERTCertificate *tempCert = CERT_DupCertificate(trustAnchor);

And this I was getting bad cleanups, so I deciced to revert to the version before we used ScopedCert and yay clean shutdown.
Attachment #742615 - Attachment is obsolete: true
Attachment #742873 - Flags: review?(cviecco)
Comment on attachment 742873 [details] [diff] [review]
Fix crash due to reference counting error

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

Good to know about Scoped$BAR.forget()
Attachment #742873 - Flags: review?(cviecco) → review+
Looks like a success: (partial try)
https://tbpl.mozilla.org/?tree=Try&rev=8106173395b6
just build try: removal of one extra declaration (no logic changes)
https://tbpl.mozilla.org/?tree=Try&rev=1798dd258e98
https://hg.mozilla.org/mozilla-central/rev/e088156c89d0
https://hg.mozilla.org/mozilla-central/rev/4fc7f15531a2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: CVE-2013-6673
You need to log in before you can comment on or make changes to this bug.