Check whether or not a certificate is self-signed.

Check whether or not a certificate is self-signed.

This will enable telemetry, and possibly heuristics for gauging the severity
of HTTPS authentication failures.

BUG=392310
Committed: https://crrev.com/32b3d8ab60476036ea7d687f43f23e0b63796a5a
Cr-Commit-Position: refs/heads/master@{#301912}

[palmer, 2014-10-07 18:51:43]

palmer@chromium.org changed reviewers:
+ felt@chromium.org, radhikabhar@chromium.org, rsleevi@chromium.org,
willchan@chromium.org

[palmer, 2014-10-07 18:51:44]

PTAL, thanks.

Currently the only caller is the unit test, but in the future (soon?), we should
add UMA counters and maybe can use it to modulate HTTPS warnings in certain
(very limited) cases.

[palmer, 2014-10-07 18:52:20]

palmer@chromium.org changed reviewers:
- willchan@chromium.org

[willchan no longer on Chromium, 2014-10-07 19:27:25]

Chris, did you add me because you needed a quick review? Otherwise,
I'd wait for Ryan to get back from the networking summit and have him
review this. Cheers.

On Tue, Oct 7, 2014 at 11:51 AM,  <palmer@chromium.org> wrote:
> Reviewers: Ryan Sleevi, willchan OOO until 03-22-15, felt, radhikabhar,
>
> Message:
> PTAL, thanks.
>
> Currently the only caller is the unit test, but in the future (soon?), we
> should
> add UMA counters and maybe can use it to modulate HTTPS warnings in certain
> (very limited) cases.
>
> Description:
> Check whether or not a certificate is self-signed.
>
> This will enable telemetry, and possibly heuristics for gauging the severity
> of HTTPS authentication failures.
>
> Currently, there is no implementation for iOS. That is OK because this code
> is not critical for security. If it ever becomes critical, obviously we need
> to implement for iOS.
>
> BUG=392310
>
> Please review this at https://codereview.chromium.org/634033002/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+90, -0 lines):
>   M net/cert/x509_certificate.h
>   M net/cert/x509_certificate_ios.cc
>   M net/cert/x509_certificate_mac.cc
>   M net/cert/x509_certificate_nss.cc
>   M net/cert/x509_certificate_openssl.cc
>   M net/cert/x509_certificate_unittest.cc
>   M net/cert/x509_certificate_win.cc
>
>
> Index: net/cert/x509_certificate.h
> diff --git a/net/cert/x509_certificate.h b/net/cert/x509_certificate.h
> index
>
97906ed0461f02c4d8705e2e3db8109484f21f1b..7b4ecbb03109ea8df0ea15be5d78876ae25c83db
> 100644
> --- a/net/cert/x509_certificate.h
> +++ b/net/cert/x509_certificate.h
> @@ -421,6 +421,9 @@ class NET_EXPORT X509Certificate
>        OSCertHandle leaf,
>        const OSCertHandles& intermediates);
>
> +  // Returns true if the certificate is self-signed.
> +  static bool IsSelfSigned(OSCertHandle cert_handle);
> +
>   private:
>    friend class base::RefCountedThreadSafe<X509Certificate>;
>    friend class TestRootCerts;  // For unit tests
> Index: net/cert/x509_certificate_ios.cc
> diff --git a/net/cert/x509_certificate_ios.cc
> b/net/cert/x509_certificate_ios.cc
> index
>
f59cb650993cb039bf64d67f794c7952a5f3e366..3a1deeadec8e8d465e532bc794dbed1ae5ac8a4e
> 100644
> --- a/net/cert/x509_certificate_ios.cc
> +++ b/net/cert/x509_certificate_ios.cc
> @@ -234,4 +234,10 @@ void X509Certificate::GetPublicKeyInfo(OSCertHandle
> cert_handle,
>    x509_util::GetPublicKeyInfo(nss_cert.cert_handle(), size_bits, type);
>  }
>
> +// static
> +bool X509Certificate::IsSelfSigned(OSCertHandle cert_handle) {
> +  // TODO(palmer): Implement this.
> +  return false;
> +}
> +
>  }  // namespace net
> Index: net/cert/x509_certificate_mac.cc
> diff --git a/net/cert/x509_certificate_mac.cc
> b/net/cert/x509_certificate_mac.cc
> index
>
716bdd5c4ec1ce4ca272b5cf2fb187b6731feeb0..174301e19cb557f2ce6019d8b9df8b2afe9fc355
> 100644
> --- a/net/cert/x509_certificate_mac.cc
> +++ b/net/cert/x509_certificate_mac.cc
> @@ -15,6 +15,7 @@
>  #include "base/mac/mac_logging.h"
>  #include "base/mac/scoped_cftyperef.h"
>  #include "base/memory/singleton.h"
> +#include "base/numerics/safe_conversions.h"
>  #include "base/pickle.h"
>  #include "base/sha1.h"
>  #include "base/strings/string_piece.h"
> @@ -513,4 +514,24 @@ void X509Certificate::GetPublicKeyInfo(OSCertHandle
> cert_handle,
>    }
>  }
>
> +// static
> +bool X509Certificate::IsSelfSigned(OSCertHandle cert_handle) {
> +  std::string der_cert;
> +  if (!GetDEREncoded(cert_handle, &der_cert))
> +    return false;
> +
> +  const unsigned char* cert_data =
> +      reinterpret_cast<const unsigned char*>(der_cert.data());
> +  int cert_data_len = checked_cast<int>(der_cert.size());
> +  ScopedX509 cert(d2i_X509(NULL, &cert_data, cert_data_len));
> +  crypto::ScopedEVP_PKEY scoped_key(X509_get_pubkey(cert_handle));
> +  if (!scoped_key)
> +    return false;
> +  DCHECK(scoped_key.get());
> +  EVP_PKEY* key = scoped_key.get();
> +
> +  // NOTE: X509_verify() returns 1 in case of success, 0 or -1 on error.
> +  return X509_verify(cert.get(), key) == 1;
> +}
> +
>  }  // namespace net
> Index: net/cert/x509_certificate_nss.cc
> diff --git a/net/cert/x509_certificate_nss.cc
> b/net/cert/x509_certificate_nss.cc
> index
>
9019625869aacf35bcf3280a4733eeb2bac5ccc6..05ca13ba4526e74b0e05581c54cc5d8333a06ac0
> 100644
> --- a/net/cert/x509_certificate_nss.cc
> +++ b/net/cert/x509_certificate_nss.cc
> @@ -266,4 +266,15 @@ void X509Certificate::GetPublicKeyInfo(OSCertHandle
> cert_handle,
>    x509_util::GetPublicKeyInfo(cert_handle, size_bits, type);
>  }
>
> +// static
> +bool X509Certificate::IsSelfSigned(OSCertHandle cert_handle) {
> +  SECKEYPublicKey* public_key = CERT_ExtractPublicKey(cert_handle);
> +  if (!public_key)
> +    return false;
> +
> +  SECStatus verified = CERT_VerifySignedDataWithPublicKey(
> +      &cert_handle->signatureWrap, public_key, NULL);
> +  return verified == SECSuccess;
> +}
> +
>  }  // namespace net
> Index: net/cert/x509_certificate_openssl.cc
> diff --git a/net/cert/x509_certificate_openssl.cc
> b/net/cert/x509_certificate_openssl.cc
> index
>
9cb0670d51c1bc56ddf87c82c8c21ea6a744e547..d5d08318600f1cd114347005bf6e575781f73dfc
> 100644
> --- a/net/cert/x509_certificate_openssl.cc
> +++ b/net/cert/x509_certificate_openssl.cc
> @@ -14,6 +14,7 @@
>  #include <openssl/x509v3.h>
>
>  #include "base/memory/singleton.h"
> +#include "base/numerics/safe_conversions.h"
>  #include "base/pickle.h"
>  #include "base/sha1.h"
>  #include "base/strings/string_number_conversions.h"
> @@ -36,6 +37,7 @@ namespace {
>
>  typedef crypto::ScopedOpenSSL<GENERAL_NAMES, GENERAL_NAMES_free>::Type
>      ScopedGENERAL_NAMES;
> +typedef crypto::ScopedOpenSSL<X509, X509_free>::Type ScopedX509;
>
>  void CreateOSCertHandlesFromPKCS7Bytes(
>      const char* data, int length,
> @@ -439,4 +441,24 @@ bool X509Certificate::IsIssuedByEncoded(
>    return false;
>  }
>
> +// static
> +bool X509Certificate::IsSelfSigned(OSCertHandle cert_handle) {
> +  std::string der_cert;
> +  if (!GetDEREncoded(cert_handle, &der_cert))
> +    return false;
> +
> +  const unsigned char* cert_data =
> +      reinterpret_cast<const unsigned char*>(der_cert.data());
> +  int cert_data_len = checked_cast<int>(der_cert.size());
> +  ScopedX509 cert(d2i_X509(NULL, &cert_data, cert_data_len));
> +  crypto::ScopedEVP_PKEY scoped_key(X509_get_pubkey(cert_handle));
> +  if (!scoped_key)
> +    return false;
> +  DCHECK(scoped_key.get());
> +  EVP_PKEY* key = scoped_key.get();
> +
> +  // NOTE: X509_verify() returns 1 in case of success, 0 or -1 on error.
> +  return X509_verify(cert.get(), key) == 1;
> +}
> +
>  }  // namespace net
> Index: net/cert/x509_certificate_unittest.cc
> diff --git a/net/cert/x509_certificate_unittest.cc
> b/net/cert/x509_certificate_unittest.cc
> index
>
158806ed7da53dba364da4e487b89940bbf1b36d..9f4ca401489fbea9293cbc2aea06eeff5fc24b39
> 100644
> --- a/net/cert/x509_certificate_unittest.cc
> +++ b/net/cert/x509_certificate_unittest.cc
> @@ -697,6 +697,20 @@ TEST(X509CertificateTest, IsIssuedByEncoded) {
>    EXPECT_TRUE(google_cert->IsIssuedByEncoded(issuers));
>  }
>
> +TEST(X509CertificateTest, IsSelfSigned) {
> +  base::FilePath certs_dir = GetTestCertsDirectory();
> +
> +  scoped_refptr<X509Certificate> cert(
> +      ImportCertFromFile(certs_dir, "mit.davidben.der"));
> +  ASSERT_NE(static_cast<X509Certificate*>(NULL), cert.get());
> +  EXPECT_FALSE(X509Certificate::IsSelfSigned(cert->os_cert_handle()));
> +
> +  scoped_refptr<X509Certificate> self_signed(
> +      ImportCertFromFile(certs_dir, "aia-root.pem"));
> +  ASSERT_NE(static_cast<X509Certificate*>(NULL), self_signed.get());
> +
> EXPECT_TRUE(X509Certificate::IsSelfSigned(self_signed->os_cert_handle()));
> +}
> +
>  TEST(X509CertificateTest, IsIssuedByEncodedWithIntermediates) {
>    static const unsigned char kPolicyRootDN[] = {
>      0x30, 0x1e, 0x31, 0x1c, 0x30, 0x1a, 0x06, 0x03, 0x55, 0x04, 0x03, 0x0c,
> Index: net/cert/x509_certificate_win.cc
> diff --git a/net/cert/x509_certificate_win.cc
> b/net/cert/x509_certificate_win.cc
> index
>
ab92b6f2a628fcc10185274291d2f9c353ae346f..8a5ddcf8e0ca4b785060541906b4c332f5985dc9
> 100644
> --- a/net/cert/x509_certificate_win.cc
> +++ b/net/cert/x509_certificate_win.cc
> @@ -453,4 +453,17 @@ bool X509Certificate::IsIssuedByEncoded(
>    return false;
>  }
>
> +// static
> +bool X509Certificate::IsSelfSigned(OSCertHandle cert_handle) {
> +  return CryptVerifyCertificateSignatureEx(
> +      NULL,
> +      X509_ASN_ENCODING,
> +      CRYPT_VERIFY_CERT_SIGN_SUBJECT_CERT,
> +      cert_handle,
> +      CRYPT_VERIFY_CERT_SIGN_ISSUER_CERT,
> +      cert_handle,
> +      0,
> +      NULL);
> +}
> +
>  }  // namespace net
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[palmer, 2014-10-13 23:02:32]

palmer@chromium.org changed reviewers:
+ agl@chromium.org, davidben@chromium.org
- willchan@chromium.org

[palmer, 2014-10-13 23:02:32]

Adding some more OWNERS.

[palmer, 2014-10-16 17:30:12]

Say, OWNERS, felt and I would like to land this so that we can do some Fancy
Data Gathering. Thanks!

[felt, 2014-10-16 18:03:15]

let's get this party started: lgtm % comments

https://codereview.chromium.org/634033002/diff/1/net/cert/x509_certificate_io...
File net/cert/x509_certificate_ios.cc (right):

https://codereview.chromium.org/634033002/diff/1/net/cert/x509_certificate_io...
net/cert/x509_certificate_ios.cc:239: // TODO(palmer): Implement this.
nit: can you put the bug ID next to this TODO?

https://codereview.chromium.org/634033002/diff/1/net/cert/x509_certificate_op...
File net/cert/x509_certificate_openssl.cc (right):

https://codereview.chromium.org/634033002/diff/1/net/cert/x509_certificate_op...
net/cert/x509_certificate_openssl.cc:452: int cert_data_len =
checked_cast<int>(der_cert.size());
felt-bot says:
error: no template named 'checked_cast'; did you mean 'base::checked_cast'?

[agl, 2014-10-16 18:07:23]

Self-signed certs are those where the subject and issuer names are equal. The
signature doesn't actually matter.

[Ryan Sleevi, 2014-10-16 18:14:21]

On 2014/10/16 18:07:23, agl wrote:
> Self-signed certs are those where the subject and issuer names are equal. The
> signature doesn't actually matter.

Not quite true. We have several cross-signed with key rollover with duplicate
subjects.

Signature totally matters :)

[agl, 2014-10-16 18:19:52]

On 2014/10/16 18:14:21, Ryan Sleevi wrote:
> Not quite true. We have several cross-signed with key rollover with duplicate
> subjects.

Do we accept them!?

[Ryan Sleevi, 2014-10-16 18:25:48]

On 2014/10/16 18:19:52, agl wrote:
> On 2014/10/16 18:14:21, Ryan Sleevi wrote:
> > Not quite true. We have several cross-signed with key rollover with
duplicate
> > subjects.
> 
> Do we accept them!?

All our platforms support building through such chains, yes.

[palmer, 2014-10-16 20:51:39]

So, for our experiments, we'd like to check that the signatures actually do or
do not match. So for our porpoises, I hope this CL is good.

[Ryan Sleevi, 2014-10-21 22:27:44]

https://codereview.chromium.org/634033002/diff/120001/net/cert/x509_certifica...
File net/cert/x509_certificate_mac.cc (right):

https://codereview.chromium.org/634033002/diff/120001/net/cert/x509_certifica...
net/cert/x509_certificate_mac.cc:555: return X509_verify(cert.get(), key) == 1;
Mixing BoringSSL types like this with a platform-specific file is a no-go.

The way is something like

x509_util::CSSMCachedCertificate cached_cert;
OSStatus status = cached_cert.Init(cert_handle);
if (status)
  return false;

x509_util::CSSMFieldValue normalized_subject;
x509_util::CSSMFieldValue normalized_issuer;
status = cached_cert.GetField(&CSSMOID_X509V1SubjectNameStd,
&normalized_subject);
if (!status || !normalized_subject.field())
  return false;
status = cached_cert.GetField(&CSSMOID_X509V1IssuerNameStd, &normalized_issuer);
if (!status || !normalized_issuer.field())
  return false;

if (normalized_subject.field()->Length != normalized_issuer.field()->Length ||
    memcmp(normalized_subject.field()->Data, normalized_issuer.field()->Data,
normalized_issuer.field()->Length) != 0)
  return false;

CSSM_CL_HANDLE cl_handle = CSSM_INVALID_HANDLE;
status = SecCertificateGetCLHandle(os_cert_handle, &cl_handle);
if (status)
  return false;
CSSM_DATA cert_data;
status = SecCertificateGetData(os_cert_handle, &cert_data);
if (status)
  return false;
CSSM_RETURN ret = CSSM_CL_CertVerify(cl_handle, 0, &cert_data, &cert_data, NULL,
0);
if (ret)
  return false;
return true;

https://codereview.chromium.org/634033002/diff/120001/net/cert/x509_certifica...
File net/cert/x509_certificate_win.cc (right):

https://codereview.chromium.org/634033002/diff/120001/net/cert/x509_certifica...
net/cert/x509_certificate_win.cc:481:
reinterpret_cast<void*>(<const_cast<PCERT_CONTEXT>(cert_handle)),
stray < it seems (between void*(<const )

[palmer, 2014-10-21 23:02:13]

https://codereview.chromium.org/634033002/diff/120001/net/cert/x509_certifica...
File net/cert/x509_certificate_mac.cc (right):

https://codereview.chromium.org/634033002/diff/120001/net/cert/x509_certifica...
net/cert/x509_certificate_mac.cc:555: return X509_verify(cert.get(), key) == 1;
On 2014/10/21 22:27:43, Ryan Sleevi wrote:
> Mixing BoringSSL types like this with a platform-specific file is a no-go.
> 
> The way is something like
> 
> x509_util::CSSMCachedCertificate cached_cert;
> OSStatus status = cached_cert.Init(cert_handle);
> if (status)
>   return false;
> 
> x509_util::CSSMFieldValue normalized_subject;
> x509_util::CSSMFieldValue normalized_issuer;
> status = cached_cert.GetField(&CSSMOID_X509V1SubjectNameStd,
> &normalized_subject);
> if (!status || !normalized_subject.field())
>   return false;
> status = cached_cert.GetField(&CSSMOID_X509V1IssuerNameStd,
&normalized_issuer);
> if (!status || !normalized_issuer.field())
>   return false;
> 
> if (normalized_subject.field()->Length != normalized_issuer.field()->Length ||
>     memcmp(normalized_subject.field()->Data, normalized_issuer.field()->Data,
> normalized_issuer.field()->Length) != 0)
>   return false;
> 
> CSSM_CL_HANDLE cl_handle = CSSM_INVALID_HANDLE;
> status = SecCertificateGetCLHandle(os_cert_handle, &cl_handle);
> if (status)
>   return false;
> CSSM_DATA cert_data;
> status = SecCertificateGetData(os_cert_handle, &cert_data);
> if (status)
>   return false;
> CSSM_RETURN ret = CSSM_CL_CertVerify(cl_handle, 0, &cert_data, &cert_data,
NULL,
> 0);
> if (ret)
>   return false;
> return true;

Done.

https://codereview.chromium.org/634033002/diff/120001/net/cert/x509_certifica...
File net/cert/x509_certificate_win.cc (right):

https://codereview.chromium.org/634033002/diff/120001/net/cert/x509_certifica...
net/cert/x509_certificate_win.cc:481:
reinterpret_cast<void*>(<const_cast<PCERT_CONTEXT>(cert_handle)),
On 2014/10/21 22:27:43, Ryan Sleevi wrote:
> stray < it seems (between void*(<const )

Done.

[palmer, 2014-10-21 23:07:05]

Super double mega-thanks for your CSSM code, Ryan!

[palmer, 2014-10-22 01:54:58]

rsleevi: Looks like Windows has finally acquiesced to my demands. LGTY?

[Ryan Sleevi, 2014-10-24 22:56:25]

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
File net/cert/x509_certificate_ios.cc (right):

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
net/cert/x509_certificate_ios.cc:255: // TODO(palmer): Implement this.
Are you planning on doing this? It's a blocker for landing this CL :)

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
File net/cert/x509_certificate_openssl.cc (right):

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
net/cert/x509_certificate_openssl.cc:463: ScopedX509 cert(d2i_X509(NULL,
&cert_data, cert_data_len));
Why do you do this round-trip? |cert_handle| is an X509*

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
net/cert/x509_certificate_openssl.cc:467: DCHECK(scoped_key.get());
Unnecessary - you just handled it on 465/466, so don't DCHECK here

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
net/cert/x509_certificate_openssl.cc:468: EVP_PKEY* key = scoped_key.get();
Unnecessary temporary? Just use scoped_key.get() on 471?

[palmer, 2014-10-24 23:32:46]

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
File net/cert/x509_certificate_ios.cc (right):

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
net/cert/x509_certificate_ios.cc:255: // TODO(palmer): Implement this.
> Are you planning on doing this? It's a blocker for landing this CL :)

Why is it a blocker? The only callers of this API are going to be, in the short
term and likely in the medium term, UMA-gathering code by felt.

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
File net/cert/x509_certificate_openssl.cc (right):

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
net/cert/x509_certificate_openssl.cc:463: ScopedX509 cert(d2i_X509(NULL,
&cert_data, cert_data_len));
On 2014/10/24 22:56:24, Ryan Sleevi wrote:
> Why do you do this round-trip? |cert_handle| is an X509*

Done.

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
net/cert/x509_certificate_openssl.cc:467: DCHECK(scoped_key.get());
On 2014/10/24 22:56:24, Ryan Sleevi wrote:
> Unnecessary - you just handled it on 465/466, so don't DCHECK here

Done.

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
net/cert/x509_certificate_openssl.cc:468: EVP_PKEY* key = scoped_key.get();
On 2014/10/24 22:56:24, Ryan Sleevi wrote:
> Unnecessary temporary? Just use scoped_key.get() on 471?

Done.

[Ryan Sleevi, 2014-10-24 23:46:53]

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
File net/cert/x509_certificate_ios.cc (right):

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
net/cert/x509_certificate_ios.cc:255: // TODO(palmer): Implement this.
On 2014/10/24 23:32:46, Chromium Palmer wrote:
> > Are you planning on doing this? It's a blocker for landing this CL :)
> 
> Why is it a blocker? The only callers of this API are going to be, in the
short
> term and likely in the medium term, UMA-gathering code by felt.

1) You're gonna fail your unittest ;)
2) iOS is a supported platform. We've got enough issues from people doing "TODO
- not implemented" and fail-open that I'm very opposed to more of these. eg:
today's bug of http://crbug.com/426948

This is iOS code, so you can do the same thing we're roughly doing on line
249/250 to get this to pass - e.g. explicitly creating an
x509_util_ios::NSSCertificate and using an NSS API here.

[palmer, 2014-10-27 21:20:27]

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
File net/cert/x509_certificate_ios.cc (right):

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
net/cert/x509_certificate_ios.cc:255: // TODO(palmer): Implement this.
> This is iOS code, so you can do the same thing we're roughly doing on line
> 249/250 to get this to pass - e.g. explicitly creating an
> x509_util_ios::NSSCertificate and using an NSS API here.

Any hints as to what NSS API? The public docs seem to assume I am validating a
full chain against a trust anchor store.

[Ryan Sleevi, 2014-10-27 21:22:17]

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
File net/cert/x509_certificate_ios.cc (right):

https://codereview.chromium.org/634033002/diff/180001/net/cert/x509_certifica...
net/cert/x509_certificate_ios.cc:255: // TODO(palmer): Implement this.
On 2014/10/27 21:20:27, Chromium Palmer wrote:
> > This is iOS code, so you can do the same thing we're roughly doing on line
> > 249/250 to get this to pass - e.g. explicitly creating an
> > x509_util_ios::NSSCertificate and using an NSS API here.
> 
> Any hints as to what NSS API? The public docs seem to assume I am validating a
> full chain against a trust anchor store.

The same one you're using in the _nss file? :)

[palmer, 2014-10-27 23:06:32]

> The same one you're using in the _nss file? :)

:] Done.

[Ryan Sleevi, 2014-10-27 23:33:13]

Two BUGS that need to be fixed and one nit, but I'm gonna go ahead and give you
the LGTM because they're trivial and impossible to botch :)

https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
File net/cert/x509_certificate_ios.cc (right):

https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
net/cert/x509_certificate_ios.cc:256: SECKEYPublicKey* public_key =
CERT_ExtractPublicKey(nss_cert.cert_handle());
BUG: You're leaking this

https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
File net/cert/x509_certificate_nss.cc (right):

https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
net/cert/x509_certificate_nss.cc:286: SECKEYPublicKey* public_key =
CERT_ExtractPublicKey(cert_handle);
BUG: You're leaking this

https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
File net/cert/x509_certificate_openssl.cc (right):

https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
net/cert/x509_certificate_openssl.cc:39: typedef crypto::ScopedOpenSSL<X509,
X509_free>::Type ScopedX509;
Unnecessary

[palmer, 2014-10-28 00:59:38]

https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
File net/cert/x509_certificate_ios.cc (right):

https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
net/cert/x509_certificate_ios.cc:256: SECKEYPublicKey* public_key =
CERT_ExtractPublicKey(nss_cert.cert_handle());
> BUG: You're leaking this

What's the best way to handle it? I don't see a scoped type. Are we leaking it
because x509_util_ios::NSSCertificate copied the data, or are we also leaking it
in _nss.cc too?

[Ryan Sleevi, 2014-10-28 01:24:58]

On 2014/10/28 00:59:38, Chromium Palmer wrote:
>
https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
> File net/cert/x509_certificate_ios.cc (right):
> 
>
https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
> net/cert/x509_certificate_ios.cc:256: SECKEYPublicKey* public_key =
> CERT_ExtractPublicKey(nss_cert.cert_handle());
> > BUG: You're leaking this
> 
> What's the best way to handle it? I don't see a scoped type. Are we leaking it
> because x509_util_ios::NSSCertificate copied the data, or are we also leaking
it
> in _nss.cc too?

As I mentioned in _nss, you're leaking it in both places.

Either use a ScopedSECKEYPublicKey (from scoped_nss_types) or just call
SECKEY_DestroyPublicKey directly.

[palmer, 2014-10-28 23:13:42]

https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
File net/cert/x509_certificate_ios.cc (right):

https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
net/cert/x509_certificate_ios.cc:256: SECKEYPublicKey* public_key =
CERT_ExtractPublicKey(nss_cert.cert_handle());
On 2014/10/27 23:33:13, Ryan Sleevi wrote:
> BUG: You're leaking this

Done.

https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
File net/cert/x509_certificate_nss.cc (right):

https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
net/cert/x509_certificate_nss.cc:286: SECKEYPublicKey* public_key =
CERT_ExtractPublicKey(cert_handle);
On 2014/10/27 23:33:13, Ryan Sleevi wrote:
> BUG: You're leaking this

Done.

https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
File net/cert/x509_certificate_openssl.cc (right):

https://codereview.chromium.org/634033002/diff/240001/net/cert/x509_certifica...
net/cert/x509_certificate_openssl.cc:39: typedef crypto::ScopedOpenSSL<X509,
X509_free>::Type ScopedX509;
On 2014/10/27 23:33:13, Ryan Sleevi wrote:
> Unnecessary

Done.

[palmer, 2014-10-29 18:43:43]

The CQ bit was checked by palmer@chromium.org

[commit-bot: I haz the power, 2014-10-29 18:45:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634033002/260001

[commit-bot: I haz the power, 2014-10-29 19:51:29]

Committed patchset #12 (id:260001)

[commit-bot: I haz the power, 2014-10-29 19:52:42]

Patchset 12 (id:??) landed as
https://crrev.com/32b3d8ab60476036ea7d687f43f23e0b63796a5a
Cr-Commit-Position: refs/heads/master@{#301912}