[PATCH 0/6] MR7117: crypt32: Some fixes for verify_ssl_policy().
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/tests/chain.c | 133 +++++++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/dlls/crypt32/tests/chain.c b/dlls/crypt32/tests/chain.c index 9717b184c14..d198b92a31c 100644 --- a/dlls/crypt32/tests/chain.c +++ b/dlls/crypt32/tests/chain.c @@ -5376,10 +5376,143 @@ static void testVerifyCertChainPolicy(void) check_msroot_policy(); } +static void test_VerifyCertChainPolicy_flags(void) +{ + static const struct + { + DWORD trust_status; + unsigned int index; + DWORD policy_flags; + DWORD ssl_policy_flags; + DWORD expected_error; + BOOL wine_todo; + } + tests[] = + { + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, 0, 0, CRYPT_E_REVOCATION_OFFLINE, TRUE }, + /* CERT_TRUST_REVOCATION_STATUS_UNKNOWN is only cheked on the end certificate. */ + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 1, 0, 0, ERROR_SUCCESS, TRUE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 2, 0, 0, ERROR_SUCCESS, TRUE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_END_REV_UNKNOWN_FLAG, 0, 0, TRUE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_CTL_SIGNER_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE, TRUE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_CA_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE, TRUE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_ROOT_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE, TRUE }, + /* CERT_TRUST_IS_OFFLINE_REVOCATION is ignored. */ + { CERT_TRUST_IS_OFFLINE_REVOCATION, 0, 0, 0, ERROR_SUCCESS, TRUE }, + { CERT_TRUST_IS_OFFLINE_REVOCATION, 1, 0, 0, ERROR_SUCCESS, TRUE }, + { CERT_TRUST_IS_OFFLINE_REVOCATION, 2, 0, 0, ERROR_SUCCESS, TRUE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 0, 0, 0, CRYPT_E_REVOCATION_OFFLINE, TRUE }, + /* CERT_TRUST_REVOCATION_STATUS_UNKNOWN is only cheked on the end certificate. */ + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 1, 0, 0, ERROR_SUCCESS }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 2, 0, 0, ERROR_SUCCESS }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 0, CERT_CHAIN_POLICY_IGNORE_END_REV_UNKNOWN_FLAG, 0, 0 }, + + { CERT_TRUST_IS_REVOKED, 0, 0, 0, CRYPT_E_REVOKED, TRUE }, + { CERT_TRUST_IS_REVOKED, 1, 0, 0, CRYPT_E_REVOKED, TRUE }, + { CERT_TRUST_IS_REVOKED, 2, 0, 0, CRYPT_E_REVOKED, TRUE }, + + { CERT_TRUST_IS_NOT_VALID_FOR_USAGE, 0, 0, 0, CERT_E_WRONG_USAGE }, + { CERT_TRUST_IS_NOT_VALID_FOR_USAGE, 1, 0, 0, CERT_E_WRONG_USAGE }, + { CERT_TRUST_IS_NOT_VALID_FOR_USAGE, 2, 0, 0, CERT_E_WRONG_USAGE }, + { CERT_TRUST_IS_NOT_VALID_FOR_USAGE, 0, 0, SECURITY_FLAG_IGNORE_WRONG_USAGE, ERROR_SUCCESS }, + { CERT_TRUST_IS_NOT_VALID_FOR_USAGE, 0, CERT_CHAIN_POLICY_IGNORE_WRONG_USAGE_FLAG, 0, ERROR_SUCCESS, TRUE }, + + { CERT_TRUST_IS_SELF_SIGNED, 0, 0, 0, TRUST_E_CERT_SIGNATURE }, + { CERT_TRUST_IS_SELF_SIGNED, 1, 0, 0, TRUST_E_CERT_SIGNATURE }, + { CERT_TRUST_IS_SELF_SIGNED, 2, 0, 0, TRUST_E_CERT_SIGNATURE }, + { CERT_TRUST_IS_SELF_SIGNED, 2, 0, SECURITY_FLAG_IGNORE_UNKNOWN_CA, TRUST_E_CERT_SIGNATURE }, + { CERT_TRUST_IS_SELF_SIGNED, 2, CERT_CHAIN_POLICY_ALLOW_UNKNOWN_CA_FLAG, 0, TRUST_E_CERT_SIGNATURE }, + }; + + BOOL ret; + PCCERT_CONTEXT cert; + CERT_CHAIN_PARA para = { 0 }; + PCCERT_CHAIN_CONTEXT chain; + FILETIME fileTime; + HCERTSTORE store; + static char one_two_three[] = "1.2.3"; + LPSTR oids[1]; + SSL_EXTRA_CERT_CHAIN_POLICY_PARA ssl_para; + CERT_CHAIN_POLICY_PARA policy_para; + CERT_CHAIN_POLICY_STATUS status; + CERT_REVOCATION_INFO rev_info[3]; + //CERT_REVOCATION_CRL_INFO crl_info; + unsigned int i; + + store = CertOpenStore(CERT_STORE_PROV_MEMORY, 0, 0, CERT_STORE_CREATE_NEW_FLAG, NULL); + CertAddEncodedCertificateToStore(store, X509_ASN_ENCODING, geotrust_global_ca, + sizeof(geotrust_global_ca), CERT_STORE_ADD_ALWAYS, NULL); + CertAddEncodedCertificateToStore(store, X509_ASN_ENCODING, google_internet_authority, + sizeof(google_internet_authority), CERT_STORE_ADD_ALWAYS, NULL); + cert = CertCreateCertificateContext(X509_ASN_ENCODING, google_com, sizeof(google_com)); + SystemTimeToFileTime(&oct2009, &fileTime); + memset(¶, 0, sizeof(para)); + para.cbSize = sizeof(para); + oids[0] = one_two_three; + para.RequestedUsage.dwType = USAGE_MATCH_TYPE_AND; + para.RequestedUsage.Usage.rgpszUsageIdentifier = oids; + para.RequestedUsage.Usage.cUsageIdentifier = 1; + ret = CertGetCertificateChain(NULL, cert, &fileTime, store, ¶, CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY, NULL, &chain); + ok(ret, "got error %#lx.\n", GetLastError()); + ok(chain->cChain == 1, "got %lu.\n", chain->cChain); + ok(chain->rgpChain[0]->cElement == 3, "got %lu.\n", chain->rgpChain[0]->cElement); + + memset(&policy_para, 0, sizeof(policy_para)); + policy_para.cbSize = sizeof(policy_para); + memset(&ssl_para, 0, sizeof(ssl_para)); + ssl_para.cbSize = sizeof(ssl_para); + ssl_para.dwAuthType = AUTHTYPE_SERVER; + ssl_para.pwszServerName = (WCHAR *)L"www.google.com"; + policy_para.pvExtraPolicyPara = &ssl_para; + status.cbSize = sizeof(status); + + for (i = 0; i < chain->rgpChain[0]->cElement; ++i) + { + chain->rgpChain[0]->rgpElement[i]->TrustStatus.dwErrorStatus = 0; + memset(&rev_info[i], 0, sizeof(rev_info[i])); + rev_info[i].cbSize = sizeof(rev_info); + chain->rgpChain[0]->rgpElement[i]->pRevocationInfo = &rev_info[i]; + } + + for (i = 0; i < ARRAY_SIZE(tests); ++i) + { + winetest_push_context("test %u", i); + *(DWORD *)&chain->TrustStatus.dwErrorStatus = tests[i].trust_status;//CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION;//CERT_TRUST_IS_REVOKED;//CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION; + chain->rgpChain[0]->TrustStatus.dwErrorStatus = chain->TrustStatus.dwErrorStatus; + chain->rgpChain[0]->rgpElement[tests[i].index]->TrustStatus.dwErrorStatus = chain->TrustStatus.dwErrorStatus; + policy_para.dwFlags = tests[i].policy_flags; + ssl_para.cbSize = sizeof(ssl_para); + ssl_para.fdwChecks = tests[i].ssl_policy_flags; + policy_para.pvExtraPolicyPara = &ssl_para; + ret = CertVerifyCertificateChainPolicy(CERT_CHAIN_POLICY_SSL, chain, &policy_para, &status); + ok(ret, "got error %#lx.\n", GetLastError()); + todo_wine_if(tests[i].wine_todo) ok(status.dwError == tests[i].expected_error, "got %#lx, expected %#lx.\n", status.dwError, tests[i].expected_error); + if (status.dwError) + { + ok(!status.lChainIndex, "got %ld.\n", status.lChainIndex); + ok(status.lElementIndex == tests[i].index, "got %ld.\n", status.lElementIndex); + } + else + { + ok(status.lChainIndex == -1, "got %ld.\n", status.lChainIndex); + ok(status.lElementIndex == -1, "got %ld.\n", status.lElementIndex); + } + chain->rgpChain[0]->rgpElement[tests[i].index]->TrustStatus.dwErrorStatus = 0; + winetest_pop_context(); + } + for (i = 0; i < chain->rgpChain[0]->cElement; ++i) + chain->rgpChain[0]->rgpElement[i]->pRevocationInfo = NULL; + + CertFreeCertificateChain(chain); + CertFreeCertificateContext(cert); + CertCloseStore(store, 0); +} + START_TEST(chain) { testCreateCertChainEngine(); testVerifyCertChainPolicy(); testGetCertChain(); test_CERT_CHAIN_PARA_cbSize(); + test_VerifyCertChainPolicy_flags(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7117
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/chain.c | 4 ++-- dlls/crypt32/tests/chain.c | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/dlls/crypt32/chain.c b/dlls/crypt32/chain.c index 5e4288eda37..b7072a2f8b4 100644 --- a/dlls/crypt32/chain.c +++ b/dlls/crypt32/chain.c @@ -3513,7 +3513,7 @@ static BOOL WINAPI verify_ssl_policy(LPCSTR szPolicyOID, else if (pChainContext->TrustStatus.dwErrorStatus & CERT_TRUST_IS_REVOKED && !(checks & SECURITY_FLAG_IGNORE_REVOCATION)) { - pPolicyStatus->dwError = CERT_E_REVOKED; + pPolicyStatus->dwError = CRYPT_E_REVOKED; find_element_with_error(pChainContext, CERT_TRUST_IS_REVOKED, &pPolicyStatus->lChainIndex, &pPolicyStatus->lElementIndex); @@ -3522,7 +3522,7 @@ static BOOL WINAPI verify_ssl_policy(LPCSTR szPolicyOID, CERT_TRUST_IS_OFFLINE_REVOCATION && !(checks & SECURITY_FLAG_IGNORE_REVOCATION)) { - pPolicyStatus->dwError = CERT_E_REVOCATION_FAILURE; + pPolicyStatus->dwError = CRYPT_E_REVOCATION_OFFLINE; find_element_with_error(pChainContext, CERT_TRUST_IS_OFFLINE_REVOCATION, &pPolicyStatus->lChainIndex, &pPolicyStatus->lElementIndex); diff --git a/dlls/crypt32/tests/chain.c b/dlls/crypt32/tests/chain.c index d198b92a31c..db3c3f36228 100644 --- a/dlls/crypt32/tests/chain.c +++ b/dlls/crypt32/tests/chain.c @@ -5389,14 +5389,14 @@ static void test_VerifyCertChainPolicy_flags(void) } tests[] = { - { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, 0, 0, CRYPT_E_REVOCATION_OFFLINE, TRUE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, 0, 0, CRYPT_E_REVOCATION_OFFLINE }, /* CERT_TRUST_REVOCATION_STATUS_UNKNOWN is only cheked on the end certificate. */ { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 1, 0, 0, ERROR_SUCCESS, TRUE }, { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 2, 0, 0, ERROR_SUCCESS, TRUE }, { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_END_REV_UNKNOWN_FLAG, 0, 0, TRUE }, - { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_CTL_SIGNER_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE, TRUE }, - { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_CA_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE, TRUE }, - { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_ROOT_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE, TRUE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_CTL_SIGNER_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_CA_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_ROOT_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE }, /* CERT_TRUST_IS_OFFLINE_REVOCATION is ignored. */ { CERT_TRUST_IS_OFFLINE_REVOCATION, 0, 0, 0, ERROR_SUCCESS, TRUE }, { CERT_TRUST_IS_OFFLINE_REVOCATION, 1, 0, 0, ERROR_SUCCESS, TRUE }, @@ -5407,9 +5407,9 @@ static void test_VerifyCertChainPolicy_flags(void) { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 2, 0, 0, ERROR_SUCCESS }, { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 0, CERT_CHAIN_POLICY_IGNORE_END_REV_UNKNOWN_FLAG, 0, 0 }, - { CERT_TRUST_IS_REVOKED, 0, 0, 0, CRYPT_E_REVOKED, TRUE }, - { CERT_TRUST_IS_REVOKED, 1, 0, 0, CRYPT_E_REVOKED, TRUE }, - { CERT_TRUST_IS_REVOKED, 2, 0, 0, CRYPT_E_REVOKED, TRUE }, + { CERT_TRUST_IS_REVOKED, 0, 0, 0, CRYPT_E_REVOKED }, + { CERT_TRUST_IS_REVOKED, 1, 0, 0, CRYPT_E_REVOKED }, + { CERT_TRUST_IS_REVOKED, 2, 0, 0, CRYPT_E_REVOKED }, { CERT_TRUST_IS_NOT_VALID_FOR_USAGE, 0, 0, 0, CERT_E_WRONG_USAGE }, { CERT_TRUST_IS_NOT_VALID_FOR_USAGE, 1, 0, 0, CERT_E_WRONG_USAGE }, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7117
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/chain.c | 8 +++----- dlls/crypt32/tests/chain.c | 14 +++++++------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/dlls/crypt32/chain.c b/dlls/crypt32/chain.c index b7072a2f8b4..fb2f391ff6e 100644 --- a/dlls/crypt32/chain.c +++ b/dlls/crypt32/chain.c @@ -2707,6 +2707,7 @@ static void CRYPT_VerifyChainRevocation(PCERT_CHAIN_CONTEXT chain, switch (revocationStatus.dwError) { + case CRYPT_E_REVOCATION_OFFLINE: case CRYPT_E_NO_REVOCATION_CHECK: case CRYPT_E_NO_REVOCATION_DLL: case CRYPT_E_NOT_IN_REVOCATION_DATABASE: @@ -2716,9 +2717,6 @@ static void CRYPT_VerifyChainRevocation(PCERT_CHAIN_CONTEXT chain, error = CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION; break; - case CRYPT_E_REVOCATION_OFFLINE: - error = CERT_TRUST_IS_OFFLINE_REVOCATION; - break; case CRYPT_E_REVOKED: error = CERT_TRUST_IS_REVOKED; break; @@ -3519,12 +3517,12 @@ static BOOL WINAPI verify_ssl_policy(LPCSTR szPolicyOID, &pPolicyStatus->lElementIndex); } else if (pChainContext->TrustStatus.dwErrorStatus & - CERT_TRUST_IS_OFFLINE_REVOCATION && + CERT_TRUST_REVOCATION_STATUS_UNKNOWN && !(checks & SECURITY_FLAG_IGNORE_REVOCATION)) { pPolicyStatus->dwError = CRYPT_E_REVOCATION_OFFLINE; find_element_with_error(pChainContext, - CERT_TRUST_IS_OFFLINE_REVOCATION, &pPolicyStatus->lChainIndex, + CERT_TRUST_REVOCATION_STATUS_UNKNOWN, &pPolicyStatus->lChainIndex, &pPolicyStatus->lElementIndex); } else if (pChainContext->TrustStatus.dwErrorStatus & diff --git a/dlls/crypt32/tests/chain.c b/dlls/crypt32/tests/chain.c index db3c3f36228..f4f5a69698f 100644 --- a/dlls/crypt32/tests/chain.c +++ b/dlls/crypt32/tests/chain.c @@ -5398,14 +5398,14 @@ static void test_VerifyCertChainPolicy_flags(void) { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_CA_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE }, { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_ROOT_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE }, /* CERT_TRUST_IS_OFFLINE_REVOCATION is ignored. */ - { CERT_TRUST_IS_OFFLINE_REVOCATION, 0, 0, 0, ERROR_SUCCESS, TRUE }, - { CERT_TRUST_IS_OFFLINE_REVOCATION, 1, 0, 0, ERROR_SUCCESS, TRUE }, - { CERT_TRUST_IS_OFFLINE_REVOCATION, 2, 0, 0, ERROR_SUCCESS, TRUE }, - { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 0, 0, 0, CRYPT_E_REVOCATION_OFFLINE, TRUE }, + { CERT_TRUST_IS_OFFLINE_REVOCATION, 0, 0, 0, ERROR_SUCCESS }, + { CERT_TRUST_IS_OFFLINE_REVOCATION, 1, 0, 0, ERROR_SUCCESS }, + { CERT_TRUST_IS_OFFLINE_REVOCATION, 2, 0, 0, ERROR_SUCCESS }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 0, 0, 0, CRYPT_E_REVOCATION_OFFLINE }, /* CERT_TRUST_REVOCATION_STATUS_UNKNOWN is only cheked on the end certificate. */ - { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 1, 0, 0, ERROR_SUCCESS }, - { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 2, 0, 0, ERROR_SUCCESS }, - { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 0, CERT_CHAIN_POLICY_IGNORE_END_REV_UNKNOWN_FLAG, 0, 0 }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 1, 0, 0, ERROR_SUCCESS, TRUE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 2, 0, 0, ERROR_SUCCESS, TRUE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 0, CERT_CHAIN_POLICY_IGNORE_END_REV_UNKNOWN_FLAG, 0, 0, TRUE }, { CERT_TRUST_IS_REVOKED, 0, 0, 0, CRYPT_E_REVOKED }, { CERT_TRUST_IS_REVOKED, 1, 0, 0, CRYPT_E_REVOKED }, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7117
From: Paul Gofman <pgofman(a)codeweavers.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56559 --- dlls/crypt32/chain.c | 2 +- dlls/crypt32/tests/chain.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/crypt32/chain.c b/dlls/crypt32/chain.c index fb2f391ff6e..61f9e66f30c 100644 --- a/dlls/crypt32/chain.c +++ b/dlls/crypt32/chain.c @@ -3518,7 +3518,7 @@ static BOOL WINAPI verify_ssl_policy(LPCSTR szPolicyOID, } else if (pChainContext->TrustStatus.dwErrorStatus & CERT_TRUST_REVOCATION_STATUS_UNKNOWN && - !(checks & SECURITY_FLAG_IGNORE_REVOCATION)) + !(checks & SECURITY_FLAG_IGNORE_REVOCATION) && !(baseChecks & CERT_CHAIN_POLICY_IGNORE_END_REV_UNKNOWN_FLAG)) { pPolicyStatus->dwError = CRYPT_E_REVOCATION_OFFLINE; find_element_with_error(pChainContext, diff --git a/dlls/crypt32/tests/chain.c b/dlls/crypt32/tests/chain.c index f4f5a69698f..9bf5f890167 100644 --- a/dlls/crypt32/tests/chain.c +++ b/dlls/crypt32/tests/chain.c @@ -5393,7 +5393,7 @@ static void test_VerifyCertChainPolicy_flags(void) /* CERT_TRUST_REVOCATION_STATUS_UNKNOWN is only cheked on the end certificate. */ { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 1, 0, 0, ERROR_SUCCESS, TRUE }, { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 2, 0, 0, ERROR_SUCCESS, TRUE }, - { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_END_REV_UNKNOWN_FLAG, 0, 0, TRUE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_END_REV_UNKNOWN_FLAG, 0, 0 }, { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_CTL_SIGNER_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE }, { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_CA_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE }, { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_ROOT_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE }, @@ -5405,7 +5405,7 @@ static void test_VerifyCertChainPolicy_flags(void) /* CERT_TRUST_REVOCATION_STATUS_UNKNOWN is only cheked on the end certificate. */ { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 1, 0, 0, ERROR_SUCCESS, TRUE }, { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 2, 0, 0, ERROR_SUCCESS, TRUE }, - { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 0, CERT_CHAIN_POLICY_IGNORE_END_REV_UNKNOWN_FLAG, 0, 0, TRUE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 0, CERT_CHAIN_POLICY_IGNORE_END_REV_UNKNOWN_FLAG, 0, 0 }, { CERT_TRUST_IS_REVOKED, 0, 0, 0, CRYPT_E_REVOKED }, { CERT_TRUST_IS_REVOKED, 1, 0, 0, CRYPT_E_REVOKED }, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7117
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/chain.c | 25 +++++++++++++++++++++---- dlls/crypt32/tests/chain.c | 8 ++++---- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/dlls/crypt32/chain.c b/dlls/crypt32/chain.c index 61f9e66f30c..e8dc15ff7e9 100644 --- a/dlls/crypt32/chain.c +++ b/dlls/crypt32/chain.c @@ -2989,6 +2989,24 @@ static void find_element_with_error(PCCERT_CHAIN_CONTEXT chain, DWORD error, } } +static BOOL find_chain_first_element_with_error(PCCERT_CHAIN_CONTEXT chain, DWORD error, LONG *chain_idx, + LONG *element_idx) +{ + unsigned int i; + + for (i = 0; i < chain->cChain; i++) + { + if (!chain->rgpChain[i]->cElement) continue; + if (chain->rgpChain[i]->rgpElement[0]->TrustStatus.dwErrorStatus & error) + { + *chain_idx = i; + *element_idx = 0; + return TRUE; + } + } + return FALSE; +} + static BOOL WINAPI verify_base_policy(LPCSTR szPolicyOID, PCCERT_CHAIN_CONTEXT pChainContext, PCERT_CHAIN_POLICY_PARA pPolicyPara, PCERT_CHAIN_POLICY_STATUS pPolicyStatus) @@ -3518,12 +3536,11 @@ static BOOL WINAPI verify_ssl_policy(LPCSTR szPolicyOID, } else if (pChainContext->TrustStatus.dwErrorStatus & CERT_TRUST_REVOCATION_STATUS_UNKNOWN && - !(checks & SECURITY_FLAG_IGNORE_REVOCATION) && !(baseChecks & CERT_CHAIN_POLICY_IGNORE_END_REV_UNKNOWN_FLAG)) + !(checks & SECURITY_FLAG_IGNORE_REVOCATION) && !(baseChecks & CERT_CHAIN_POLICY_IGNORE_END_REV_UNKNOWN_FLAG) + && find_chain_first_element_with_error(pChainContext, CERT_TRUST_REVOCATION_STATUS_UNKNOWN, + &pPolicyStatus->lChainIndex, &pPolicyStatus->lElementIndex)) { pPolicyStatus->dwError = CRYPT_E_REVOCATION_OFFLINE; - find_element_with_error(pChainContext, - CERT_TRUST_REVOCATION_STATUS_UNKNOWN, &pPolicyStatus->lChainIndex, - &pPolicyStatus->lElementIndex); } else if (pChainContext->TrustStatus.dwErrorStatus & CERT_TRUST_HAS_NOT_SUPPORTED_CRITICAL_EXT) diff --git a/dlls/crypt32/tests/chain.c b/dlls/crypt32/tests/chain.c index 9bf5f890167..d9f674bdc0e 100644 --- a/dlls/crypt32/tests/chain.c +++ b/dlls/crypt32/tests/chain.c @@ -5391,8 +5391,8 @@ static void test_VerifyCertChainPolicy_flags(void) { { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, 0, 0, CRYPT_E_REVOCATION_OFFLINE }, /* CERT_TRUST_REVOCATION_STATUS_UNKNOWN is only cheked on the end certificate. */ - { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 1, 0, 0, ERROR_SUCCESS, TRUE }, - { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 2, 0, 0, ERROR_SUCCESS, TRUE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 1, 0, 0, ERROR_SUCCESS }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 2, 0, 0, ERROR_SUCCESS }, { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_END_REV_UNKNOWN_FLAG, 0, 0 }, { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_CTL_SIGNER_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE }, { CERT_TRUST_REVOCATION_STATUS_UNKNOWN | CERT_TRUST_IS_OFFLINE_REVOCATION, 0, CERT_CHAIN_POLICY_IGNORE_CA_REV_UNKNOWN_FLAG, 0, CRYPT_E_REVOCATION_OFFLINE }, @@ -5403,8 +5403,8 @@ static void test_VerifyCertChainPolicy_flags(void) { CERT_TRUST_IS_OFFLINE_REVOCATION, 2, 0, 0, ERROR_SUCCESS }, { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 0, 0, 0, CRYPT_E_REVOCATION_OFFLINE }, /* CERT_TRUST_REVOCATION_STATUS_UNKNOWN is only cheked on the end certificate. */ - { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 1, 0, 0, ERROR_SUCCESS, TRUE }, - { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 2, 0, 0, ERROR_SUCCESS, TRUE }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 1, 0, 0, ERROR_SUCCESS }, + { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 2, 0, 0, ERROR_SUCCESS }, { CERT_TRUST_REVOCATION_STATUS_UNKNOWN, 0, CERT_CHAIN_POLICY_IGNORE_END_REV_UNKNOWN_FLAG, 0, 0 }, { CERT_TRUST_IS_REVOKED, 0, 0, 0, CRYPT_E_REVOKED }, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7117
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/chain.c | 2 +- dlls/crypt32/tests/chain.c | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/dlls/crypt32/chain.c b/dlls/crypt32/chain.c index e8dc15ff7e9..fa74ade13d6 100644 --- a/dlls/crypt32/chain.c +++ b/dlls/crypt32/chain.c @@ -3519,7 +3519,7 @@ static BOOL WINAPI verify_ssl_policy(LPCSTR szPolicyOID, } else if (pChainContext->TrustStatus.dwErrorStatus & CERT_TRUST_IS_NOT_VALID_FOR_USAGE && - !(checks & SECURITY_FLAG_IGNORE_WRONG_USAGE)) + !(checks & SECURITY_FLAG_IGNORE_WRONG_USAGE) && !(baseChecks & CERT_CHAIN_POLICY_IGNORE_WRONG_USAGE_FLAG)) { pPolicyStatus->dwError = CERT_E_WRONG_USAGE; find_element_with_error(pChainContext, diff --git a/dlls/crypt32/tests/chain.c b/dlls/crypt32/tests/chain.c index d9f674bdc0e..46e031101ad 100644 --- a/dlls/crypt32/tests/chain.c +++ b/dlls/crypt32/tests/chain.c @@ -5385,7 +5385,6 @@ static void test_VerifyCertChainPolicy_flags(void) DWORD policy_flags; DWORD ssl_policy_flags; DWORD expected_error; - BOOL wine_todo; } tests[] = { @@ -5415,7 +5414,7 @@ static void test_VerifyCertChainPolicy_flags(void) { CERT_TRUST_IS_NOT_VALID_FOR_USAGE, 1, 0, 0, CERT_E_WRONG_USAGE }, { CERT_TRUST_IS_NOT_VALID_FOR_USAGE, 2, 0, 0, CERT_E_WRONG_USAGE }, { CERT_TRUST_IS_NOT_VALID_FOR_USAGE, 0, 0, SECURITY_FLAG_IGNORE_WRONG_USAGE, ERROR_SUCCESS }, - { CERT_TRUST_IS_NOT_VALID_FOR_USAGE, 0, CERT_CHAIN_POLICY_IGNORE_WRONG_USAGE_FLAG, 0, ERROR_SUCCESS, TRUE }, + { CERT_TRUST_IS_NOT_VALID_FOR_USAGE, 0, CERT_CHAIN_POLICY_IGNORE_WRONG_USAGE_FLAG, 0, ERROR_SUCCESS }, { CERT_TRUST_IS_SELF_SIGNED, 0, 0, 0, TRUST_E_CERT_SIGNATURE }, { CERT_TRUST_IS_SELF_SIGNED, 1, 0, 0, TRUST_E_CERT_SIGNATURE }, @@ -5486,7 +5485,7 @@ static void test_VerifyCertChainPolicy_flags(void) policy_para.pvExtraPolicyPara = &ssl_para; ret = CertVerifyCertificateChainPolicy(CERT_CHAIN_POLICY_SSL, chain, &policy_para, &status); ok(ret, "got error %#lx.\n", GetLastError()); - todo_wine_if(tests[i].wine_todo) ok(status.dwError == tests[i].expected_error, "got %#lx, expected %#lx.\n", status.dwError, tests[i].expected_error); + ok(status.dwError == tests[i].expected_error, "got %#lx, expected %#lx.\n", status.dwError, tests[i].expected_error); if (status.dwError) { ok(!status.lChainIndex, "got %ld.\n", status.lChainIndex); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7117
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=150809 Your paranoid android. === debian11b (64 bit WoW report) === mshtml: xmlhttprequest.c:460: Test failed: AllResponseHeaders(L"Date: Thu, 09 Jan 2025 02:20:28 GMT\r\nContent-Type: application/xml\r\nTransfer-Encoding: chunked\r\nConnection: keep-alive\r\nLast-Modified: Tue, 14 Jun 2022 15:45:18 GMT\r\nETag: W/\"33-5e16a4aa23f18\"\r\nCF-Cache-Status: DYNAMIC\r\nReport-To: {\"endpoints\":[{\"url\":\"https:\\/\\/a.n"...) don't have expected substr(L"Content-Length: 51") xmlhttprequest.c:460: Test failed: AllResponseHeaders(L"Date: Thu, 09 Jan 2025 02:20:29 GMT\r\nContent-Type: application/xml\r\nTransfer-Encoding: chunked\r\nConnection: keep-alive\r\nLast-Modified: Tue, 14 Jun 2022 15:45:18 GMT\r\nETag: W/\"33-5e16a4aa23f18\"\r\nCF-Cache-Status: DYNAMIC\r\nReport-To: {\"endpoints\":[{\"url\":\"https:\\/\\/a.n"...) don't have expected substr(L"Content-Length: 51") user32: input.c:4305: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 00000000006600E4, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032 win.c:4070: Test failed: Expected active window 0000000004180150, got 0000000000000000. win.c:4071: Test failed: Expected focus window 0000000004180150, got 0000000000000000. winhttp: notification.c:734: Test failed: got unexpected thread 0xb48, err 0 winhttp.c:1205: Test failed: available_size = 542
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56559 I initially made some comments on this in https://gitlab.winehq.org/wine/wine/-/merge_requests/7112. While this is probably also not for code freeze and the problems addressed by this MR are orthogonal to !7112 , that one might make problem solved here more likely to trigger, so it is probably best to have both at once. The most unobvious part is that while CertGetCertificateChain() places errors for unknown revocation status for intermediate certs in the chain structure and in the chain-global TrustStatus.dwErrorStatus, CertVerifyCertificateChainPolicy() ignores those (without any flags suggesting that passed) unless the error relates to the end (first in chain) certificate. I could suspect that something is going wrong in my faking of errors in chain structure. So I tested it directly by simulating real offline revocation (in the attached test program which uses the same encoded certs as in !7112) and this test confirms the behaviour. It also addiotionally confirms that CERT_TRUST_IS_OFFLINE_REVOCATION flag for TrustStatus.dwErrorStatus is not set without CERT_TRUST_REVOCATION_STATUS_UNKNOWN in the chain structure (which is also fixed by this MR). [revocchain.c](/uploads/122fa673861398fb7b7bcb18bc73c5a3/revocchain.c) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7117#note_91683
Looks good to apply after the code freeze. You probably want to remove some leftover // comments in the tests. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7117#note_91701
This merge request was approved by Hans Leidekker. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7117
participants (4)
-
Hans Leidekker (＠hans) -
Marvin -
Paul Gofman
-
Paul Gofman (＠gofman)