Allow revocation checking to be disabled on Mac, overriding/ignoring system settings

Allow certificate revocation checking to be enabled/disabled independent of the OS settings on OS X.

R=agl
BUG=78523, 79533
TEST=See bug for test case

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81702

[Ryan Sleevi, 2011-04-12 07:17:29]

agl: Would you be comfortable reviewing this? It's targeted presently at M13, so
if you would rather wait for wtc's return, that's fine. As shipping in M12, the
user can still consider revocation checking via Keychain Preferences - but it is
disabled by default.

wtc, rtenneti: FYI.

http://codereview.chromium.org/6824069/diff/2001/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6824069/diff/2001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:319: // ignored.
agl: Given your experience with the unfortunate reality of the SSL/TLS MITM
proxies, I was wondering if you might have any ancedotal information about
organizations that may use a local OCSP responder who might be affected by the
above change.

I followed the discussion last year on the PKIX mailing list ("Revising OCSP",
July 2010), and the impression I got was that the profile support in the main
deployed libraries was too weak for organizations to reliably deploy such
responders.

http://codereview.chromium.org/6824069/diff/2001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:331: tp_ocsp_options.Flags =
CSSM_TP_ACTION_OCSP_SUFFICIENT;
agl (or wtc): An alternative here would be to *require* OCSP if an OCSP
responder is present, making it a hard failure if both OCSP and CRL information
is in the certificate and OCSP is unavailable.

Given the DoS concerns regarding OCSP, I believed the above to be the correct
setting, but I wanted to make sure that was the posture you two/Google wanted to
take.

[agl, 2011-04-12 13:19:50]

LGTM

http://codereview.chromium.org/6824069/diff/2001/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6824069/diff/2001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:319: // ignored.
On 2011/04/12 07:17:29, Ryan Sleevi wrote:
> I followed the discussion last year on the PKIX mailing list ("Revising OCSP",
> July 2010), and the impression I got was that the profile support in the main
> deployed libraries was too weak for organizations to reliably deploy such
> responders.

I've never heard of anyone deploying a custom OCSP responder in any non-trivial
setup so I'm not worried about this.

http://codereview.chromium.org/6824069/diff/2001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:331: tp_ocsp_options.Flags =
CSSM_TP_ACTION_OCSP_SUFFICIENT;
On 2011/04/12 07:17:29, Ryan Sleevi wrote:
> Given the DoS concerns regarding OCSP, I believed the above to be the correct
> setting, but I wanted to make sure that was the posture you two/Google wanted
to
> take.

Yes it is, absolutely.

http://codereview.chromium.org/6824069/diff/2001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:843: tp_action_data.ActionFlags |=
CSSM_TP_ACTION_REQUIRE_REV_PER_CERT;
What does this do if both OCSP and CRL downloading fail? (At most we want to
throw a UI warning in this case.) Please update the comment with this
information.

[Ryan Sleevi, 2011-04-13 03:45:37]

agl: Updated the comment explaining a bit more about the behaviour. Would you
mind checking to make sure that comment reflects the behaviour you were
expecting? Also, a question below seeking clarification.

I'm also seeing Valgrind errors being raised from the Security.framework in the
latest try job (now that check_deps is fixed), so even if it LGTY, I'm going to
hold off until I can make sure the bug is Apple's, and if so, update this CL to
include suppressions.

http://codereview.chromium.org/6824069/diff/2001/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6824069/diff/2001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:331: tp_ocsp_options.Flags =
CSSM_TP_ACTION_OCSP_SUFFICIENT;
I realize the reference to "the above" was a bit ambiguous, so I wanted to
confirm. Your "Yes it is, absolutely." is referring to the code as written -
OCSP with CRL fallback, if both are present - with the *undesired* behaviour
being "OCSP or death", if both present, correct?

[agl, 2011-04-13 13:40:51]

LGTM.

The behaviour (which, from the new comments, appears to be the case with this
patch) should:
  * Check OCSP
  * if that fails, check CRL
  * If that fails, report unable to check revocation (which is just a minor UI
warning)

If the certificate doesn't contain any revocation mechanisms, then settings a
status flag is good, but that status flag shouldn't do anything.

The important point is that we can continue to function even if both the OCSP
responder and CRL server are down.

[wtc, 2011-04-20 19:40:33]

LGTM.  Thanks for fixing this in M12.  Just some nits below.

http://codereview.chromium.org/6824069/diff/8001/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6824069/diff/8001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:293: // the SSL peer that the certificate
should be verified against. |flags| is
Nit: should we change "SSL peer" to "SSL server"?

http://codereview.chromium.org/6824069/diff/8001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:320: // ignored.
Is the locally configured OCSP responder URL a system setting?
If so, we don't need to mention that here because we no longer
honor the system revocation settings.

Note that NSS supports a locally configured OCSP responder
URL, and Firefox exposes that in its UI.  I don't know how
many Firefox users use that feature.

http://codereview.chromium.org/6824069/diff/8001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:339: // CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY.
Ideally we want to not do any revocation checking in this case.

I got the CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY flag either
from some SDK sample code or at the suggestion of a code
reviewer.  Since I wasn't familiar with CryptoAPI's certificate
verification code, I used the flag.

http://codereview.chromium.org/6824069/diff/8001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:361: &kCFTypeArrayCallBacks);
Just wanted to confirm that CFArrayCreate will retain
all the CFTypeRefs in local_policies.

http://codereview.chromium.org/6824069/diff/8001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:860: tp_action_data.ActionFlags =
CSSM_TP_ACTION_FETCH_CERT_FROM_NET;
Good change!  I didn't know CSSM can do this.  I wonder why
Safari doesn't enable this feature.

http://codereview.chromium.org/6824069/diff/8001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:881: // so as to attempt OCSP fetching if it
believes a certificate may chain
Nit: OCSP fetching => OCSP checking

"fetching" implies fetching from the network to me.

http://codereview.chromium.org/6824069/diff/8001/third_party/apple_apsl/READM...
File third_party/apple_apsl/README.chromium (right):

http://codereview.chromium.org/6824069/diff/8001/third_party/apple_apsl/READM...
third_party/apple_apsl/README.chromium:38: cssmapplePriv.h from:
It seems bad for us to depend on a "private" header, so please
add a comment to explain why it is safe to do so.

Perhaps you can just move the comment you added to cssmapplePriv.h
here.

[Ryan Sleevi, 2011-04-20 22:46:51]

http://codereview.chromium.org/6824069/diff/8001/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6824069/diff/8001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:320: // ignored.
On 2011/04/20 19:40:33, wtc wrote:
> Is the locally configured OCSP responder URL a system setting?
> If so, we don't need to mention that here because we no longer
> honor the system revocation settings.
> 
> Note that NSS supports a locally configured OCSP responder
> URL, and Firefox exposes that in its UI.  I don't know how
> many Firefox users use that feature.

It is a system setting. I documented this because of the difference between the
Windows implementation.

On Windows, revocation checking is handled by whichever DLLs have registered as
the CryptoAPI CertDllVerifyRevocation implementation. Any settings (including
settings for the default implementation, cryptnet.dll), are respected. The only
setting we don't respect with regards to system (IE) settings is whether or not
to check revocation.

Here, on Mac, we don't respect /any/ system settings - whether or not to check
OCSP settings and whether or not to use a local responder. So there's that
difference.

I've gone and removed the comment though, to avoid confusion.

http://codereview.chromium.org/6824069/diff/8001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:339: // CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY.
On 2011/04/20 19:40:33, wtc wrote:
> Ideally we want to not do any revocation checking in this case.
> 
> I got the CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY flag either
> from some SDK sample code or at the suggestion of a code
> reviewer.  Since I wasn't familiar with CryptoAPI's certificate
> verification code, I used the flag.

Really? This surprises me a little. I thought the reason for disabling
revocation checking is if a user wishes to avoid potentially privacy-leaking
events (checking for CRLs, pinging an OCSP responder, potentially downloading an
AIA cert...). 

On Windows, and here, if a known-bad response is available, we still respect it
even when revocation checking is disabled. A known bad response can be obtained
by previously having checking enabled OR, at least in the case of Windows
(though I believe with Mac as well), by a system administrator pushing out a
local/organizational CRL revoking one or more certificates (via
HKLM\Software\Policies\Microsoft\SystemCertificates)

I just want to confirm your intent here is that revocation checking /fully/
disables revocation checking, and that known-revoked certificates (potentially
including the Comodo certificates) will be accepted as valid here.

http://codereview.chromium.org/6824069/diff/8001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:361: &kCFTypeArrayCallBacks);
On 2011/04/20 19:40:33, wtc wrote:
> Just wanted to confirm that CFArrayCreate will retain
> all the CFTypeRefs in local_policies.

Yes, per the documentation for the |callBacks| parameter at 
http://developer.apple.com/library/mac/#documentation/CoreFoundation/Referenc...

"The retain callback is used within this function, for example, to retain all of
the new values from the values C array. "

[wtc, 2011-04-20 23:23:54]

http://codereview.chromium.org/6824069/diff/8001/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6824069/diff/8001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:339: // CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY.
On 2011/04/20 22:46:51, Ryan Sleevi wrote:
>
> I just want to confirm your intent here is that revocation checking /fully/
> disables revocation checking, and that known-revoked certificates (potentially
> including the Comodo certificates) will be accepted as valid here.

Yes.  That's what I expect if I uncheck the "Check for
server certificate revocation" checkbox in our Options UI.
I believe a user unfamiliar with how revocation checking
is done will expect the same thing.

Does IE not behave this way?  Can you test the URL
https://test-ssev.verisign.com:2443/test-SSEV-revoked-verisign.html ?
(I don't have access to a Windows computer now.)  Thanks.

Note: Safari and Mac Chrome do behave this way as you
described.

In any case, either behavior is acceptable, so no need to
change this.