Address alv's comments on codereview.chromium.org/119026....

net/base/x509_certificate_nss.cc

 // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/x509_certificate.h"
 
 // Work around https://bugzilla.mozilla.org/show_bug.cgi?id=455424
 // until NSS 3.12.2 comes out and we update to it.
 #define Lock FOO_NSS_Lock
 #include <cert.h>
@@ skipping 82 matching lines @@
       }
     }
   }
 
  private:
   CERTValOutParam* cvout_;
 
   DISALLOW_COPY_AND_ASSIGN(ScopedCERTValOutParam);
 };
 
+class ScopedCERTCertificatePolicies {

[wtc, 2009/08/13 00:35:33]

Nit: move this up, under ScopedCERTCertList, because these
two are more related (in NSS).

+ public:
+  explicit ScopedCERTCertificatePolicies(CERTCertificatePolicies* policies)
+      : policies_(policies) {}
+
+  ~ScopedCERTCertificatePolicies() {
+    if (policies_)
+      CERT_DestroyCertificatePoliciesExtension(policies_);
+  }
+
+ private:
+  CERTCertificatePolicies* policies_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedCERTCertificatePolicies);
+};
+
+
 // Map PORT_GetError() return values to our network error codes.
 int MapSecurityError(int err) {
   switch (err) {
     case SEC_ERROR_INVALID_TIME:
     case SEC_ERROR_EXPIRED_CERTIFICATE:
       return ERR_CERT_DATE_INVALID;
     case SEC_ERROR_UNKNOWN_ISSUER:
     case SEC_ERROR_UNTRUSTED_ISSUER:
     case SEC_ERROR_CA_CERT_INVALID:
     case SEC_ERROR_UNTRUSTED_CERT:
@@ skipping 211 matching lines @@
 // If policy_oids is not NULL and num_policy_oids is positive, policies
 // are also checked.
 // Caller must initialize cvout before calling this function.
 SECStatus PKIXVerifyCert(X509Certificate::OSCertHandle cert_handle,
                          const SECOidTag* policy_oids,
                          int num_policy_oids,
                          CERTValOutParam* cvout) {
   PRUint64 revocation_method_flags =
       CERT_REV_M_TEST_USING_THIS_METHOD |
       CERT_REV_M_ALLOW_NETWORK_FETCHING |
-      CERT_REV_M_ALLOW_IMPLICIT_DEFAULT_SOURCE |
-      CERT_REV_M_STOP_TESTING_ON_FRESH_INFO;
+      CERT_REV_M_IGNORE_IMPLICIT_DEFAULT_SOURCE |
+      CERT_REV_M_STOP_TESTING_ON_FRESH_INFO |
+      CERT_REV_M_IGNORE_MISSING_FRESH_INFO;

[wtc, 2009/08/13 01:20:58]

Nit: please list CERT_REV_M_IGNORE_MISSING_FRESH_INFO
before CERT_REV_M_STOP_TESTING_ON_FRESH_INFO, to match
the order in which these macros are defined in the NSS
header certt.h.  This makes it easier to verify that we
didn't miss any macros.

   PRUint64 revocation_method_independent_flags =
       CERT_REV_MI_TEST_ALL_LOCAL_INFORMATION_FIRST;
   if (policy_oids && num_policy_oids > 0) {

[wtc, 2009/08/13 00:35:33]

Alexei, I had to add this if statement so that we're
using different flags for normal and EV verifications.
Please verify that these two sets of flags look sane.

     // EV verification requires revocation checking.  Consider the certificate
     // revoked if we don't have revocation info.
     // TODO(wtc): Add a bool parameter to expressly specify we're doing EV
     // verification or we want strict revocation flags.
     revocation_method_flags |= CERT_REV_M_REQUIRE_INFO_ON_MISSING_SOURCE;
     revocation_method_independent_flags |=
         CERT_REV_MI_REQUIRE_SOME_FRESH_INFO_AVAILABLE;
   } else {
     revocation_method_flags |= CERT_REV_M_SKIP_TEST_ON_MISSING_SOURCE;
     revocation_method_independent_flags |=
         CERT_REV_MI_NO_OVERALL_INFO_REQUIREMENT;
   }
   PRUint64 method_flags[2];
   method_flags[cert_revocation_method_crl] = revocation_method_flags;
   method_flags[cert_revocation_method_ocsp] = revocation_method_flags;
 
   // TODO(ukai): need to find out if we need to call OCSP-related NSS functions,

[wtc, 2009/08/13 00:35:33]

Please remove this TODO comment.  Alexei told me that since
we use CERT_PKIXVerifyCert (rather than the old
CERT_VerifyCert function and its variants), we don't need
to call these functions.

   // CERT_EnableOCSPChecking, CERT_DisableOCSPDefaultResponder and
   // CERT_SetOCSPFailureMode.
   CERTRevocationMethodIndex preferred_revocation_methods[1];
   preferred_revocation_methods[0] = cert_revocation_method_ocsp;
 
   CERTRevocationFlags revocation_flags;
   revocation_flags.leafTests.number_of_defined_methods =
       arraysize(method_flags);
   revocation_flags.leafTests.cert_rev_flags_per_method = method_flags;
   revocation_flags.leafTests.number_of_preferred_methods =
@@ skipping 39 matching lines @@
   if (rv != SECSuccess) {
     LOG(ERROR) << "Cert has no policies extension.";
     return false;
   }
   CERTCertificatePolicies* policies =
       CERT_DecodeCertificatePoliciesExtension(&policy_ext);
   if (!policies) {
     LOG(ERROR) << "Failed to decode certificate policy.";
     return false;
   }
+  ScopedCERTCertificatePolicies scoped_policies(policies);

[alv, 2009/08/12 18:40:42]

It will work, but it is too fancy and increases size of the code. Since there is
no need to use "goto" I would use a "tag_found" boolean and return it value
after destroying the policies object. You don't even need to have "if" since you
have already made a check at line 429.

[wtc, 2009/08/13 00:38:22]

I, a C programmer, agree with the "too fancy" opinion, but
this is a C++ project, so we need to use the techniques
familiar to C++ programmers.

   CERTPolicyInfo** policy_infos = policies->policyInfos;
   while (*policy_infos != NULL) {
     CERTPolicyInfo* policy_info = *policy_infos++;
     SECOidTag oid_tag = policy_info->oid;
     if (oid_tag == SEC_OID_UNKNOWN)
       continue;
     if (oid_tag == ev_policy_tag)
       return true;
   }
   LOG(ERROR) << "No EV Policy Tag";
@@ skipping 90 matching lines @@
   if (IsCertStatusError(verify_result->cert_status))
     return MapCertStatusToNetError(verify_result->cert_status);
 
   if ((flags & VERIFY_EV_CERT) && VerifyEV())
     verify_result->cert_status |= CERT_STATUS_IS_EV;
   return OK;
 }
 
 // Studied Mozilla's code (esp. security/manager/ssl/src/nsIdentityChecking.cpp
 // and nsNSSCertHelper.cpp) to learn how to verify EV certificate.
-// TODO(wtc): We may be able to request cert_po_policyOID and just
-// check if any of the returned policies is the EV policy of the trust anchor.
-// Another possible optimization is that we get the trust anchor from
+// TODO(wtc): Possible optimization is that we get the trust anchor from
 // the first PKIXVerifyCert call.  We look up the EV policy for the trust
 // anchor.  If the trust anchor has no EV policy, we know the cert isn't EV.
 // Otherwise, we pass just that EV policy (as opposed to all the EV policies)
 // to the second PKIXVerifyCert call.
 bool X509Certificate::VerifyEV() const {
   net::EVRootCAMetadata* metadata = net::EVRootCAMetadata::GetInstance();
 
   CERTValOutParam cvout[3];
   int cvout_index = 0;
   cvout[cvout_index].type = cert_po_trustAnchor;
@@ skipping 53 matching lines @@
   DCHECK(0 != cert->derCert.len);
 
   SECStatus rv = HASH_HashBuf(HASH_AlgSHA1, sha1.data,
                               cert->derCert.data, cert->derCert.len);
   DCHECK(rv == SECSuccess);
 
   return sha1;
 }
 
 }  // namespace net