Check X509Certificate::CreateFromHandle result.

Check X509Certificate::CreateFromHandle result.

Previously CreateFromHandle could not fail, as it assumed creating an OSCertHandle would validate the contents. Any errors during X509Certificate::Initialize were silently ignored. A followup change will expose errors during X509Certificate::Initialize by failing to create an X509Certificate. Further followups will invalidate the assumption that creating an "OSCertHandle" completely parses and validates the certificate data.

BUG=671420
Review-Url: https://codereview.chromium.org/2760723002
Cr-Commit-Position: refs/heads/master@{#459955}
Committed: https://chromium.googlesource.com/chromium/src/+/5ed988687799ba8050ecda6e69ee3b0702a9b2ae

[mattm, 2017-03-17 22:03:31]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 22:04:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-03-17 22:21:46]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[mattm, 2017-03-17 22:22:12]

Patchset #2 (id:20001) has been deleted

[mattm, 2017-03-17 22:22:19]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-03-17 22:22:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-03-17 22:24:00]

mattm@chromium.org changed reviewers:
+ rsleevi@chromium.org

[mattm, 2017-03-17 22:24:01]

I wasn't too sure about the CertVerifyProc changes, so feel free to object or
suggest other ways of handling those failures.

https://codereview.chromium.org/2760723002/diff/40001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/2760723002/diff/40001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.cc:426: LOG(ERROR) <<
"X509Certificate::CreateFromHandle failed";
just trying to match the style of the rest of this file.. I can remove it if you
want though :)

[commit-bot: I haz the power, 2017-03-17 22:45:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 22:45:45]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mattm, 2017-03-17 23:03:26]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 23:03:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-03-17 23:13:02]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 23:13:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-18 01:03:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-18 01:03:28]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mattm, 2017-03-21 05:26:43]

Not sure if this got buried in your inbox or you just didn't have time to look
yet.

[mattm, 2017-03-21 05:26:44]

Not sure if this got buried in your inbox or you just didn't have time to look
yet.

[Ryan Sleevi, 2017-03-21 12:36:39]

rsleevi@chromium.org changed reviewers:
+ eroman@chromium.org

[Ryan Sleevi, 2017-03-21 12:36:40]

Sorry, was out Monday, and in CA/B Forum through Thursday. This was one I wanted
to spend more time with, but will defer to Eric instead.

[eroman, 2017-03-21 21:24:47]

Can you give more context on this change? Either in the bug description, or by
linking to a bug.

This adds checks around X509Certificate::CreateFromHandle(), which at present
cannot return null (it just returns a new X509Certificate).

I am guessing this is in anticipation of making changes to that function for the
X509Certificate/DER work, but would like to see that described to help my
review.

Thanks!

[mattm, 2017-03-22 01:57:15]

Description was changed from

==========
Fix places that weren't checking X509Certificate creation result [net/].

BUG=none
==========

to

==========
Check X509Certificate::CreateFromHandle result.

Previously CreateFromHandle could not fail, as it assumed creating an
OSCertHandle would validate the contents. Any errors during
X509Certificate::Initialize were silently ignored. A followup change will expose
errors during X509Certificate::Initialize by failing to create an
X509Certificate. Further followups will invalidate the assumption that creating
an "OSCertHandle" completely parses and validates the certificate data.

BUG=671420
==========

[mattm, 2017-03-22 02:02:46]

On 2017/03/21 21:24:47, eroman wrote:
> Can you give more context on this change? Either in the bug description, or by
> linking to a bug.
> 
> This adds checks around X509Certificate::CreateFromHandle(), which at present
> cannot return null (it just returns a new X509Certificate).
> 
> I am guessing this is in anticipation of making changes to that function for
the
> X509Certificate/DER work, but would like to see that described to help my
> review.
> 
> Thanks!

Updated CL description.

https://codereview.chromium.org/2758803003 is the followup which allows
Initialize & CreateFromHandle to fail.

https://codereview.chromium.org/2746103003 is the WIP followup to that. Not
quite ready for review but you can peek at it if you are curious.

[eroman, 2017-03-22 18:42:32]

Thanks for the extra background Matt!

Can you explain a bit more why the decision to surface parsing failure on
creation of X509Certificate, rather than maintaining the current semantics of
default values in X509Certificate on failure?

My understanding of the CL progression is that ultimately X509Certificate
becomes a container of
  (1) CRYPTO_BUFFER for the DER of the target cert
  (2) vector of CRYPTO_BUFFER for the intermediates
  (3) Immutable cache for core parsed fields: subject, issuer, validity, serial
number (although even this maybe can go...)

So an alternate design is for X509Certificate::CreateFromHandle(), in which
"handle" is now a CRYPTO_BUFFER, to always returns a new object which wraps the
given CRYPTO_BUFFERs for target/intermediates.

The accessor methods serial_number(), subject(), issuer(), valid_start(),
valid_end() would return their default (empty) values as they do today in the
case where X509Certificate::Initialize() fails to parse the DER enough to
extract those fields.

The advantage of not returning a null X509Certificate on parse failure is:
* not needing to do more nullity checking (forgetting it may lead to crashes) as
in this CL
* always having access to the certificate DER even for failed parses (useful for
error propagation).

The main disadvantage to not erroring at creation is the resulting ambiguity for
cert->subject(), cert->issuer() etc when the basic tier of parsing failed. That
said, this ambiguity is pretty benign, since the empty defaults are reasonable,
and moreover the decisions on them should be guarded by certificate validation
success anyway. Also, we probably already have this ambiguity given that
X509Certificate::Initialize() can fail today, so not strictly a new thing.

Finally, in regards to subject, issuer, validity, serial number -- can we remove
them in the final conversion, and make X509Certificate be purely a
representation for a certificate chain (i.e. vector of CRYPTO_BUFFER) ? Assuming
the parsing is fast enough for the current use-cases of those accessors (which I
suspect it is), that means access to all fields can just be in terms of lazy
parsing on CRYPTO_BUFFER. This will remove the desire to coerce things
(intermediates) into X509Certificate just to use those helpers, since all the
helpers will already be in terms of CRYPTO_BUFFER (the new OSCertHandle of
sorts).

WDTY?

[Ryan Sleevi, 2017-03-22 18:46:25]

On 2017/03/22 18:42:32, eroman wrote:
> Thanks for the extra background Matt!
> 
> Can you explain a bit more why the decision to surface parsing failure on
> creation of X509Certificate, rather than maintaining the current semantics of
> default values in X509Certificate on failure?
> 
> My understanding of the CL progression is that ultimately X509Certificate
> becomes a container of
>   (1) CRYPTO_BUFFER for the DER of the target cert
>   (2) vector of CRYPTO_BUFFER for the intermediates
>   (3) Immutable cache for core parsed fields: subject, issuer, validity,
serial
> number (although even this maybe can go...)
> 
> So an alternate design is for X509Certificate::CreateFromHandle(), in which
> "handle" is now a CRYPTO_BUFFER, to always returns a new object which wraps
the
> given CRYPTO_BUFFERs for target/intermediates.
> 
> The accessor methods serial_number(), subject(), issuer(), valid_start(),
> valid_end() would return their default (empty) values as they do today in the
> case where X509Certificate::Initialize() fails to parse the DER enough to
> extract those fields.
> 
> The advantage of not returning a null X509Certificate on parse failure is:
> * not needing to do more nullity checking (forgetting it may lead to crashes)
as
> in this CL
> * always having access to the certificate DER even for failed parses (useful
for
> error propagation).

So this is something that David, Matt and I discussed, and apologies for not
discussing this design more broadly. The current implementation of OSCertHandle
strictly guarantees that only valid certificates are OSCertHandle. CRYPTO_BUFFER
explicitly does not try to provide that contract (this is part of the
performance and memory improvements), and so it makes sense to move this to the
layer above.

> 
> The main disadvantage to not erroring at creation is the resulting ambiguity
for
> cert->subject(), cert->issuer() etc when the basic tier of parsing failed.
That
> said, this ambiguity is pretty benign, since the empty defaults are
reasonable,
> and moreover the decisions on them should be guarded by certificate validation
> success anyway. Also, we probably already have this ambiguity given that
> X509Certificate::Initialize() can fail today, so not strictly a new thing.

Correct, these can already fail, so this is strictly more correct.

> 
> Finally, in regards to subject, issuer, validity, serial number -- can we
remove
> them in the final conversion, and make X509Certificate be purely a
> representation for a certificate chain (i.e. vector of CRYPTO_BUFFER) ?
Assuming
> the parsing is fast enough for the current use-cases of those accessors (which
I
> suspect it is), that means access to all fields can just be in terms of lazy
> parsing on CRYPTO_BUFFER. This will remove the desire to coerce things
> (intermediates) into X509Certificate just to use those helpers, since all the
> helpers will already be in terms of CRYPTO_BUFFER (the new OSCertHandle of
> sorts).

I think we want to avoid reparsing for these commonly-used UI fields. We should
see more classes pass CRYPTO_BUFFERs around, rather than X509Certificate, but
using this as the cache for UI relevant operations seems to make perfect sense.

[eroman, 2017-03-22 19:37:09]

I don't want to rehash past discussions that you already had, so maybe I am not
not the right reviewer for this.

But in terms of guaranteeing "validity" at the CreateFromHandle() layer, that
feels like a confused layering.

If we *fully* parse the certificate then and there, then that is something we
can guarantee, and explain in an unambiguous way. However fully parsing has its
own problems.

Short of that, if all we are doing is partially parsing the certificate to
extract the core fields, that is a much weaker guarantee of validity. Modeling
the rest of the code as if this partially parsed certificate is valid is not
accurate, and at odds with the other accessors we will be using to extract
interesting information from the certificate.

I am not sure I see the benefits from trying to promise a partial degree of
validity during this construction, what is the use case you find improved by the
partially valid guarantee?

[Ryan Sleevi, 2017-03-22 19:44:32]

On 2017/03/22 19:37:09, eroman wrote:
> But in terms of guaranteeing "validity" at the CreateFromHandle() layer, that
> feels like a confused layering.
> 
> If we *fully* parse the certificate then and there, then that is something we
> can guarantee, and explain in an unambiguous way. However fully parsing has
its
> own problems.

I probably explained it poorly. Today, OSCertHandle guarantees a minimal
structural validity (e.g. not deep-diving into extensions) across all platforms
- that is, the primary fields of the TBSCertificate are guaranteed to be
correct, and thus extractable for an X509Certificate.

Tomorrow, CRYPTO_BUFFER won't even guarantee that basic premise. Thus the goal
would be / is that if you have an X509Certificate, you can access the accessors.

> Short of that, if all we are doing is partially parsing the certificate to
> extract the core fields, that is a much weaker guarantee of validity. Modeling
> the rest of the code as if this partially parsed certificate is valid is not
> accurate, and at odds with the other accessors we will be using to extract
> interesting information from the certificate.

Could you explain? A number of our platforms don't dive beyond the
TBSCertificate.

> I am not sure I see the benefits from trying to promise a partial degree of
> validity during this construction, what is the use case you find improved by
the
> partially valid guarantee?

Does above explain?

[eroman, 2017-03-22 20:01:36]

We should probably continue this discussion over hangouts instead of hijacking
Matt's thread :)

But to answer your last question, why do you think the X509Certificate wrapper
needs to match any structural guarantees in the same manner that  OSCertHandle
does today? Do you have specific use cases you can describe that this model
simplifies?

The normal workflow is that we pass X509Certificate to CertVerifyProc for
validation. At this stage, the CRYPTO_BUFFER is going to necessarily get
converted to an (actual) OSCertHandle for the platform validator to operate on.
So all those basic validity checks we discussed, are done here (and duplicated).

Moreover, the validation itself will do remaining parsing checks that during the
course of validating the certificate chain itself (for instance parsing
extensions). We already have mechanisms to report errors at CertVerifyProc
layer, and if we wanted can provide more granular parsing errors here.

Consumers of X509Certificates that haven't been validated, can't assume validity
of the certificate. So why do a partial approach where some fields are
guaranteed to be available, but by and large the rest is not?

[Ryan Sleevi, 2017-03-22 20:04:31]

On 2017/03/22 20:01:36, eroman wrote:
> We should probably continue this discussion over hangouts instead of hijacking
> Matt's thread :)

It's good to document :)

> But to answer your last question, why do you think the X509Certificate wrapper
> needs to match any structural guarantees in the same manner that  OSCertHandle
> does today? Do you have specific use cases you can describe that this model
> simplifies?
> 
> The normal workflow is that we pass X509Certificate to CertVerifyProc for
> validation. At this stage, the CRYPTO_BUFFER is going to necessarily get
> converted to an (actual) OSCertHandle for the platform validator to operate
on.
> So all those basic validity checks we discussed, are done here (and
duplicated).

This may be the disconnect. Every X509Certificate does not pass through
CertVerifyProc, and we expect to see less of that in the future.

> Consumers of X509Certificates that haven't been validated, can't assume
validity
> of the certificate. So why do a partial approach where some fields are
> guaranteed to be available, but by and large the rest is not?

Consumers of X509Certificates are guaranteed that the OSCertHandle will have
parsed the certificate and they are guaranteed to be available. In a
CRYPTO_BUFFER world, that will not change.

[eroman, 2017-03-22 22:17:52]

Can you update the documentation of X509Certificate::CreateFromHandle() to
explain the contract (that callers need to check for null, and under what
circumstances it fails)

https://codereview.chromium.org/2760723002/diff/80001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_ios.cc (right):

https://codereview.chromium.org/2760723002/diff/80001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_ios.cc:124: continue;
What makes this a non-fatal failure, whereas GetDEREncoded() now is fatal?

https://codereview.chromium.org/2760723002/diff/80001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_ios.cc:269: return ERR_CERT_INVALID;
Is it necessary to set cert_status too?

https://codereview.chromium.org/2760723002/diff/80001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/2760723002/diff/80001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_mac.cc:899: return ERR_CERT_INVALID;
same question about cert_status throughout.

https://codereview.chromium.org/2760723002/diff/80001/net/cert/x509_certifica...
File net/cert/x509_certificate.cc (right):

https://codereview.chromium.org/2760723002/diff/80001/net/cert/x509_certifica...
net/cert/x509_certificate.cc:451: results.push_back(cert);
std::move ?

https://codereview.chromium.org/2760723002/diff/80001/net/ssl/client_cert_sto...
File net/ssl/client_cert_store_nss.cc (right):

https://codereview.chromium.org/2760723002/diff/80001/net/ssl/client_cert_sto...
net/ssl/client_cert_store_nss.cc:142: certs->push_back(cert);
std::move ?

https://codereview.chromium.org/2760723002/diff/80001/net/third_party/mozilla...
File net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp (right):

https://codereview.chromium.org/2760723002/diff/80001/net/third_party/mozilla...
net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:231:
imported_certs->push_back(x509_cert);
move?

[mattm, 2017-03-23 22:48:57]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 22:49:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-03-23 22:59:03]

updated. 

I also fixed the issue of setting verify_result->verified_cert to null on
failure instead of leaving the original (input) cert.

https://codereview.chromium.org/2760723002/diff/80001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_ios.cc (right):

https://codereview.chromium.org/2760723002/diff/80001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_ios.cc:124: continue;
On 2017/03/22 22:17:52, eroman wrote:
> What makes this a non-fatal failure, whereas GetDEREncoded() now is fatal?

Good point, seems this should be fatal as well.

https://codereview.chromium.org/2760723002/diff/80001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_ios.cc:269: return ERR_CERT_INVALID;
On 2017/03/22 22:17:52, eroman wrote:
> Is it necessary to set cert_status too?

Hm, I guess setting cert_status and letting that propagate to the return value
is preferable. That way if CertVerifyProc (non-platform-specific code) adds more
error statuses and recomputes the net error code, it doesn't get lost.

https://codereview.chromium.org/2760723002/diff/80001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/2760723002/diff/80001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_mac.cc:899: return ERR_CERT_INVALID;
On 2017/03/22 22:17:52, eroman wrote:
> same question about cert_status throughout.

Acknowledged.

https://codereview.chromium.org/2760723002/diff/80001/net/cert/x509_certifica...
File net/cert/x509_certificate.cc (right):

https://codereview.chromium.org/2760723002/diff/80001/net/cert/x509_certifica...
net/cert/x509_certificate.cc:451: results.push_back(cert);
On 2017/03/22 22:17:52, eroman wrote:
> std::move ?

Done.

https://codereview.chromium.org/2760723002/diff/80001/net/ssl/client_cert_sto...
File net/ssl/client_cert_store_nss.cc (right):

https://codereview.chromium.org/2760723002/diff/80001/net/ssl/client_cert_sto...
net/ssl/client_cert_store_nss.cc:142: certs->push_back(cert);
On 2017/03/22 22:17:52, eroman wrote:
> std::move ?

Done.

https://codereview.chromium.org/2760723002/diff/80001/net/third_party/mozilla...
File net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp (right):

https://codereview.chromium.org/2760723002/diff/80001/net/third_party/mozilla...
net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:231:
imported_certs->push_back(x509_cert);
On 2017/03/22 22:17:52, eroman wrote:
> move?

Done.

[eroman, 2017-03-23 23:41:32]

lgtm

https://codereview.chromium.org/2760723002/diff/120001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_ios.cc (right):

https://codereview.chromium.org/2760723002/diff/120001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:104: bool GetCertChainInfo(CFArrayRef
cert_chain, CertVerifyResult* verify_result) {
I left a comment about GetCertChainInfo() for the Win version. That pattern, if
you choose to use it, is applicable  to these other ones too.

https://codereview.chromium.org/2760723002/diff/120001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_win.cc (right):

https://codereview.chromium.org/2760723002/diff/120001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_win.cc:335: return true;
Should this be false? (more discussion on this below).

https://codereview.chromium.org/2760723002/diff/120001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_win.cc:378: return true;
Not clear what true/false means from this function.

From the code it looks like you false basically just means CreateFromHandle()
failed, and all the other short-circuits are true. From reading the signature,
my expectation may have been something different (that true means
verify_result->verified_cert was successfully assigned).

Could you clarify this with documentation, or a different code pattern?

One suggestion would be to keep the void return type, and set
verify_result->cert_status |= CERT_STATUS_INVALID internally in this function.
As in:

if (verified_cert_with_chain)
  verify_result->verified_cert = std::move(...);
else
  verify_result->cert_status |= CERT_STATUS_INVALID;

https://codereview.chromium.org/2760723002/diff/120001/net/cert/x509_certific...
File net/cert/x509_certificate.h (right):

https://codereview.chromium.org/2760723002/diff/120001/net/cert/x509_certific...
net/cert/x509_certificate.h:130: // underlying crypto library. Returns NULL on
failure to parse or extract
nit: NULL --> nullptr ?

[commit-bot: I haz the power, 2017-03-24 00:44:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 00:44:53]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-03-24 21:49:35]

https://codereview.chromium.org/2760723002/diff/120001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_ios.cc (right):

https://codereview.chromium.org/2760723002/diff/120001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:104: bool GetCertChainInfo(CFArrayRef
cert_chain, CertVerifyResult* verify_result) {
On 2017/03/23 23:41:31, eroman wrote:
> I left a comment about GetCertChainInfo() for the Win version. That pattern,
if
> you choose to use it, is applicable  to these other ones too.

Done.

https://codereview.chromium.org/2760723002/diff/120001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_win.cc (right):

https://codereview.chromium.org/2760723002/diff/120001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_win.cc:335: return true;
On 2017/03/23 23:41:31, eroman wrote:
> Should this be false? (more discussion on this below).

I guess the problem is I'm not really sure either.

It *seems* like this is something that either shouldn't happen or that there
should already be some other error code set if it did, but I'm not really sure.
But maybe it's better to set an error code on any case like this, just to be
safe? I dunno.

https://codereview.chromium.org/2760723002/diff/120001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_win.cc:378: return true;
On 2017/03/23 23:41:31, eroman wrote:
> Not clear what true/false means from this function.
> 
> From the code it looks like you false basically just means CreateFromHandle()
> failed, and all the other short-circuits are true. From reading the signature,
> my expectation may have been something different (that true means
> verify_result->verified_cert was successfully assigned).
> 
> Could you clarify this with documentation, or a different code pattern?
> 
> One suggestion would be to keep the void return type, and set
> verify_result->cert_status |= CERT_STATUS_INVALID internally in this function.
> As in:
> 
> if (verified_cert_with_chain)
>   verify_result->verified_cert = std::move(...);
> else
>   verify_result->cert_status |= CERT_STATUS_INVALID;

I like the idea of just setting the status directly instead of returning a
value. Done.

https://codereview.chromium.org/2760723002/diff/120001/net/cert/x509_certific...
File net/cert/x509_certificate.h (right):

https://codereview.chromium.org/2760723002/diff/120001/net/cert/x509_certific...
net/cert/x509_certificate.h:130: // underlying crypto library. Returns NULL on
failure to parse or extract
On 2017/03/23 23:41:31, eroman wrote:
> nit: NULL --> nullptr ?

All the other comments say NULL, so I'm not sure if it's better to be consistent
or not?

[mattm, 2017-03-24 21:49:44]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 21:50:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-03-24 22:07:23]

lgtm

https://codereview.chromium.org/2760723002/diff/120001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_win.cc (right):

https://codereview.chromium.org/2760723002/diff/120001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_win.cc:335: return true;
On 2017/03/24 21:49:35, mattm wrote:
> On 2017/03/23 23:41:31, eroman wrote:
> > Should this be false? (more discussion on this below).
> 
> I guess the problem is I'm not really sure either.
> 
> It *seems* like this is something that either shouldn't happen or that there
> should already be some other error code set if it did, but I'm not really
sure.
> But maybe it's better to set an error code on any case like this, just to be
> safe? I dunno.

Either approach sounds fine to me.

https://codereview.chromium.org/2760723002/diff/140001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_ios.cc (right):

https://codereview.chromium.org/2760723002/diff/140001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:146: verify_result->cert_status |=
CERT_STATUS_INVALID;
I don't know about this one, as reaching it means the chain was empty. That
said, according to the DCHECK() at the start on cert_chain's length, this
shouldn't be reachable, so meh.

https://codereview.chromium.org/2760723002/diff/140001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/2760723002/diff/140001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_mac.cc:200: verify_result->cert_status |=
CERT_STATUS_INVALID;
same comment as for ios

[commit-bot: I haz the power, 2017-03-24 23:06:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 23:06:29]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-03-27 23:24:37]

https://codereview.chromium.org/2760723002/diff/120001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_win.cc (right):

https://codereview.chromium.org/2760723002/diff/120001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_win.cc:335: return true;
On 2017/03/24 22:07:23, eroman wrote:
> On 2017/03/24 21:49:35, mattm wrote:
> > On 2017/03/23 23:41:31, eroman wrote:
> > > Should this be false? (more discussion on this below).
> > 
> > I guess the problem is I'm not really sure either.
> > 
> > It *seems* like this is something that either shouldn't happen or that there
> > should already be some other error code set if it did, but I'm not really
> sure.
> > But maybe it's better to set an error code on any case like this, just to be
> > safe? I dunno.
> 
> Either approach sounds fine to me. 

Ok, I'll just leave this one as is for now.

https://codereview.chromium.org/2760723002/diff/140001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_ios.cc (right):

https://codereview.chromium.org/2760723002/diff/140001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:146: verify_result->cert_status |=
CERT_STATUS_INVALID;
On 2017/03/24 22:07:23, eroman wrote:
> I don't know about this one, as reaching it means the chain was empty. That
> said, according to the DCHECK() at the start on cert_chain's length, this
> shouldn't be reachable, so meh.

Yeah, on this and the mac one, due to the presence of the NOTREACHED, I felt
more confident that it shouldn't really happen and adding the error code just to
be safe.

But yeah, probably doesn't matter either way.

[mattm, 2017-03-27 23:24:44]

The CQ bit was checked by mattm@chromium.org

[commit-bot: I haz the power, 2017-03-27 23:25:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 00:45:13]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490657084015920,
"parent_rev": "f0b120b107fa1429a89fa180561d098fbf0b77ae", "commit_rev":
"5ed988687799ba8050ecda6e69ee3b0702a9b2ae"}

[commit-bot: I haz the power, 2017-03-28 00:46:19]

Description was changed from

==========
Check X509Certificate::CreateFromHandle result.

Previously CreateFromHandle could not fail, as it assumed creating an
OSCertHandle would validate the contents. Any errors during
X509Certificate::Initialize were silently ignored. A followup change will expose
errors during X509Certificate::Initialize by failing to create an
X509Certificate. Further followups will invalidate the assumption that creating
an "OSCertHandle" completely parses and validates the certificate data.

BUG=671420
==========

to

==========
Check X509Certificate::CreateFromHandle result.

Previously CreateFromHandle could not fail, as it assumed creating an
OSCertHandle would validate the contents. Any errors during
X509Certificate::Initialize were silently ignored. A followup change will expose
errors during X509Certificate::Initialize by failing to create an
X509Certificate. Further followups will invalidate the assumption that creating
an "OSCertHandle" completely parses and validates the certificate data.

BUG=671420

Review-Url: https://codereview.chromium.org/2760723002
Cr-Commit-Position: refs/heads/master@{#459955}
Committed:
https://chromium.googlesource.com/chromium/src/+/5ed988687799ba8050ecda6e69ee...
==========

[commit-bot: I haz the power, 2017-03-28 00:46:20]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/5ed988687799ba8050ecda6e69ee...