net: mark cert as revoked if EV revocation check receives revoked response (Win).

net: test that cert is marked as revoked if EV revocation check receives revoked response (Win).

If we do an online revocation check because a certificate is EV and we don't
have CRLSet coverage then we would like to mark the certificate as revoked
if the check indicates that, rather than just removing the EV badge.

However, on non-Windows platforms we don't get enough information from the
verification so this change just adds a test for Windows.

This also fixes a small memory leak.

BUG=279282
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220429

[agl, 2013-08-27 19:14:56]

This is easy for CryptoAPI, but I'm not sure whether the same is possible for
NSS/OS X? Do we get sufficient signals on those platforms?

[Ryan Sleevi, 2013-08-27 19:25:07]

OS X doesn't. Note that I'm not 100% convinced that CryptoAPI returns the right
error case in all cases - I think this may end up triggering some hard-fail-like
behaviour. CAs would be thrilled by this, but I don't know how users will feel.

For NSS I'd need to spleunk again. I think the information gets lost in
translation when it crosses the libpkix->nss boundary

https://codereview.chromium.org/23441005/diff/1/net/cert/cert_verify_proc_win.cc
File net/cert/cert_verify_proc_win.cc (right):

https://codereview.chromium.org/23441005/diff/1/net/cert/cert_verify_proc_win...
net/cert/cert_verify_proc_win.cc:656: if
(chain_context->TrustStatus.dwErrorStatus &
So, there's a slight side-effect of changing the order here that I'm not sure
about.

In the past, if we had an EV candidate (eg: has the OID), then we'd verify
online first, then try verifying (as non-EV) if the EV check failed.

If you have two versions of a CA - one EV enabled and one not - and the EV
enabled CA is revoked, the "old" behaviour is that CAPI would pick up the
revocation, and then try an alternate (non-revoked) path. The alternate path
would fail the EV policy OID  check, and thus come back with
NOT_VALID_FOR_USAGE.

We'd then downgrade to non-EV, and the path building would/should succeed.

The new behaviour would, by virtue of the ordering, have us treat the cert as
revoked, even though a valid path may exist. Alternatively, we'd flag it
NOT_VALID_FOR_USAGE, thus triggering an ERR_CERT_INVALID (IIRC), rather than an
ERR_CERT_REVOKED.

In practice, I don't think this matters, but it's something we should keep an
eye out for in bug reports. Only the largest of CAs would likely be playing with
both EV and non-EV enabled cross-signings, as I think it's a pretty crazy edge
case.

[agl, 2013-08-28 16:42:58]

PTAL.

https://codereview.chromium.org/23441005/diff/1/net/cert/cert_verify_proc_win.cc
File net/cert/cert_verify_proc_win.cc (right):

https://codereview.chromium.org/23441005/diff/1/net/cert/cert_verify_proc_win...
net/cert/cert_verify_proc_win.cc:656: if
(chain_context->TrustStatus.dwErrorStatus &
On 2013/08/27 19:25:07, Ryan Sleevi wrote:
> So, there's a slight side-effect of changing the order here that I'm not sure
> about.
> 
> In the past, if we had an EV candidate (eg: has the OID), then we'd verify
> online first, then try verifying (as non-EV) if the EV check failed.
> 
> If you have two versions of a CA - one EV enabled and one not - and the EV
> enabled CA is revoked, the "old" behaviour is that CAPI would pick up the
> revocation, and then try an alternate (non-revoked) path. The alternate path
> would fail the EV policy OID  check, and thus come back with
> NOT_VALID_FOR_USAGE.
> 
> We'd then downgrade to non-EV, and the path building would/should succeed.
> 
> The new behaviour would, by virtue of the ordering, have us treat the cert as
> revoked, even though a valid path may exist. Alternatively, we'd flag it
> NOT_VALID_FOR_USAGE, thus triggering an ERR_CERT_INVALID (IIRC), rather than
an
> ERR_CERT_REVOKED.
> 
> In practice, I don't think this matters, but it's something we should keep an
> eye out for in bug reports. Only the largest of CAs would likely be playing
with
> both EV and non-EV enabled cross-signings, as I think it's a pretty crazy edge
> case.

Ok, that's a fair point and I was really shuffling code around to try and avoid
some duplication, but I'll just leave it be and just fix the memory leak.

[Ryan Sleevi, 2013-08-28 19:23:11]

Blergh, I've confirmed that NSS will swallow the error and treat it as revoked.
Specifically, see
http://mxr.mozilla.org/nss/source/lib/libpkix/pkix/checker/pkix_revocationche...
- the overall status (PKIX_RevStatus_NoInfo) will be mapped into
PKIX_RevStatus_Revoked, which will ultimately cause it to raise a revoked error.

Otherwise, LGTM.

https://codereview.chromium.org/23441005/diff/31001/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/23441005/diff/31001/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:5746: // response, or an OCSP failure.
comment nit: ever trying to reduce the pronoun usage, suggested rewording:

// Currently only works for Windows. When using NSS or OS X, it's not
// possible to determine whether the check failed because of actual
// revocation or because there was an OCSP failure.

[wtc, 2013-08-28 23:58:42]

Patch set 2 LGTM.

Does this part in the CL's description still apply?

  If we do an online revocation check because a certificate is EV and we don't
  have CRLSet coverage then we should mark the certificate as revoked if the
  check indicates that, rather than just removing the EV badge.

https://codereview.chromium.org/23441005/diff/31001/net/cert/cert_verify_proc...
File net/cert/cert_verify_proc_win.cc (right):

https://codereview.chromium.org/23441005/diff/31001/net/cert/cert_verify_proc...
net/cert/cert_verify_proc_win.cc:688: (flags &
CertVerifier::VERIFY_REV_CHECKING_REQUIRED_LOCAL_ANCHORS)) {

Should we add

    && !rev_checking_enabled

to this conditional expression?

Otherwise we may be redoing the same verification if rev_checking_enabled
was already true.

https://codereview.chromium.org/23441005/diff/31001/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/23441005/diff/31001/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:5746: // response, or an OCSP failure.

Perhaps for NSS we can first request a soft-fail check for OCSP for an
EV candidate cert?

[agl, 2013-08-29 16:46:59]

https://codereview.chromium.org/23441005/diff/31001/net/cert/cert_verify_proc...
File net/cert/cert_verify_proc_win.cc (right):

https://codereview.chromium.org/23441005/diff/31001/net/cert/cert_verify_proc...
net/cert/cert_verify_proc_win.cc:688: (flags &
CertVerifier::VERIFY_REV_CHECKING_REQUIRED_LOCAL_ANCHORS)) {
On 2013/08/28 23:58:42, wtc wrote:
> 
> Should we add
> 
>     && !rev_checking_enabled
> 
> to this conditional expression?
> 
> Otherwise we may be redoing the same verification if rev_checking_enabled
> was already true.

Just adding the conditional wouldn't be correct, I believe, because it would
skip the hard fail in the case that we did a soft-fail, online check. But there
might be scope for avoiding a verification here. However, I don't think it's in
scope for this change.

https://codereview.chromium.org/23441005/diff/31001/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/23441005/diff/31001/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:5746: // response, or an OCSP failure.
On 2013/08/28 19:23:11, Ryan Sleevi wrote:
> comment nit: ever trying to reduce the pronoun usage, suggested rewording:
> 
> // Currently only works for Windows. When using NSS or OS X, it's not
> // possible to determine whether the check failed because of actual
> // revocation or because there was an OCSP failure.

Updated comment.

https://codereview.chromium.org/23441005/diff/31001/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:5746: // response, or an OCSP failure.
On 2013/08/28 23:58:42, wtc wrote:
> 
> Perhaps for NSS we can first request a soft-fail check for OCSP for an
> EV candidate cert?

That means that we would be making two OCSP requests for any EV certificate
that's not covered by a CRLSet? I'm not convinced that making a single one is
really needed, but I'm pretty sure that two is too much for this.

Counterpoints welcome but I'll save that for a future CL.

[commit-bot: I haz the power, 2013-08-29 20:51:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/23441005/19001

[commit-bot: I haz the power, 2013-08-29 23:37:50]

Change committed as 220429