[PATCH 1/2] crypt32/test: additional test for skipping unknown CAs
This is already tested by setting a flag in the ssl policy parameters, but apparently the flag in base policy parameters also needs to be respected. Tested on Win7. Signed-off-by: Ilia Mirkin <imirkin(a)alum.mit.edu> --- dlls/crypt32/tests/chain.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dlls/crypt32/tests/chain.c b/dlls/crypt32/tests/chain.c index e2a7633526..1bf78b084d 100644 --- a/dlls/crypt32/tests/chain.c +++ b/dlls/crypt32/tests/chain.c @@ -4743,6 +4743,11 @@ static void check_ssl_policy(void) CHECK_CHAIN_POLICY_STATUS(CERT_CHAIN_POLICY_SSL, NULL, ignoredUnknownCAPolicyCheck, &oct2007, &policyPara); sslPolicyPara.fdwChecks = 0; + /* And again, but specifying the ignore in dwFlags */ + policyPara.dwFlags = CERT_CHAIN_POLICY_ALLOW_UNKNOWN_CA_FLAG; + CHECK_CHAIN_POLICY_STATUS(CERT_CHAIN_POLICY_SSL, NULL, + ignoredUnknownCAPolicyCheck, &oct2007, &policyPara); + policyPara.dwFlags = 0; /* And again, but checking the Google chain at a bad date */ sslPolicyPara.pwszServerName = google_dot_com; CHECK_CHAIN_POLICY_STATUS(CERT_CHAIN_POLICY_SSL, NULL, -- 2.24.1
It appears that the untrusted root check should be skipped if this flag is set even if the ExtraPolicyPara one is not set. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48495 Signed-off-by: Ilia Mirkin <imirkin(a)alum.mit.edu> --- dlls/crypt32/chain.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/dlls/crypt32/chain.c b/dlls/crypt32/chain.c index 396a563c04..935fd6e344 100644 --- a/dlls/crypt32/chain.c +++ b/dlls/crypt32/chain.c @@ -3455,10 +3455,13 @@ static BOOL WINAPI verify_ssl_policy(LPCSTR szPolicyOID, PCERT_CHAIN_POLICY_STATUS pPolicyStatus) { HTTPSPolicyCallbackData *sslPara = NULL; - DWORD checks = 0; + DWORD checks = 0, baseChecks = 0; if (pPolicyPara) + { + baseChecks = pPolicyPara->dwFlags; sslPara = pPolicyPara->pvExtraPolicyPara; + } if (TRACE_ON(chain)) dump_ssl_extra_chain_policy_para(sslPara); if (sslPara && sslPara->u.cbSize >= sizeof(HTTPSPolicyCallbackData)) @@ -3474,7 +3477,8 @@ static BOOL WINAPI verify_ssl_policy(LPCSTR szPolicyOID, } else if (pChainContext->TrustStatus.dwErrorStatus & CERT_TRUST_IS_UNTRUSTED_ROOT && - !(checks & SECURITY_FLAG_IGNORE_UNKNOWN_CA)) + !(checks & SECURITY_FLAG_IGNORE_UNKNOWN_CA) && + !(baseChecks & CERT_CHAIN_POLICY_ALLOW_UNKNOWN_CA_FLAG)) { pPolicyStatus->dwError = CERT_E_UNTRUSTEDROOT; find_element_with_error(pChainContext, -- 2.24.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=63639 Your paranoid android. === debian10 (32 bit report) === crypt32: chain.c:4549: Test failed: ignoredUnknownCAPolicyCheck[0](#0004): expected 800b0101, got 800b0109 === debian10 (32 bit French report) === crypt32: chain.c:4549: Test failed: ignoredUnknownCAPolicyCheck[0](#0004): expected 800b0101, got 800b0109 === debian10 (32 bit Japanese:Japan report) === crypt32: chain.c:4549: Test failed: ignoredUnknownCAPolicyCheck[0](#0004): expected 800b0101, got 800b0109 === debian10 (32 bit Chinese:China report) === crypt32: chain.c:4549: Test failed: ignoredUnknownCAPolicyCheck[0](#0004): expected 800b0101, got 800b0109 === debian10 (32 bit WoW report) === crypt32: chain.c:4549: Test failed: ignoredUnknownCAPolicyCheck[0](#0004): expected 800b0101, got 800b0109 === debian10 (64 bit WoW report) === crypt32: chain.c:4549: Test failed: ignoredUnknownCAPolicyCheck[0](#0004): expected 800b0101, got 800b0109
Hello Ilia, thanks for the patch. On 1/22/20 9:45 AM, Ilia Mirkin wrote:
This is already tested by setting a flag in the ssl policy parameters, but apparently the flag in base policy parameters also needs to be respected. Tested on Win7.
Signed-off-by: Ilia Mirkin <imirkin(a)alum.mit.edu> --- dlls/crypt32/tests/chain.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/dlls/crypt32/tests/chain.c b/dlls/crypt32/tests/chain.c index e2a7633526..1bf78b084d 100644 --- a/dlls/crypt32/tests/chain.c +++ b/dlls/crypt32/tests/chain.c @@ -4743,6 +4743,11 @@ static void check_ssl_policy(void) CHECK_CHAIN_POLICY_STATUS(CERT_CHAIN_POLICY_SSL, NULL, ignoredUnknownCAPolicyCheck, &oct2007, &policyPara); sslPolicyPara.fdwChecks = 0; + /* And again, but specifying the ignore in dwFlags */ + policyPara.dwFlags = CERT_CHAIN_POLICY_ALLOW_UNKNOWN_CA_FLAG; + CHECK_CHAIN_POLICY_STATUS(CERT_CHAIN_POLICY_SSL, NULL, + ignoredUnknownCAPolicyCheck, &oct2007, &policyPara); + policyPara.dwFlags = 0; /* And again, but checking the Google chain at a bad date */ sslPolicyPara.pwszServerName = google_dot_com; CHECK_CHAIN_POLICY_STATUS(CERT_CHAIN_POLICY_SSL, NULL,
As the testbot has complained, we don't want the tests to fail even temporarily on Wine; that's what todo_wine is for. To resolve this, because crypt32 is a terrible mess, you could either duplicate "ignoredUnknownCAPolicyCheck" and add the TODO flag, or just reorder or combine the patches.
participants (3)
-
Ilia Mirkin -
Marvin -
Zebediah Figura