[PATCH 1/2] crypt32/test: additional test for skipping unknown CAs

[Ilia Mirkin]

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 
---
 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,

[PATCH 2/2] crypt32: also check CERT_CHAIN_POLICY_ALLOW_UNKNOWN_CA_FLAG

[Ilia Mirkin]

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 
---
 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,

[Marvin]

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

[Zebediah Figura]

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
>
> 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.