Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1557133002:
Perform CRLSet evaluation during Path Building on Windows (Closed)
|
Created:
4 years, 11 months ago by Ryan Sleevi Modified:
4 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPerform CRLSet evaluation during Path Building on Windows
On Windows, add CRLSet checking to the path building phase by
registering a CryptoAPI Revocation Provider. The CRLSet is stashed
in thread-local storage in order to make it from the CertVerifyProc
to the Revocation Provider callback. CRLSet evaluation still happens
at the end for the completed chain, but this should reduce the risk
of path building errors.
The Revocation Provider always returns one of two messages - unknown
or revoked. It never positively asserts that a certificate is NOT
revoked, in order to allow the CRL and OCSP caches to still serve
as secondary sources of data.
BUG=570908
TEST=TODO
Committed: https://crrev.com/f140b3b1a394a74efcfd2c2f59d3890a496962ac
Cr-Commit-Position: refs/heads/master@{#374301}
Patch Set 1 #
Total comments: 48
Patch Set 2 : Beginning of tests #Patch Set 3 : More tests #
Total comments: 28
Patch Set 4 : Review feedback #
Total comments: 13
Patch Set 5 : Review feedback #Patch Set 6 : Rebased #Patch Set 7 : Test fixes #Messages
Total messages: 42 (12 generated)
rsleevi@chromium.org changed reviewers: + davidben@chromium.org, eroman@chromium.org
David, Eric: This definitely needs more tests, which is next on my agenda to do, but this substantially overhauls how Windows does the CRLSet parsing (mostly early returns, and should have the same observable effects). And also because I don't want to be the only one hoarding CAPI knowledge.
More context and background reading: https://msdn.microsoft.com/en-us/library/ms995348.aspx covers the basic layering idea https://msdn.microsoft.com/en-us/library/windows/desktop/aa380214(v=vs.85).aspx (for in process injecting) https://msdn.microsoft.com/en-us/library/windows/desktop/aa377167(v=vs.85).aspx https://msdn.microsoft.com/en-us/library/windows/desktop/aa377524(v=vs.85).aspx (for documenting revocation providers API behaviours) Note that though CertVerifyRevocation is documented as taking a chain, none of the invocations Chrome makes cause it to actually do so (instead, it goes a cert at a time). I'm not sure the value in enhancing the code to handle the chain, and from looking at how the Microsoft implementations work, both the CRL and OCSP revocation providers only do a cert-at-a-time for the inbound chain (you can attach a debugger to CryptNet.dll's CertDllCertVerifyRevocation function to see the entry into MicrosoftCertVerifyRevocation, which has that behaviour)
Oh, and finally, PLEASE communicate if anything doesn't make sense or requires you to do added reading; hopefully I've got enough comments to make it logically flow (mod the API documentation in MSDN), but if there are things that are confusing or non-obvious, let me know.
https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... File net/cert/cert_verify_proc_win.cc (right): https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:714: previous_cert)) { Note: This was to avoid MSVC complaints about assignment in conditionals https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:859: DWORD chain_flags = CERT_CHAIN_REVOCATION_CHECK_CHAIN; I stopped supplying CACHE_END_CERT for two reasons: 1) It never did anything (the cert cached is keyed off of the hAdditionalStore to CertGetCertificateChain, so we've been disabling the Windows cache for some time) 2) To force revocation checks as best as possible (note: AFAICT, Windows still still perform revocation checks even for cached certs, but... can't find anything documented that says that)
I don't suppose we have any hope of tests here? https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... File net/cert/cert_verify_proc_win.cc (right): https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:396: CRLSetResult CheckRevocationWithCRLSet(CRLSet* crl_set, Documentation comment? It's somewhat unclear what previous_hash is and that it's actually an input/output parameter. Something like, on input, |*previous_hash| must either be empty or the hash of |issuer_cert|'s SPKI. On ??? return, |*previous_hash| will be set to the hash of |subject_cert|'s SPKI. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:411: NOTREACHED(); This NOTREACHED() is because, to have gotten this far, Windows must have been happy with the certificate, right? But this might fail if Windows' parser is lenient in any way since we tend to write strict ones. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:434: // The bytes of the serial number are stored little-endian. ...what? There's also the leading 00 bytes and the sign bit and stuff. Per https://msdn.microsoft.com/en-us/library/windows/desktop/aa377200(v=vs.85).aspx, apparently Microsoft strips off leading 0x00 and 0xff bytes (which means that this code can't distinguish negative serial numbers from the corresponding positive one). Anyway, CRLSet is actually just fine with all this (positive/negative collision aside) since it also strips leading zeros, but it probably deserves a mention in this comment. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:450: NOTREACHED(); Ditto about NOTREACHED(). https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:451: return kCRLSetError; This codepath doesn't touch *previous_hash which actually messes up the loop later. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:460: if (result == CRLSet::REVOKED) switch-case, so we catch it all? https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:463: previous_hash->swap(subject_hash); How come previous_hash does not get updated on revoke here but does up in line 418? https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:488: return kCRLSetUnknown; This check got moved from the end. It's a change in behavior; if an expired CRLSet affirmatively says that we should revoke some certs, I would think that still means we should revoke it. Even though we did, in this case, have to "unrevoke" something, good connectivity will pick that up, whereas an attacker might try to suppress downloading a new CRLSet. (If you disagree, then at least we should change this in all CertVerifyProc implementations and not have them behave differently.) https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:510: return result; This might warrant a comment like: // At this point |result| is the result of the leaf certificate. (This logic seems a little weird. If our results are {CRLSet::UNKNOWN, CRLSet::GOOD}, surely we should be returning kCRLSetUnknown? But if we want to change that, we should change that in all of them concurrently.) https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:628: base::ThreadLocalPointer<CRLSet> thread_local_crlset; Ooh, nice trick. We should use it to lessen some of the globals in nss_ocsp.h too. (Or replace it with Eric's new thing.) The current arrangement with globals and stuff is pretty problematic. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:660: if ((GET_CERT_ENCODING_TYPE(encoding_type) != X509_ASN_ENCODING) || Just to confirm, this one should never happen due to the second parameter to CryptInstallOIDFunctionAddress, right? https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:677: local_params.cbSize = sizeof(local_params); Nit: Could also take this and line 672 out and move it after the memset/memcpy and outside the conditional. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:689: // Verifying a chain; issuer is the next cert in the chain. A priori I would have expected this to have expected all the certificates. I mention this doesn't happen in the review comment, but it seems this should at least have a comment if not a (D)CHECK or error return or DumpWithoutCrashing or something to the effect. Or am I misunderstanding and that's something else? Microsoft's docs also say: """ Of these options, a provider MUST support the CERT_VERIFY_REV_CHAIN_FLAG flag. This flag indicates that all certificates in the certificate chain are to be validated. When this flag is provided, the calling application must provide the certificate chain in the rgpvContext[ ] parameter; when processing this parameter, a revocation provider might assume that this parameter contains the entire chain with the first one being the end-entity certificate. Examples of such calling applications include the Authenticode Software Publishing module and WinVerifyTrust(). When CERT_VERIFY_REV_CHAIN_FLAG is specified, the provider MUST return overall status versus status of an individual certificate. If it does not have enough information to make the appropriate requests, it MUST return CRYPT_E_NO_REVOCATION_CHECK. """ https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:714: previous_cert)) { On 2016/01/04 22:33:33, Ryan Sleevi wrote: > Note: This was to avoid MSVC complaints about assignment in conditionals I am fond of Go's decl + comparison pattern, but I don't think we do it in C++ much and the comma operator is kind of screwy. Perhaps: for (;;) { PCCERT_CONTEXT previous_cert = ....; if (!previous_cert) break; // Rest of the loop. } You can also then write this like... for (;;) { DWORD store_search_flags = CERT_STORE_SIGNATURE_FLAG; PCCERT_CONTEXT previous_cert = .... &store_search_flags ... if (!....) { } } Specifically this avoids having to reset store_search_flags in line 723. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:723: store_search_flags = CERT_STORE_SIGNATURE_FLAG; Looks like previous_cert gets leaked if you hit this codepath. Make it a ScopedPCCERT_CONTEXT? https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:740: revocation_status->dwError = static_cast<DWORD>(CRYPT_E_REVOCATION_OFFLINE); Why OFFLINE and not NO_REVOCATION_CHECK? The docs say: """ When CERT_VERIFY_REV_CHAIN_FLAG is specified, the provider MUST return overall status versus status of an individual certificate. If it does not have enough information to make the appropriate requests, it MUST return CRYPT_E_NO_REVOCATION_CHECK. """ https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:745: // TODO(rsleevi) Do baked-in certificate revocation. super nitpicky nit: comma after close-colon. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:751: if (crl_set->IsExpired()) Ditto re comment on line 485. (Also this would be redundant with CheckRevocationWithCRLSet's check, right?) https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:754: std::string unused; Nit: Since this is an input/output parameter, it's not quiiite unused. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:757: if (result == kCRLSetRevoked) { Nit: Should this be a switch-case just to explicitly list out the cases? It's not immediately obvious this is what you wanted for kCRLSetError. (Though it's probably fine?) https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:766: // certificate is already revoked. It doesn't return TRUE in that case either, right? https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:875: // Root store used by TestRootCerts as changed, via CertControlStore with the Root -> root? Or is this a Windows proper noun?
Tests are coming (see comment #2), just on triage duty this week and RWC so that's dominating, but I wanted to give y'all adequate time to look at it and stew on it and review/research/point out obvious bugs as I do :) Plus if I missed any bugs, we all learn from it - and hopefully will be able to spot if there are more. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... File net/cert/cert_verify_proc_win.cc (right): https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:396: CRLSetResult CheckRevocationWithCRLSet(CRLSet* crl_set, On 2016/01/06 03:37:48, davidben wrote: > Documentation comment? It's somewhat unclear what previous_hash is and that it's > actually an input/output parameter. Something like, on input, |*previous_hash| > must either be empty or the hash of |issuer_cert|'s SPKI. On ??? return, > |*previous_hash| will be set to the hash of |subject_cert|'s SPKI. Yeah, this part I threw up a little early to get feedback on, as I'm on triage rotation today & tomorrow. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:411: NOTREACHED(); On 2016/01/06 03:37:48, davidben wrote: > This NOTREACHED() is because, to have gotten this far, Windows must have been > happy with the certificate, right? But this might fail if Windows' parser is > lenient in any way since we tend to write strict ones. And it'll log, but behave consistently - but we certainly shouldn't reach this path. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:434: // The bytes of the serial number are stored little-endian. On 2016/01/06 03:37:48, davidben wrote: > ...what? There's also the leading 00 bytes and the sign bit and stuff. Per > https://msdn.microsoft.com/en-us/library/windows/desktop/aa377200(v=vs.85).aspx, > apparently Microsoft strips off leading 0x00 and 0xff bytes (which means that > this code can't distinguish negative serial numbers from the corresponding > positive one). But negative serials never happen! ;) https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:451: return kCRLSetError; On 2016/01/06 03:37:47, davidben wrote: > This codepath doesn't touch *previous_hash which actually messes up the loop > later. D'oh, good spot. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:460: if (result == CRLSet::REVOKED) On 2016/01/06 03:37:48, davidben wrote: > switch-case, so we catch it all? This was intentional; I think a switch hinders, rather than helps the readability here. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:463: previous_hash->swap(subject_hash); On 2016/01/06 03:37:48, davidben wrote: > How come previous_hash does not get updated on revoke here but does up in line > 418? Because revoke terminates processing. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:488: return kCRLSetUnknown; On 2016/01/06 03:37:48, davidben wrote: > This check got moved from the end. It's a change in behavior; if an expired > CRLSet affirmatively says that we should revoke some certs, I would think that > still means we should revoke it. Even though we did, in this case, have to > "unrevoke" something, good connectivity will pick that up, whereas an attacker > might try to suppress downloading a new CRLSet. An attacker can suppress downloading an updated CRLSet to block their revocation. But I think it's more important that we have the capability to "undo" something here - as evidenced by the bad revokes of Symantec & Comodo and the pain they caused. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:510: return result; On 2016/01/06 03:37:48, davidben wrote: > (This logic seems a little weird. If our results are {CRLSet::UNKNOWN, > CRLSet::GOOD}, surely we should be returning kCRLSetUnknown? But if we want to > change that, we should change that in all of them concurrently.) I don't understand what you're suggesting we would change - our results are kCRLSetGood or kCRLSetUnknown https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:628: base::ThreadLocalPointer<CRLSet> thread_local_crlset; On 2016/01/06 03:37:48, davidben wrote: > Ooh, nice trick. We should use it to lessen some of the globals in nss_ocsp.h > too. (Or replace it with Eric's new thing.) The current arrangement with globals > and stuff is pretty problematic. Um, wat? That doesn't really reduce the globals at all for nss_ocsp - that's how NSS behaves and requires. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:660: if ((GET_CERT_ENCODING_TYPE(encoding_type) != X509_ASN_ENCODING) || On 2016/01/06 03:37:48, davidben wrote: > Just to confirm, this one should never happen due to the second parameter to > CryptInstallOIDFunctionAddress, right? Right https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:689: // Verifying a chain; issuer is the next cert in the chain. On 2016/01/06 03:37:48, davidben wrote: > A priori I would have expected this to have expected all the certificates. I > mention this doesn't happen in the review comment, but it seems this should at > least have a comment if not a (D)CHECK or error return or DumpWithoutCrashing or > something to the effect. Or am I misunderstanding and that's something else? Self-signed certs and janky-ass third-party invocations (because it's installed in the process space, this will be called for everything - including things like WinVerifyTrust and Authenticode) I suppose I could move line 748 up some to bail early for those cases > When CERT_VERIFY_REV_CHAIN_FLAG is specified, the provider MUST return overall > status versus status of an individual certificate. If it does not have enough > information to make the appropriate requests, it MUST return > CRYPT_E_NO_REVOCATION_CHECK. Which is what we do. I explained in the commit comment and in the comments here that we either say "try the next one" or "gtfo". CAPI *has* to call this during path building - in fact, CAPI *never* calls it with a chain for CertGetCertificateChain(), because it's already computed the status per-cert during path building. I'm not sure I understand what your concern is? https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:723: store_search_flags = CERT_STORE_SIGNATURE_FLAG; On 2016/01/06 03:37:48, davidben wrote: > Looks like previous_cert gets leaked if you hit this codepath. Make it a > ScopedPCCERT_CONTEXT? Line 711 frees it (all the enumerative functions take the previous cert to use as the iterator context, with null being the 'start enumeration' context) https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:740: revocation_status->dwError = static_cast<DWORD>(CRYPT_E_REVOCATION_OFFLINE); On 2016/01/06 03:37:48, davidben wrote: > """ > When CERT_VERIFY_REV_CHAIN_FLAG is specified, the provider MUST return overall > status versus status of an individual certificate. If it does not have enough > information to make the appropriate requests, it MUST return > CRYPT_E_NO_REVOCATION_CHECK. > """ Because it has enough information (e.g. from the certificate or, in this case, by the fact that we're called) but it's unable to. CRYPT_E_NO_REVOCATION_CHECK is the no-error try next case CRYPT_E_REVOCATION_OFFLINE indicates there was an active issue w/ revocation checking. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:745: // TODO(rsleevi) Do baked-in certificate revocation. On 2016/01/06 03:37:48, davidben wrote: > super nitpicky nit: comma after close-colon. super nitpicky nit nit: Did you mean colon after close-paren? ;) https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:751: if (crl_set->IsExpired()) On 2016/01/06 03:37:48, davidben wrote: > Ditto re comment on line 485. (Also this would be redundant with > CheckRevocationWithCRLSet's check, right?) No - this is executed during path building, and CheckRevocationWithCRLSet doesn't check IsExpired (you're thinking CheckChainRevocationWithCRLSet, which does, and which this is the mirror too) Because CheckChainRev is implemented in terms of CheckRev, I didn't want to have to check IsExpired in every iteration of the loop of CheckChainRev, hence why I foisted it out of CheckRev and into here (... hopefully that makes sense) https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:754: std::string unused; On 2016/01/06 03:37:48, davidben wrote: > Nit: Since this is an input/output parameter, it's not quiiite unused. Is there a concrete suggestion here for what you think is more suitable? I think "unused" here is entirely accurate in terms of the caller context - even if the function uses it. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:757: if (result == kCRLSetRevoked) { On 2016/01/06 03:37:48, davidben wrote: > Nit: Should this be a switch-case just to explicitly list out the cases? It's > not immediately obvious this is what you wanted for kCRLSetError. (Though it's > probably fine?) It is not what we want for kCRLSetError, which the documentation on 475-ish was meant to cover (which needs to be foisted into enum documentation) https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:766: // certificate is already revoked. On 2016/01/06 03:37:48, davidben wrote: > It doesn't return TRUE in that case either, right? Comment BUG, thanks :) Revoked gets a FALSE return too, and SetLastError smuggles that state; TRUE is only used for "it's good" https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:875: // Root store used by TestRootCerts as changed, via CertControlStore with the On 2016/01/06 03:37:48, davidben wrote: > Root -> root? Or is this a Windows proper noun? It's the name of a specific Logical Store ("Root")
https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... File net/cert/cert_verify_proc_win.cc (right): https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:628: base::ThreadLocalPointer<CRLSet> thread_local_crlset; I guess I should mention what I'd mentioned to Eric, which is that this is kind of a gross hack. I mean, it works, because CertVerifyRevocation is called during chain building, and presently chain building is single threaded, but that's not an API guarantee. I originally had something far grosser (which is more in line with the spec), which is creating a fake certificate (fake SPKI and everything), fake-signing it (using one of the old "sha1WithNoEncryption" signature algorithm OIDs, which triggers a special path in Windows), and sticking a pointer to the CRLSet in an extension. In order to prevent an attacker from supplying an evil cert w/ the same extension and parameters, I further had an ASLR-like magic value in the extension that had to match the per-process magic value before the pointer was trusted and de-referenced. Of course, there was even MORE logic to ensure CryptoAPI wouldn't cache this dummy cert (past the life of the pointer), including signalling the necessary events on function exit to get CAPI to flush its caches. As you can tell, this was gross as hell, but is more API conforming. I didn't want to document all of what the 'didn't work', but I suppose I should document more about why this is still "Gross Hack", but "cleaner-though-not-quite-conforming" than the alternative.
A few more comments and refinements, as well as the beginning of more tests (so you can see the permutations I'm going to cover) These tests fail on other platforms (somewhat expected - womp womp) but at least covers most cases. I didn't cover the AIA case, but I view that as a variation of the 'misordered chain' case (at least, it is for Windows; for OS X and NSS it's... more interesting) https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... File net/cert/cert_verify_proc_win.cc (right): https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:434: // The bytes of the serial number are stored little-endian. On 2016/01/06 04:26:20, Ryan Sleevi wrote: > But negative serials never happen! ;) Confirmed it's a documentation bug; it's talking about the CertCompareIntegerBlob function. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:510: return result; On 2016/01/06 03:37:48, davidben wrote: > This might warrant a comment like: > // At this point |result| is the result of the leaf certificate. > > (This logic seems a little weird. If our results are {CRLSet::UNKNOWN, > CRLSet::GOOD}, surely we should be returning kCRLSetUnknown? But if we want to > change that, we should change that in all of them concurrently.) I reparsed what you're saying - you're talking about say, a root, intermediate, leaf which results in {UNKNOWN, UNKNOWN, GOOD} - in this case, returning kCRLSetOK for the reasons documented above in kCRLSetUnknown - to handle the case where the intermediate is covered by the CRLSet. I'm not entirely sure I agree with that decision, but any intermediate revocations will be done via hard blacklist *anyway*, so it's not the end of the world?
Eric, David: I think this is good for the groundwork, mod restricting the tests to Windows. After going through this, my next steps will be to create an ad-hoc stack-allocated CRLSet for our hardcoded revocations (within CertVerifyProc's base) and pass those into the CertVerifyProc::VerifyInternal's verifier, so that our hardcoded verifications also factor into chain building. From experimenting, that seems minimally necessary for OS X. Anyways, please consider doing a FULL REVIEW at this point on this code, mod the one known issue (test platform disabling) https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_win.cc (right): https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:725: if ((flags & CERT_VERIFY_REV_CHAIN_FLAG) && num_contexts > 1) { I just went ahead and did the iterative code, although I'm still somewhat torn on this. It's definitely not testing all the possible permutations of here, and the 'realistic' response is that I should probably spin out a cert_verify_proc_unittest_win.cc that just calls the WinAPI functions in the permutations (that is CertVerifyRevocation). But that seems wrong from a testing perspective (testing more of the implementation details), so I'm torn. Thoughts? https://codereview.chromium.org/1557133002/diff/40001/net/data/ssl/scripts/ge... File net/data/ssl/scripts/generate-multi-root-test-chains.sh (right): https://codereview.chromium.org/1557133002/diff/40001/net/data/ssl/scripts/ge... net/data/ssl/scripts/generate-multi-root-test-chains.sh:40: # 8. H -> I -> J2 -> K2 [revoked by SPKI] -> J2 -> K [revoked by SPKI] The complexities of these chains are for the inevitable NSS tests (see https://crbug.com/472978 and https://ssl-tools.net/subjects/7cb166549cabdb44ee622616adf4657bf77ad594 for how the cycles are structured ), but I wanted to make sure the Windows code all handled these cases fine (it does; the ROOT store has a greater priority queue in the Collection Store that is used for chain building, and so it trumps the notBefore sorting that Collection Stores normally use for Find operations, since Find is ordered by Priority -> StoreCallback, where StoreCallback is dependent on whether it's CRL, CTL, or Cert)
The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1557133002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1557133002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I still need to read through the test code more carefully (they're kinda verbose and it's late), but otherwise this looks good. https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... File net/cert/cert_verify_proc_win.cc (right): https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:628: base::ThreadLocalPointer<CRLSet> thread_local_crlset; On 2016/01/06 04:59:00, Ryan Sleevi wrote: > I guess I should mention what I'd mentioned to Eric, which is that this is kind > of a gross hack. > > I mean, it works, because CertVerifyRevocation is called during chain building, > and presently chain building is single threaded, but that's not an API > guarantee. > > I originally had something far grosser (which is more in line with the spec), > which is creating a fake certificate (fake SPKI and everything), fake-signing it > (using one of the old "sha1WithNoEncryption" signature algorithm OIDs, which > triggers a special path in Windows), and sticking a pointer to the CRLSet in an > extension. In order to prevent an attacker from supplying an evil cert w/ the > same extension and parameters, I further had an ASLR-like magic value in the > extension that had to match the per-process magic value before the pointer was > trusted and de-referenced. > > Of course, there was even MORE logic to ensure CryptoAPI wouldn't cache this > dummy cert (past the life of the pointer), including signalling the necessary > events on function exit to get CAPI to flush its caches. > > As you can tell, this was gross as hell, but is more API conforming. I didn't > want to document all of what the 'didn't work', but I suppose I should document > more about why this is still "Gross Hack", but > "cleaner-though-not-quite-conforming" than the alternative. Hrm, that's true. We would be assuming it calls the fetching callbacks from the same thread. My motivation here is that, while NSS's global callback is unavoidable, having nss_ocsp.h also have a global URLRequestContext and TaskRunner and stuff could be avoided with your thread-local trick. I started doing a cleanup of that code a while ago, but didn't get very far. The way things are right now is totally insane. EnsureNSSHttpIOInit gets called inside the SSLClientSocket implementations which samples the message loop then, but this is the wrong message loop anyway because you actually want the message loop SetURLRequestContextForNSSHttpIO calls. And then the EnsureNSSHttpIOInit calls happen at yet another point and has a ShutdownNSSHttpIO. If that stuff could be more local, maybe even switching to Eric's CertNetFetcherImpl now, I think that would be worthwhile. Anyway, this isn't actually relevant to this CL. https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:1373: GetTestCertsDirectory(), "multi-root-chain1.pem", Seeing as the tests themselves disassemble the pre-made chain files anyway, it seems better to put each individual certificate in its own file so I don't have to go back and check that multi-root-chain1's 3rd element is indeed such-and-such. It's bad enough that I have to look back at my notes to see what A, B, etc., are. https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:1423: EXPECT_EQ("F Root CA", verified_root->subject().common_name); It seems these tests could do with a few helper functions to make them less long. It's a bit difficult to read through them all. (In comparison, the tests we recently added for alt-chains in BoringSSL are a lot easier to read. https://boringssl.googlesource.com/boringssl/+/master/crypto/x509/x509_test.c... ) https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_win.cc (right): https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:420: // |previous_hash|. If |issuer_cert| is omitted, and |previous_hash| is empty, Nit: Most of these |previous_hash|s should probably be |*previous_hash|. (Or maybe all of them just |previous_hash|. I'm used to |*previous_hash| from BoringSSL, but I think Chromium tends use it interchangeably.) https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:450: std::string subject_hash = crypto::SHA256HashString(spki); Nit: I'd probably take lines 436 through 450 and pull them into some function like bool HashSPKI(PCCERT_CONTEXT cert, std::string* hash) { } Then you can call it here and lines 482 through 494. https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:454: previous_hash->swap(subject_hash); This is inconsistent with line 500's CRLSet::REVOKED which does *not* update previous_hash. I realize it doesn't matter, but this function should be consistent about it. I would probably go with updating it in all cases (except when we can't and have to clear it) to avoid surprises if someone tweaks things. https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:472: for (unsigned j = 0; j < serial_blob->cbData; j++) Nit: unsigned -> DWORD, to match cbData's type? https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:478: // use that; otherwise, compute it based on |issuer_cert|. Nit: I would take this block and switch it with the "Compute the subject's serial" block. It makes it more obvious why issuer_cert can't be NULL here. In fact, you could probably fold some of the checks together like: std::string issuer_hash_local; std::string* issuer_hash = previous_hash; if (issuer_hash->empty()) { if (!issuer_cert) { // No issuer cert nor a hash of the issuer's SPKI was provided, // so no further checks can be done. previous_hash->swap(subject_hash); return kCRLSetUnknown; } der_bytes = ... ... } https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:537: PCCERT_CONTEXT subject = elements[num_elements - i - 1]->pCertContext; Optional: If you want to avoid the num_elements - i - 1 computation : for (DWORD i = num_elements - 1; i < num_elements; i--) { https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:680: PCERT_CONTEXT* cert_contexts = reinterpret_cast<PCERT_CONTEXT*>(rgpvContext); Nit: newline here? Comments that aren't preceded by blank lines often hard for me to read, but it might be just me. https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:698: if ((GET_CERT_ENCODING_TYPE(encoding_type) != X509_ASN_ENCODING) || Style nit: Unnecessary parens. (Alternatively, missing parens on the line below if you want the parens) https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:733: for (DWORD i = num_contexts; i > 0; --i) { Optional: Another option is to write this as: for (DWORD i = num_contexts - 1; i < num_contexts; --i) { ... } which avoids the pesky i-1s. https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:735: if (!subject_cert) { Why does this function check for NULL while CheckChainRevocationWithCRLSet doesn't? Extra paranoia where it comes to third-party crap? https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:767: if (local_params.pIssuerCert && [I love how they don't say anything useful about the interaction between pIssuerCert and CERT_VERIFY_REV_CHAIN_FLAG. Ah well.] https://codereview.chromium.org/1557133002/diff/40001/net/data/ssl/scripts/ge... File net/data/ssl/scripts/generate-multi-root-test-chains.sh (right): https://codereview.chromium.org/1557133002/diff/40001/net/data/ssl/scripts/ge... net/data/ssl/scripts/generate-multi-root-test-chains.sh:40: # 8. H -> I -> J2 -> K2 [revoked by SPKI] -> J2 -> K [revoked by SPKI] [Incidentally, this became so much easier to understand once I realized to draw this out with certificates as edges between nodes rather than nodes.] https://codereview.chromium.org/1557133002/diff/40001/net/data/ssl/scripts/ge... net/data/ssl/scripts/generate-multi-root-test-chains.sh:161: # TODO(rsleevi): AKI/SPKI issues? Wouldn't it be equally unnecessary for C2 to sign B? (I'm not clear on what the comment is here for.) https://codereview.chromium.org/1557133002/diff/40001/net/data/ssl/scripts/ge... net/data/ssl/scripts/generate-multi-root-test-chains.sh:199: -extensions ca_cert_with_aki \ Why does this and the one below have AKI, but not the others? https://codereview.chromium.org/1557133002/diff/40001/net/data/ssl/scripts/ge... net/data/ssl/scripts/generate-multi-root-test-chains.sh:232: # TODO(rsleevi): AKI/SPKI issues? (Ditto about what this comment is for)
Thanks for the detailed look. A few of the tests ended up not relevant (for CryptoAPI), but will be relevant for NSS, hence why they're there but unused. I'll move those out to a separate CL once I start on the NSS work, which I haven't started in favor of restructuring the OS X work (which is simpler, but requires pulling out the hardcoded blacklist) https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... File net/cert/cert_verify_proc_win.cc (right): https://codereview.chromium.org/1557133002/diff/1/net/cert/cert_verify_proc_w... net/cert/cert_verify_proc_win.cc:628: base::ThreadLocalPointer<CRLSet> thread_local_crlset; On 2016/01/21 02:37:38, davidben (slow) wrote: > Hrm, that's true. We would be assuming it calls the fetching callbacks from the > same thread. Which NSS's API contract is such that that is not guaranteed. NSS, by way of NSPR, is allowed (and does) start dedicated threads for some tasks. Not for fetching, but that's not guaranteed ;) But yeah, not relevant for this CL :) https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:1423: EXPECT_EQ("F Root CA", verified_root->subject().common_name); On 2016/01/21 02:37:39, davidben (slow) wrote: > It seems these tests could do with a few helper functions to make them less > long. It's a bit difficult to read through them all. (In comparison, the tests > we recently added for alt-chains in BoringSSL are a lot easier to read. > https://boringssl.googlesource.com/boringssl/+/master/crypto/x509/x509_test.c... > ) This is strongly discouraged by our internal testing style guide. You can find it on the TOTT archives ;) https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_win.cc (right): https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:454: previous_hash->swap(subject_hash); On 2016/01/21 02:37:39, davidben (slow) wrote: > This is inconsistent with line 500's CRLSet::REVOKED which does *not* update > previous_hash. I realize it doesn't matter, but this function should be > consistent about it. I would probably go with updating it in all cases (except > when we can't and have to clear it) to avoid surprises if someone tweaks things. I agree, we shouldn't update it here, but I disagree on the "updating it in all cases" - it's intentionally trying to avoid bugs here. This is benign though. https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:472: for (unsigned j = 0; j < serial_blob->cbData; j++) On 2016/01/21 02:37:39, davidben (slow) wrote: > Nit: unsigned -> DWORD, to match cbData's type? Yeah, good point; copy pasta. https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:478: // use that; otherwise, compute it based on |issuer_cert|. On 2016/01/21 02:37:39, davidben (slow) wrote: > Nit: I would take this block and switch it with the "Compute the subject's > serial" block. It makes it more obvious why issuer_cert can't be NULL here. In > fact, you could probably fold some of the checks together like: > > std::string issuer_hash_local; > std::string* issuer_hash = previous_hash; > if (issuer_hash->empty()) { > if (!issuer_cert) { > // No issuer cert nor a hash of the issuer's SPKI was provided, > // so no further checks can be done. > previous_hash->swap(subject_hash); > return kCRLSetUnknown; > } > > der_bytes = ... > ... > } I find that (the folding) substantially less readable, because it makes the conditions harder to determine (you have to recall more state/context) https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:537: PCCERT_CONTEXT subject = elements[num_elements - i - 1]->pCertContext; On 2016/01/21 02:37:39, davidben (slow) wrote: > Optional: If you want to avoid the num_elements - i - 1 computation : > > for (DWORD i = num_elements - 1; i < num_elements; i--) { I intentionally wanted to avoid relying on "stupid unsigned int tricks" :) https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:680: PCERT_CONTEXT* cert_contexts = reinterpret_cast<PCERT_CONTEXT*>(rgpvContext); On 2016/01/21 02:37:39, davidben (slow) wrote: > Nit: newline here? Comments that aren't preceded by blank lines often hard for > me to read, but it might be just me. Agreed. https://codereview.chromium.org/1557133002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:735: if (!subject_cert) { On 2016/01/21 02:37:39, davidben (slow) wrote: > Why does this function check for NULL while CheckChainRevocationWithCRLSet > doesn't? Extra paranoia where it comes to third-party crap? Yes. CertVerifyRevocation is caller-exposed, and no fixing up of these params are done by the OS function. https://codereview.chromium.org/1557133002/diff/40001/net/data/ssl/scripts/ge... File net/data/ssl/scripts/generate-multi-root-test-chains.sh (right): https://codereview.chromium.org/1557133002/diff/40001/net/data/ssl/scripts/ge... net/data/ssl/scripts/generate-multi-root-test-chains.sh:161: # TODO(rsleevi): AKI/SPKI issues? On 2016/01/21 02:37:39, davidben (slow) wrote: > Wouldn't it be equally unnecessary for C2 to sign B? (I'm not clear on what the > comment is here for.) See line 144 for the same comment The AKI/SPKI issues are because of NSS preferences that I need to test and validate. https://codereview.chromium.org/1557133002/diff/40001/net/data/ssl/scripts/ge... net/data/ssl/scripts/generate-multi-root-test-chains.sh:199: -extensions ca_cert_with_aki \ On 2016/01/21 02:37:39, davidben (slow) wrote: > Why does this and the one below have AKI, but not the others? Blergh, this was debugging for CAPI and how it handles trust anchors, and me trying to force it to explore a J2/K2 loop, but it was too smart.
The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run
OK, excised and simplified some of the tests and test code, as well as tried to better document, as we'd suggested. Please take a look at let me know what you think.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1557133002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1557133002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
Thanks! This looks good modulo some of the platforms blowing up and a question about how well-isolated different certificate verification invocations are from each other. https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:117: for (size_t i = 0; i < N; ++i) { Optional: This could also be a for each loop if you wanted. https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:1419: int error = Verify(cert.get(), "127.0.0.1", flags, NULL, empty_cert_list_, Nit: nullptr https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:1556: EXPECT_EQ("D Root CA", verified_root->subject().common_name); Could the existence of C(E) being used in a previous test cause us to chain to E instead of D? A(B) -> B(C) -> C(E) -> E(E). Or is everything deterministic or isolated enough to always pick C(D) over C(E)? https://codereview.chromium.org/1557133002/diff/60001/net/data/ssl/scripts/ge... File net/data/ssl/scripts/generate-multi-root-test-chains.sh (right): https://codereview.chromium.org/1557133002/diff/60001/net/data/ssl/scripts/ge... net/data/ssl/scripts/generate-multi-root-test-chains.sh:40: # which are bth self-signed. Nit: bth -> both https://codereview.chromium.org/1557133002/diff/60001/net/data/ssl/scripts/ge... net/data/ssl/scripts/generate-multi-root-test-chains.sh:43: # clients are able to successfully build a valid path. Nit: "regardless of the revocation status of C or F" probably should be "as long as at least one of C or F is still valid" perhaps? https://codereview.chromium.org/1557133002/diff/60001/net/data/ssl/scripts/ge... net/data/ssl/scripts/generate-multi-root-test-chains.sh:159: # B-by-C Nit: period
https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:117: for (size_t i = 0; i < N; ++i) { On 2016/01/28 20:52:32, davidben (catching up) wrote: > Optional: This could also be a for each loop if you wanted. ? Were you thinking Go? std::for_each is awful If you meant a range-based loop (for (const auto& file : cert_files) ), that feels like an anti-pattern for something like (&cert_files)[N], due to how cert_files is implicitly castable in such a way to elide N. That is, it relies more on compiler trickery and seems less legible. I realize you mentioned it was optional, but I think I'd actually push back on code if I saw that :) https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:1556: EXPECT_EQ("D Root CA", verified_root->subject().common_name); On 2016/01/28 20:52:32, davidben (catching up) wrote: > Could the existence of C(E) being used in a previous test cause us to chain to E > instead of D? A(B) -> B(C) -> C(E) -> E(E). Or is everything deterministic or > isolated enough to always pick C(D) over C(E)? I'm not sure I understand your question. Line 1532 asserts the proper result precisely to ensure C(D) is not returned until C(E) is revoked - that's literally the cause for the complexity, to ensure that C(E) is otherwise picked first, unless it's revoked.
https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:117: for (size_t i = 0; i < N; ++i) { On 2016/02/05 21:26:03, Ryan Sleevi wrote: > On 2016/01/28 20:52:32, davidben (catching up) wrote: > > Optional: This could also be a for each loop if you wanted. > > ? Were you thinking Go? for each loops are hardly a Go innovation... > std::for_each is awful Agreed. > If you meant a range-based loop (for (const auto& file : cert_files) ), that > feels like an anti-pattern for something like (&cert_files)[N], due to how > cert_files is implicitly castable in such a way to elide N. That is, it relies > more on compiler trickery and seems less legible. > > I realize you mentioned it was optional, but I think I'd actually push back on > code if I saw that :) Yeah, I meant a range-based for loop. I think I've seen that being used on sized arrays in our code-base plenty. If the [N] got elided, I don't believe the range-based for loop would build. I disagree with you that this is trickery, but whatever. This works fine too, hence the optional. https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:1556: EXPECT_EQ("D Root CA", verified_root->subject().common_name); On 2016/02/05 21:26:03, Ryan Sleevi wrote: > On 2016/01/28 20:52:32, davidben (catching up) wrote: > > Could the existence of C(E) being used in a previous test cause us to chain to > E > > instead of D? A(B) -> B(C) -> C(E) -> E(E). Or is everything deterministic or > > isolated enough to always pick C(D) over C(E)? > > I'm not sure I understand your question. Line 1532 asserts the proper result > precisely to ensure C(D) is not returned until C(E) is revoked - that's > literally the cause for the complexity, to ensure that C(E) is otherwise picked > first, unless it's revoked. *C*(E), not F(E). This tests revokes F(E) and doesn't ever touch C(E) at all. I'm asking if any of our verifiers would introduce some non-determinism if they ran the previous test first which does use C(E).
https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:1556: EXPECT_EQ("D Root CA", verified_root->subject().common_name); On 2016/02/05 21:32:45, davidben (catching up) wrote: > *C*(E), not F(E). This tests revokes F(E) and doesn't ever touch C(E) at all. > I'm asking if any of our verifiers would introduce some non-determinism if they > ran the previous test first which does use C(E). That's hard to answer, I think, in that you're asking me to predict the future? If they did, we'd adjust to understand that cause of non-determinism and respond appropriately, and this test failing would be that signal.
https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:1556: EXPECT_EQ("D Root CA", verified_root->subject().common_name); On 2016/02/05 21:39:26, Ryan Sleevi wrote: > On 2016/02/05 21:32:45, davidben (catching up) wrote: > > *C*(E), not F(E). This tests revokes F(E) and doesn't ever touch C(E) at all. > > I'm asking if any of our verifiers would introduce some non-determinism if > they > > ran the previous test first which does use C(E). > > That's hard to answer, I think, in that you're asking me to predict the future? > If they did, we'd adjust to understand that cause of non-determinism and respond > appropriately, and this test failing would be that signal. I'm just asking if they do that right now. I agree that the test would probably notice, although it does mean an "E Root CA" check is kind of ambiguous. *shrug* Actually... why not do: EXPECT_TRUE(X509Certificate::IsSameOSCert( certs[0]->os_cert_handle(), verify_result.verified_cert->os_cert_handle())); ASSERT_EQ(3u, new_verified_intermediates.size()); EXPECT_TRUE(X509Certificate::IsSameOSCert(certs[1]->os_cert_handle(), new_verified_intermediates[0])); EXPECT_TRUE(X509Certificate::IsSameOSCert(certs[2]->os_cert_handle(), new_verified_intermediates[1])); EXPECT_TRUE(X509Certificate::IsSameOSCert(certs[3]->os_cert_handle(), new_verified_intermediates[2]));
https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1557133002/diff/60001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:1556: EXPECT_EQ("D Root CA", verified_root->subject().common_name); On 2016/02/05 21:46:48, davidben (catching up) wrote: > I'm just asking if they do that right now. I agree that the test would probably > notice, although it does mean an "E Root CA" check is kind of ambiguous. *shrug* Right, but I'm not sure that ambiguity is a problem in this test, or needs to be controlled here. I don't want to make a strong check that _prevents_ such non-determinism, I simply want to ensure that the problem path isn't found. If this test failed, it means there's non-determinism to resolve, but it could still be functioning correctly. A more detailed test would be to make sure that intermediate-1 is not F(E), which would handle if C(E) was chosen - but I *do* want to capture non-determinism if there is any remaining. I'll take this as optional, so if you wanna give your LG... :)
Oh, I didn't realize the platform-specific bits have been resolved. lgtm.
The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1557133002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1557133002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1557133002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1557133002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by rsleevi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1557133002/#ps120001 (title: "Test fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1557133002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1557133002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Perform CRLSet evaluation during Path Building on Windows On Windows, add CRLSet checking to the path building phase by registering a CryptoAPI Revocation Provider. The CRLSet is stashed in thread-local storage in order to make it from the CertVerifyProc to the Revocation Provider callback. CRLSet evaluation still happens at the end for the completed chain, but this should reduce the risk of path building errors. The Revocation Provider always returns one of two messages - unknown or revoked. It never positively asserts that a certificate is NOT revoked, in order to allow the CRL and OCSP caches to still serve as secondary sources of data. BUG=570908 TEST=TODO ========== to ========== Perform CRLSet evaluation during Path Building on Windows On Windows, add CRLSet checking to the path building phase by registering a CryptoAPI Revocation Provider. The CRLSet is stashed in thread-local storage in order to make it from the CertVerifyProc to the Revocation Provider callback. CRLSet evaluation still happens at the end for the completed chain, but this should reduce the risk of path building errors. The Revocation Provider always returns one of two messages - unknown or revoked. It never positively asserts that a certificate is NOT revoked, in order to allow the CRL and OCSP caches to still serve as secondary sources of data. BUG=570908 TEST=TODO Committed: https://crrev.com/f140b3b1a394a74efcfd2c2f59d3890a496962ac Cr-Commit-Position: refs/heads/master@{#374301} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f140b3b1a394a74efcfd2c2f59d3890a496962ac Cr-Commit-Position: refs/heads/master@{#374301} |
