Skip to content

Commit

Permalink
Revert of Perform CRLSet evaluation during Path Building on NSS (patc…
Browse files Browse the repository at this point in the history 
…hset chromium#5 id:80001 of https://codereview.chromium.org/1724413002/ )

Reason for revert:
Since this patch landed there are consistent failures on Linux valgrind.

It isn't clear to me why this change would cause the failures, but am reverting speculatively to see if the problems go away, as this is the only CL which seems close to the area.

First build with failures: https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28valgrind%29%281%29/builds/46538

Sample output:
[ RUN      ] SSLServerSocketTest.HandshakeWithClientCert
../../net/socket/ssl_server_socket_unittest.cc:389: Failure
Value of: client_ssl_config_.client_cert
  Actual: false
Expected: true
[10870:10870:0303/174222:1424458412:ERROR:ssl_server_socket_openssl.cc(649)] handshake failed; returned -1, SSL error code 1, net_error -107
[10870:10870:0303/174222:1424462684:ERROR:ssl_client_socket_openssl.cc(1140)] handshake failed; returned 0, SSL error code 1, net_error -107
../../net/socket/ssl_server_socket_unittest.cc:514: Failure
Value of: client_ret
  Actual: -107
Expected: OK
Which is: 0
[  FAILED  ] SSLServerSocketTest.HandshakeWithClientCert (242 ms)
[ RUN      ] SSLServerSocketTest.HandshakeWithClientCertRequiredNotSupplied
Received signal 11 SEGV_MAPERR 000000000148
17:42:24 common.py [INFO] process ended, did not time out
17:42:24 common.py [INFO] flushing stdout
17:42:24 common.py [INFO] collecting result code
17:42:24 common.py [ERROR] /mnt/data/b/build/slave/chromium-rel-linux-valgrind-tests-1/build/src/third_party/valgrind/linux_x64/bin/valgrind exited with non-zero result code -9
-----------------------------------------------------
Suppressions used:
  count name
      1 bug_269278b
      2 glibc-2.5.x-on-SUSE-10.2-(PPC)-2a
      6 bug_176891a
      9 bug_87500_a (Intentional)
     26 bug_32624_c
   1639 bug_64887_a
-----------------------------------------------------
17:42:25 memcheck_analyze.py [ERROR] FAIL! There were 1 errors:
17:42:25 memcheck_analyze.py [ERROR]
### BEGIN MEMORY TOOL REPORT (error hash=#A693C36946E9BE71#)
Command: /mnt/data/b/build/slave/chromium-rel-linux-valgrind-tests-1/build/src/out/Release/net_unittests --gtest_print_time --single-process-tests --gtest_filter=-Spdy_SpdyNetworkTransactionTest.StartTransactionOnReadCallback_0:DiskCacheEntryTest.SimpleCacheSizeChanges:DiskCacheBackendTest.FAILS_AppCacheInvalidEntry:DiskCacheBackendTest.FAILS_NewEvictionInvalidEntryWithLoad:KeygenHandlerTest.*ConcurrencyTest:DiskCacheBackendTest.NewEvictionTrimInvalidEntry:KeygenHandlerTest.FLAKY_*SmokeTest:DiskCacheBackendTest.InvalidEntryRead:DiskCacheBackendTest.FLAKY_NewEvictionInvalidEntryEnumeration:DiskCacheBackendTest.FLAKY_InvalidEntryWithLoad:DiskCacheBackendTest.FLAKY_NewEvictionInvalidEntryWithLoad:DiskCacheBackendTest.FAILS_ShutdownWithPendingCreate_Fast:DiskCacheEntryTest.FLAKY_SimpleCacheSizeChanges:KeygenHandlerTest.FAILS_*ConcurrencyTest:KeygenHandlerTest.FAILS_*SmokeTest:DiskCacheEntryTest.FAILS_SimpleCacheSizeChanges:DiskCacheBackendTest.NewEvictionInvalidEntryEnumeration:DiskCacheBackendTest.FLAKY_AppCacheInvalidEntry:DiskCacheBackendTest.FLAKY_NewEvictionTrimInvalidEntry2:DiskCacheBackendTest.AppCacheInvalidEntryWithLoad:DiskCacheBackendTest.FAILS_AppCacheInvalidEntryRead:DiskCacheBackendTest.FLAKY_InvalidEntryEnumeration:DiskCacheBackendTest.FLAKY_NewEvictionInvalidEntryRead:DiskCacheBackendTest.ShutdownWithPendingIO_Fast:DiskCacheBackendTest.FLAKY_AppCacheInvalidEntryWithLoad:DiskCacheBackendTest.FAILS_ShutdownWithPendingIO_Fast:DiskCacheBackendTest.ShutdownWithPendingCreate_Fast:DiskCacheBackendTest.FAILS_InvalidEntryEnumeration:DiskCacheBackendTest.FAILS_ShutdownWithPendingFileIO_Fast:DiskCacheBackendTest.FAILS_AppCacheInvalidEntryWithLoad:DiskCacheBackendTest.FLAKY_InvalidEntryRead:DiskCacheBackendTest.FLAKY_ShutdownWithPendingFileIO_Fast:DiskCacheBackendTest.NewEvictionInvalidEntryRead:KeygenHandlerTest.FLAKY_*ConcurrencyTest:DiskCacheBackendTest.FAILS_InvalidEntryRead:Spdy_SpdyNetworkTransactionTest.FLAKY_StartTransactionOnReadCallback_0:Spdy_SpdyNetworkTransactionTest.FAILS_StartTransactionOnReadCallback_0:KeygenHandlerTest.*SmokeTest:DiskCacheBackendTest.FAILS_InvalidEntry:EndToEndTests/EndToEndTest.*:DirectoryListerTest.FAILS_BigDirRecursiveTest:DiskCacheBackendTest.AppCacheInvalidEntryRead:DiskCacheBackendTest.FAILS_InvalidEntryWithLoad:DiskCacheBackendTest.ShutdownWithPendingFileIO_Fast:DiskCacheBackendTest.FAILS_NewEvictionInvalidEntryRead:DiskCacheEntryTest.SimpleCacheGrowData:DiskCacheBackendTest.FAILS_NewEvictionTrimInvalidEntry:DiskCacheBackendTest.FLAKY_ShutdownWithPendingCreate_Fast:DiskCacheBackendTest.FAILS_TrimInvalidEntry:DiskCacheEntryTest.FLAKY_SimpleCacheGrowData:DiskCacheBackendTest.FAILS_NewEvictionInvalidEntryEnumeration:DiskCacheBackendTest.NewEvictionInvalidEntryWithLoad:DiskCacheBackendTest.FAILS_NewEvictionInvalidEntry:DiskCacheBackendTest.FLAKY_ShutdownWithPendingIO_Fast:DirectoryListerTest.FLAKY_BigDirRecursiveTest:DiskCacheBackendTest.TrimInvalidEntry2:DiskCacheBackendTest.TrimInvalidEntry:DiskCacheBackendTest.NewEvictionInvalidEntry:DiskCacheBackendTest.FLAKY_TrimInvalidEntry2:DiskCacheEntryTest.SimpleCacheStreamAccess:DiskCacheEntryTest.FLAKY_SimpleCacheStreamAccess:DiskCacheBackendTest.InvalidEntryWithLoad:DiskCacheEntryTest.FAILS_SimpleCacheGrowData:DiskCacheEntryTest.FAILS_SimpleCacheStreamAccess:DiskCacheBackendTest.FLAKY_NewEvictionTrimInvalidEntry:DiskCacheBackendTest.FAILS_NewEvictionTrimInvalidEntry2:DiskCacheBackendTest.InvalidEntry:DiskCacheBackendTest.NewEvictionTrimInvalidEntry2:DiskCacheBackendTest.FAILS_TrimInvalidEntry2:DiskCacheBackendTest.FLAKY_InvalidEntry:DiskCacheBackendTest.FLAKY_AppCacheInvalidEntryRead:DiskCacheBackendTest.InvalidEntryEnumeration:DirectoryListerTest.BigDirRecursiveTest:DiskCacheBackendTest.FLAKY_NewEvictionInvalidEntry:DiskCacheBackendTest.AppCacheInvalidEntry:DiskCacheBackendTest.FLAKY_TrimInvalidEntry --test-tiny-timeout=1000
InvalidRead
Invalid read of size 8
  __gnu_cxx::new_allocator<CERTCertificateStr*>::construct(CERTCertificateStr**, CERTCertificateStr* const&) (/mnt/data/b/build/slave/chromium-rel-linux-valgrind-tests-1/build/src/out/Release/net_unittests)
  _ZNSt6vectorIP18CERTCertificateStrSaIS1_EE13_M_insert_auxIJRKS1_EEEvN9__gnu_cxx17__normal_iteratorIPS1_S3_EEDpOT_ (build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/vector.tcc:335)
  std::vector<CERTCertificateStr*, std::allocator<CERTCertificateStr*> >::push_back(CERTCertificateStr* const&) (build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_vector.h:834)
  net::X509Certificate::IsIssuedByEncoded(std::vector<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) (net/cert/x509_certificate_nss.cc:128)
  net::SSLServerSocketTest_HandshakeWithClientCertRequiredNotSupplied_Test::TestBody() (net/socket/ssl_server_socket_unittest.cc:550)
Address 0x148 is not stack'd, malloc'd or (recently) free'd
Suppression (error hash=#A693C36946E9BE71#):
  For more info on using suppressions see http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/memory-sheriff#TOC-Suppressing-memory-reports
{
   <insert_a_suppression_name_here>
   Memcheck:Unaddressable
   fun:_ZN9__gnu_cxx13new_allocatorIP18CERTCertificateStrE9constructEPS2_RKS2_
   fun:_ZNSt6vectorIP18CERTCertificateStrSaIS1_EE13_M_insert_auxIJRKS1_EEEvN9__gnu_cxx17__normal_iteratorIPS1_S3_EEDpOT_
   fun:_ZNSt6vectorIP18CERTCertificateStrSaIS1_EE9push_backERKS1_
   fun:_ZN3net15X509Certificate17IsIssuedByEncodedERKSt6vectorISsSaISsEE
   fun:_ZN3net67SSLServerSocketTest_HandshakeWithClientCertRequiredNotSupplied_Test8TestBodyEv
}

Original issue's description:
> Perform CRLSet evaluation during Path Building on NSS
>
> When using NSS for certificate verification, add CRLSet checking by
> injecting a revocation callback function which will examine the
> CRLSet and reject the certificate. If the CRLSet does not
> affirmatively reject it, continue invoking the originally supplied
> application callback (such as the ChromeOS callback) and allow it
> an opportunity to reject.
>
> Because of how NSS caches virtually everything, horribly so, this
> restructures the unittests to no longer depend on how the underlying
> library will select the path (since with NSS, it's fundamentally
> non-determistic), and instead tests that as long as a singular
> certificate path is still valid and un-revoked, it can be discovered.
>
> BUG=589336
> TEST=CertVerifyProcTest.CRLSet*
>
> Committed: https://crrev.com/c45d7cce9017369c36ecbe3ed2d4567eea786f24
> Cr-Commit-Position: refs/heads/master@{#379113}

TBR=eroman@chromium.org,rsleevi@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=589336

Review URL: https://codereview.chromium.org/1762923002

Cr-Commit-Position: refs/heads/master@{#379219}
  • Loading branch information
benwells authored and Commit bot committed Mar 4, 2016
1 parent dd0212b commit cbfcd90
Show file tree
Hide file tree
Showing 8 changed files with 227 additions and 220 deletions.
119 changes: 33 additions & 86 deletions net/cert/cert_verify_proc_nss.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ enum CRLSetResult {
// that some EV sites would otherwise take the hit of an OCSP lookup for
// no reason.
// kCRLSetOk: otherwise.
CRLSetResult CheckRevocationWithCRLSet(const CERTCertList* cert_list,
CRLSetResult CheckRevocationWithCRLSet(CERTCertList* cert_list,
CERTCertificate* root,
CRLSet* crl_set) {
std::vector<CERTCertificate*> certs;
Expand Down Expand Up @@ -352,50 +352,6 @@ CRLSetResult CheckRevocationWithCRLSet(const CERTCertList* cert_list,
return kCRLSetOk;
}

// Arguments for CheckChainRevocationWithCRLSet that are curried within the
// CERTChainVerifyCallback's isChainValidArg.
struct CheckChainRevocationArgs {
// The CRLSet to evaluate against.
CRLSet* crl_set = nullptr;

// The next callback to invoke, if the CRLSet does not report any errors.
CERTChainVerifyCallback* next_callback = nullptr;

// Indicates that the application callback failure was due to a CRLSet
// revocation, rather than due to |next_callback| rejecting it. This is
// used to map the error back to the proper caller-visible error code.
bool was_revoked = false;
};

SECStatus CheckChainRevocationWithCRLSet(void* is_chain_valid_arg,
const CERTCertList* current_chain,
PRBool* chain_ok) {
CHECK(is_chain_valid_arg);

CheckChainRevocationArgs* args =
static_cast<CheckChainRevocationArgs*>(is_chain_valid_arg);

CRLSetResult crlset_result = kCRLSetUnknown;
if (args->crl_set) {
crlset_result =
CheckRevocationWithCRLSet(current_chain, nullptr, args->crl_set);
}

if (crlset_result == kCRLSetRevoked) {
args->was_revoked = true;
*chain_ok = PR_FALSE;
return SECSuccess;
}
args->was_revoked = false;

*chain_ok = PR_TRUE;
if (!args->next_callback || !args->next_callback->isChainValid)
return SECSuccess;

return (*args->next_callback->isChainValid)(
args->next_callback->isChainValidArg, current_chain, chain_ok);
}

// Forward declarations.
SECStatus RetryPKIXVerifyCertWithWorkarounds(
CERTCertificate* cert_handle, int num_policy_oids,
Expand Down Expand Up @@ -869,22 +825,6 @@ int CertVerifyProcNSS::VerifyInternalImpl(
verify_result->cert_status |= CERT_STATUS_COMMON_NAME_INVALID;
}

// Setup a callback to call into CheckChainRevocationWithCRLSet with the
// current CRLSet. If the CRLSet revokes a given chain, |was_revoked|
// will be set to true.
// The same callback and args are used for every invocation of
// PKIXVerifyCert, as CheckChainRevocationWithCRLSet handles resetting
// |was_revoked| as necessary.
CheckChainRevocationArgs check_chain_revocation_args;
check_chain_revocation_args.crl_set = crl_set;
check_chain_revocation_args.next_callback = chain_verify_callback;

CERTChainVerifyCallback crlset_callback;
memset(&crlset_callback, 0, sizeof(crlset_callback));
crlset_callback.isChainValid = &CheckChainRevocationWithCRLSet;
crlset_callback.isChainValidArg =
static_cast<void*>(&check_chain_revocation_args);

// Make sure that the cert is valid now.
SECCertTimeValidity validity = CERT_CheckCertValidTimes(
cert_handle, PR_Now(), PR_TRUE);
Expand Down Expand Up @@ -922,9 +862,15 @@ int CertVerifyProcNSS::VerifyInternalImpl(
CertificateListToCERTCertList(additional_trust_anchors));
}

SECStatus status =
PKIXVerifyCert(cert_handle, check_revocation, false, cert_io_enabled,
NULL, 0, trust_anchors.get(), &crlset_callback, cvout);
SECStatus status = PKIXVerifyCert(cert_handle,
check_revocation,
false,
cert_io_enabled,
NULL,
0,
trust_anchors.get(),
chain_verify_callback,
cvout);

if (status == SECSuccess &&
(flags & CertVerifier::VERIFY_REV_CHECKING_REQUIRED_LOCAL_ANCHORS) &&
Expand All @@ -934,8 +880,15 @@ int CertVerifyProcNSS::VerifyInternalImpl(
// NSS tests for that feature.
scoped_cvout.Clear();
verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
status = PKIXVerifyCert(cert_handle, true, true, cert_io_enabled, NULL, 0,
trust_anchors.get(), &crlset_callback, cvout);
status = PKIXVerifyCert(cert_handle,
true,
true,
cert_io_enabled,
NULL,
0,
trust_anchors.get(),
chain_verify_callback,
cvout);
}

if (status == SECSuccess) {
Expand All @@ -957,25 +910,13 @@ int CertVerifyProcNSS::VerifyInternalImpl(

CRLSetResult crl_set_result = kCRLSetUnknown;
if (crl_set) {
if (status == SECSuccess) {
// Reverify the returned chain; NSS should have already called
// CheckChainRevocationWithCRLSet prior to returning, but given the
// edge cases (self-signed certs that are trusted; cached chains;
// unreadable code), this is more about defense in depth than
// functional necessity.
crl_set_result = CheckRevocationWithCRLSet(
cvout[cvout_cert_list_index].value.pointer.chain,
cvout[cvout_trust_anchor_index].value.pointer.cert, crl_set);
if (crl_set_result == kCRLSetRevoked) {
PORT_SetError(SEC_ERROR_REVOKED_CERTIFICATE);
status = SECFailure;
}
} else if (PORT_GetError() == SEC_ERROR_APPLICATION_CALLBACK_ERROR &&
check_chain_revocation_args.was_revoked) {
// If a CRLSet was supplied, and the error was an application callback
// error, then it was directed through the CRLSet code and that
// particular chain was revoked.
crl_set_result = CheckRevocationWithCRLSet(
cvout[cvout_cert_list_index].value.pointer.chain,
cvout[cvout_trust_anchor_index].value.pointer.cert,
crl_set);
if (crl_set_result == kCRLSetRevoked) {
PORT_SetError(SEC_ERROR_REVOKED_CERTIFICATE);
status = SECFailure;
}
}

Expand Down Expand Up @@ -1008,8 +949,14 @@ int CertVerifyProcNSS::VerifyInternalImpl(
if (check_revocation)
verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;

if (VerifyEV(cert_handle, flags, crl_set, check_revocation, metadata,
ev_policy_oid, trust_anchors.get(), &crlset_callback)) {
if (VerifyEV(cert_handle,
flags,
crl_set,
check_revocation,
metadata,
ev_policy_oid,
trust_anchors.get(),
chain_verify_callback)) {
verify_result->cert_status |= CERT_STATUS_IS_EV;
}
}
Expand Down
Loading

0 comments on commit cbfcd90

Please sign in to comment.