[PATCH v2 0/2] MR11546: winhttp: Handle incorrect handle state when querying WINHTTP_OPTION_SERVER_CERT_CHAIN_CONTEXT.

[Paul Gofman (＠gofman)]

--
  v2: winhttp: Use chain established in netconn_verify_cert() for WINHTTP_OPTION_SERVER_CERT_CHAIN_CONTEXT.
      winhttp: Handle incorrect handle state when querying WINHTTP_OPTION_SERVER_CERT_CHAIN_CONTEXT.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/11546

[PATCH v2 1/2] winhttp: Handle incorrect handle state when querying WINHTTP_OPTION_SERVER_CERT_CHAIN_CONTEXT.

[Paul Gofman]

From: Paul Gofman <[e-mail]>

---
 dlls/winhttp/session.c       |  6 ++++++
 dlls/winhttp/tests/winhttp.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/dlls/winhttp/session.c b/dlls/winhttp/session.c
index 85e3c3f18d1..d06e208ac3d 100644
--- a/dlls/winhttp/session.c
+++ b/dlls/winhttp/session.c
@@ -923,6 +923,12 @@ static BOOL request_query_option( struct object_header *hdr, DWORD option, void
         chainPara.RequestedUsage.Usage.rgpszUsageIdentifier = server_auth;
 
         if (!validate_buffer( buffer, buflen, sizeof(cert_chain) )) return FALSE;
+        if (!request || !request->server_cert)
+        {
+            SetLastError( ERROR_WINHTTP_INCORRECT_HANDLE_STATE );
+            *(CERT_CHAIN_CONTEXT **)buffer = NULL;
+            return FALSE;
+        }
         if (!CertGetCertificateChain(NULL, request->server_cert, NULL, NULL, &chainPara, 0, NULL, &cert_chain)) return FALSE;
 
         *(CERT_CHAIN_CONTEXT **)buffer = (CERT_CHAIN_CONTEXT *)cert_chain;
diff --git a/dlls/winhttp/tests/winhttp.c b/dlls/winhttp/tests/winhttp.c
index f639877d2ab..4caf2abee38 100644
--- a/dlls/winhttp/tests/winhttp.c
+++ b/dlls/winhttp/tests/winhttp.c
@@ -1099,6 +1099,9 @@ static void test_secure_connection(void)
     CERT_CONTEXT *cert;
     WINHTTP_CERTIFICATE_INFO info;
     WINHTTP_SECURITY_INFO secinfo;
+    PCCERT_CHAIN_CONTEXT chain;
+    CERT_CHAIN_POLICY_PARA chain_policy = { .cbSize = sizeof(chain_policy) };
+    CERT_CHAIN_POLICY_STATUS policy_status = { .cbSize = sizeof(policy_status) };
     char buffer[32];
 
     ses = WinHttpOpen(L"winetest", 0, NULL, NULL, 0);
@@ -1152,9 +1155,25 @@ static void test_secure_connection(void)
 
     WinHttpCloseHandle(req);
 
+    size = sizeof(chain);
+    chain = (void *)0xdeadbeef;
+    SetLastError(0xdeadbeef);
+    ret = WinHttpQueryOption(ses, WINHTTP_OPTION_SERVER_CERT_CHAIN_CONTEXT, &chain, &size);
+    ok(!ret, "unexpected success.\n");
+    todo_wine ok(GetLastError() == ERROR_WINHTTP_INCORRECT_HANDLE_TYPE, "got error %lu.\n", GetLastError());
+    ok(chain == (void *)0xdeadbeef, "got %p.\n", chain);
+
     req = WinHttpOpenRequest(con, NULL, NULL, NULL, NULL, NULL, WINHTTP_FLAG_SECURE);
     ok(req != NULL, "failed to open a request %lu\n", GetLastError());
 
+    size = sizeof(chain);
+    chain = (void *)0xdeadbeef;
+    SetLastError(0xdeadbeef);
+    ret = WinHttpQueryOption(req, WINHTTP_OPTION_SERVER_CERT_CHAIN_CONTEXT, &chain, &size);
+    ok(!ret, "unexpected success.\n");
+    ok(GetLastError() == ERROR_WINHTTP_INCORRECT_HANDLE_STATE, "got error %lu.\n", GetLastError());
+    ok(!chain, "got %p.\n", chain);
+
     flags = 0xdeadbeef;
     size = sizeof(flags);
     ret = WinHttpQueryOption(req, WINHTTP_OPTION_SECURITY_FLAGS, &flags, &size);
@@ -1198,6 +1217,18 @@ static void test_secure_connection(void)
     }
     ok(ret, "failed to send request %lu\n", GetLastError());
 
+    size = sizeof(chain);
+    chain = (void *)0xdeadbeef;
+    SetLastError(0xdeadbeef);
+    ret = WinHttpQueryOption(req, WINHTTP_OPTION_SERVER_CERT_CHAIN_CONTEXT, &chain, &size);
+    ok(ret, "got error %lu.\n", GetLastError());
+    ok(chain && chain != (void *)0xdeadbeef, "got %p.\n", chain);
+    ret = CertVerifyCertificateChainPolicy(CERT_CHAIN_POLICY_SSL, chain, &chain_policy, &policy_status);
+    ok(ret, "got error %lu.\n", GetLastError());
+    ok(!chain->TrustStatus.dwErrorStatus, "got %#lx.\n", chain->TrustStatus.dwErrorStatus);
+    ok(!policy_status.dwError, "got %#lx.\n", policy_status.dwError);
+    CertFreeCertificateChain(chain);
+
     size = sizeof(cert);
     ret = WinHttpQueryOption(req, WINHTTP_OPTION_SERVER_CERT_CONTEXT, &cert, &size );
     ok(ret, "failed to retrieve certificate context %lu\n", GetLastError());
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/11546

[PATCH v2 2/2] winhttp: Use chain established in netconn_verify_cert() for WINHTTP_OPTION_SERVER_CERT_CHAIN_CONTEXT.

[Paul Gofman]

From: Paul Gofman <[e-mail]>

---
 dlls/winhttp/net.c             |  9 ++++++---
 dlls/winhttp/session.c         | 22 ++++++----------------
 dlls/winhttp/winhttp_private.h |  1 +
 3 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/dlls/winhttp/net.c b/dlls/winhttp/net.c
index b0f405a9f52..05922acca00 100644
--- a/dlls/winhttp/net.c
+++ b/dlls/winhttp/net.c
@@ -81,7 +81,8 @@ static int sock_recv(int fd, void *msg, size_t len, int flags)
     return ret;
 }
 
-static DWORD netconn_verify_cert( PCCERT_CONTEXT cert, WCHAR *server, DWORD security_flags, BOOL check_revocation )
+static DWORD netconn_verify_cert( PCCERT_CONTEXT cert, WCHAR *server, DWORD security_flags, BOOL check_revocation,
+                                  PCCERT_CHAIN_CONTEXT *ret_chain )
 {
     HCERTSTORE store = cert->hCertStore;
     BOOL ret;
@@ -92,6 +93,7 @@ static DWORD netconn_verify_cert( PCCERT_CONTEXT cert, WCHAR *server, DWORD secu
     DWORD err = ERROR_SUCCESS;
 
     TRACE("verifying %s\n", debugstr_w( server ));
+    *ret_chain = NULL;
     chainPara.RequestedUsage.Usage.cUsageIdentifier = 1;
     chainPara.RequestedUsage.Usage.rgpszUsageIdentifier = server_auth;
     ret = CertGetCertificateChain( NULL, cert, NULL, store, &chainPara,
@@ -169,7 +171,7 @@ static DWORD netconn_verify_cert( PCCERT_CONTEXT cert, WCHAR *server, DWORD secu
                     err = ERROR_WINHTTP_SECURE_INVALID_CERT;
             }
         }
-        CertFreeCertificateChain( chain );
+        *ret_chain = chain;
     }
     else
         err = ERROR_WINHTTP_SECURE_CHANNEL_ERROR;
@@ -296,6 +298,7 @@ void netconn_release( struct netconn *conn )
         free(conn->ssl_read_buf);
         free(conn->ssl_write_buf);
         free(conn->extra_buf);
+        CertFreeCertificateChain(conn->chain);
         DeleteSecurityContext(&conn->ssl_ctx);
     }
     if (conn->socket != -1)
@@ -400,7 +403,7 @@ DWORD netconn_secure_connect( struct netconn *conn, WCHAR *hostname, DWORD secur
 
             status = QueryContextAttributesW(&ctx, SECPKG_ATTR_REMOTE_CERT_CONTEXT, (void*)&cert);
             if(status == SEC_E_OK) {
-                res = netconn_verify_cert(cert, hostname, security_flags, check_revocation);
+                res = netconn_verify_cert(cert, hostname, security_flags, check_revocation, &conn->chain);
                 CertFreeCertificateContext(cert);
                 if(res != ERROR_SUCCESS) {
                     WARN( "cert verify failed: %lu\n", res );
diff --git a/dlls/winhttp/session.c b/dlls/winhttp/session.c
index d06e208ac3d..f83c9a30c8c 100644
--- a/dlls/winhttp/session.c
+++ b/dlls/winhttp/session.c
@@ -912,28 +912,18 @@ static BOOL request_query_option( struct object_header *hdr, DWORD option, void
     }
     case WINHTTP_OPTION_SERVER_CERT_CHAIN_CONTEXT:
     {
-        const CERT_CHAIN_CONTEXT *cert_chain;
+        const CERT_CHAIN_CONTEXT *chain;
 
-        char oid_server_auth[] = szOID_PKIX_KP_SERVER_AUTH;
-        char *server_auth[] = { oid_server_auth };
-
-        CERT_CHAIN_PARA chainPara = { sizeof(chainPara), { 0 } };
-
-        chainPara.RequestedUsage.Usage.cUsageIdentifier = 1;
-        chainPara.RequestedUsage.Usage.rgpszUsageIdentifier = server_auth;
-
-        if (!validate_buffer( buffer, buflen, sizeof(cert_chain) )) return FALSE;
-        if (!request || !request->server_cert)
+        if (!validate_buffer( buffer, buflen, sizeof(chain) )) return FALSE;
+        if (!request || !request->netconn || !request->netconn->chain)
         {
             SetLastError( ERROR_WINHTTP_INCORRECT_HANDLE_STATE );
             *(CERT_CHAIN_CONTEXT **)buffer = NULL;
             return FALSE;
         }
-        if (!CertGetCertificateChain(NULL, request->server_cert, NULL, NULL, &chainPara, 0, NULL, &cert_chain)) return FALSE;
-
-        *(CERT_CHAIN_CONTEXT **)buffer = (CERT_CHAIN_CONTEXT *)cert_chain;
-        *buflen = sizeof(cert_chain);
-
+        if (!(chain = CertDuplicateCertificateChain( request->netconn->chain ))) return FALSE;
+        *(const CERT_CHAIN_CONTEXT **)buffer = chain;
+        *buflen = sizeof(chain);
         return TRUE;
     }
     case WINHTTP_OPTION_SECURITY_CERTIFICATE_STRUCT:
diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h
index 99f57749856..46baccb1b93 100644
--- a/dlls/winhttp/winhttp_private.h
+++ b/dlls/winhttp/winhttp_private.h
@@ -110,6 +110,7 @@ struct netconn
     struct hostdata *host;
     ULONGLONG keep_until;
     CtxtHandle ssl_ctx;
+    PCCERT_CHAIN_CONTEXT chain;
     SecPkgContext_StreamSizes ssl_sizes;
     char *ssl_read_buf, *ssl_write_buf;
     char *extra_buf;
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/11546

[Paul Gofman (＠gofman)]

v2:
 - add a patch which stores originally established chain and uses that for CERT_CHAIN_REVOCATION_CHECK_CHAIN_EXCLUDE_ROOT.

The main problem with the present implementation is that the intermediate certificates maybe be received from the server and the current implementation which only uses default root store when constructing chain for WINHTTP_OPTION_SERVER_CERT_CHAIN_CONTEXT may be missing those. That actually doesn't break simple cases (the issue is not reproducible with test using test.winehq.org because crypt32 will try to find issuer from URL_OID_CERTIFICATE_ISSUER and thus reconstruct the chain. But that OID is not guaranteed to exist, the cert chain from server should not be ignored. The actual issue I am reproducing with current impl is actually trickier than missing OIDs (while that is also possible). Offending server https://www2.signon.goliath.prod.deadorbit.net has intermediate "GoDaddy TLS Root CA - R1" certificate. The version of this certificate obtained from http://certificates.godaddy.com/repository/gd_tls_root-r1.crt shows up as self signed cert (issuer is the same as cert) so reconstru
 cted chain ends up without actual root and fails to verify. While the one which server sends is better and features "Go Daddy Root Certificate Authority - G2".

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/11546#note_147723

[Paul Gofman (＠gofman)]

Alternatively, passing server cert's hCertStore to CertGetCertificateChain in current implementation would also solve that but it looks cleaner and easier to me to just store the reconstructed chain, also avoiding potentially lengthy extra chain construction (with revocation checks).

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/11546#note_147724

[Hans Leidekker (＠hans)]

Hans Leidekker (@hans) commented about dlls/winhttp/session.c:
> chainPara.RequestedUsage.Usage.rgpszUsageIdentifier = server_auth;
>
> if (!validate_buffer( buffer, buflen, sizeof(cert_chain) )) return FALSE;
> +        if (!request || !request->server_cert)

The NULL request check is not needed.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/11546#note_147803

[Hans Leidekker (＠hans)]

Hans Leidekker (@hans) commented about dlls/winhttp/winhttp_private.h:
> struct hostdata *host;
>      ULONGLONG keep_until;
>      CtxtHandle ssl_ctx;
> +    PCCERT_CHAIN_CONTEXT chain;

`const CERT_CHAIN_CONTEXT *chain;`

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/11546#note_147804