[PATCH 1/5] winhttp: Move connect end checks out of the loop.

[Rémi Bernon]

This is only done when InitializeSecurityContextW returns SEC_E_OK,
which will break out of the loop, and the checks forcefully break out
of the loop if any failed.

Signed-off-by: Rémi Bernon 
---

This implements TLS re-handshake properly, instead of incorrectly
returning a client certificate error. It happens with Gears Tactics
online server, although the game still succeeds eventually (as TLS
doesn't mandate for clients to reply to re-handshake requests).

These probably lack some tests, but I couldn't find a way to write
something that's working. I'm sending them anyway, at least for the
discussion, in case I missed something.

It would need an SSL server that asks for a re-handshake, but I think
Wine current implementation lacks some features to create the server
side of such test. I may be wrong though, and maybe I just failed to
figure how to write it?

dlls/winhttp/net.c | 66 ++++++++++++++++++++++------------------------
 1 file changed, 32 insertions(+), 34 deletions(-)

diff --git a/dlls/winhttp/net.c b/dlls/winhttp/net.c
index 0cc2bb2bef7..46dc6bd21f0 100644
--- a/dlls/winhttp/net.c
+++ b/dlls/winhttp/net.c
@@ -341,53 +341,51 @@ DWORD netconn_secure_connect( struct netconn *conn, WCHAR *hostname, DWORD secur
         status = InitializeSecurityContextW(cred_handle, &ctx, hostname,  isc_req_flags, 0, 0, &in_desc,
                 0, NULL, &out_desc, &attrs, NULL);
         TRACE("InitializeSecurityContext ret %08x\n", status);
+        if(status == SEC_E_OK && in_bufs[1].BufferType == SECBUFFER_EXTRA)
+            FIXME("SECBUFFER_EXTRA not supported\n");
+    }

-        if(status == SEC_E_OK) {
-            if(in_bufs[1].BufferType == SECBUFFER_EXTRA)
-                FIXME("SECBUFFER_EXTRA not supported\n");
-
-            status = QueryContextAttributesW(&ctx, SECPKG_ATTR_STREAM_SIZES, &conn->ssl_sizes);
-            if(status != SEC_E_OK) {
-                WARN("Could not get sizes\n");
-                break;
-            }
+    heap_free(read_buf);

-            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);
-                CertFreeCertificateContext(cert);
-                if(res != ERROR_SUCCESS) {
-                    WARN("cert verify failed: %u\n", res);
-                    break;
-                }
-            }else {
-                WARN("Could not get cert\n");
-                break;
-            }
+    if(status != SEC_E_OK)
+        goto failed;

-            conn->ssl_buf = heap_alloc(conn->ssl_sizes.cbHeader + conn->ssl_sizes.cbMaximumMessage + conn->ssl_sizes.cbTrailer);
-            if(!conn->ssl_buf) {
-                res = ERROR_OUTOFMEMORY;
-                break;
-            }
-        }
+    status = QueryContextAttributesW(&ctx, SECPKG_ATTR_STREAM_SIZES, &conn->ssl_sizes);
+    if(status != SEC_E_OK) {
+        WARN("Could not get sizes\n");
+        goto failed;
     }

-    heap_free(read_buf);
+    status = QueryContextAttributesW(&ctx, SECPKG_ATTR_REMOTE_CERT_CONTEXT, (void*)&cert);
+    if(status != SEC_E_OK) {
+        WARN("Could not get cert\n");
+        goto failed;
+    }

-    if(status != SEC_E_OK || res != ERROR_SUCCESS) {
-        WARN("Failed to initialize security context: %08x\n", status);
-        heap_free(conn->ssl_buf);
-        conn->ssl_buf = NULL;
-        DeleteSecurityContext(&ctx);
-        return ERROR_WINHTTP_SECURE_CHANNEL_ERROR;
+    res = netconn_verify_cert(cert, hostname, security_flags, check_revocation);
+    CertFreeCertificateContext(cert);
+    if(res != ERROR_SUCCESS) {
+        WARN("cert verify failed: %u\n", res);
+        goto failed;
     }

+    conn->ssl_buf = heap_alloc(conn->ssl_sizes.cbHeader + conn->ssl_sizes.cbMaximumMessage + conn->ssl_sizes.cbTrailer);
+    if(!conn->ssl_buf) {
+        res = ERROR_OUTOFMEMORY;
+        goto failed;
+    }

TRACE("established SSL connection\n");
     conn->secure = TRUE;
     conn->ssl_ctx = ctx;
     return ERROR_SUCCESS;
+
+failed:
+    WARN("Failed to initialize security context: %08x\n", status);
+    heap_free(conn->ssl_buf);
+    conn->ssl_buf = NULL;
+    DeleteSecurityContext(&ctx);
+    return ERROR_WINHTTP_SECURE_CHANNEL_ERROR;
 }

static DWORD send_ssl_chunk( struct netconn *conn, const void *msg, size_t size )

[PATCH 2/5] winhttp: Introduce new netconn_negotiate helper.

[Rémi Bernon]

Signed-off-by: Rémi Bernon 
---
 dlls/winhttp/net.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/dlls/winhttp/net.c b/dlls/winhttp/net.c
index 46dc6bd21f0..2ba9b2dc65a 100644
--- a/dlls/winhttp/net.c
+++ b/dlls/winhttp/net.c
@@ -259,27 +259,22 @@ void netconn_close( struct netconn *conn )
     heap_free(conn);
 }

-DWORD netconn_secure_connect( struct netconn *conn, WCHAR *hostname, DWORD security_flags, CredHandle *cred_handle,
-                              BOOL check_revocation )
+static DWORD netconn_negotiate(struct netconn *conn, CredHandle *cred_handle, CtxtHandle *ctx_handle, WCHAR *hostname,
+                               DWORD isc_req_flags, SecBufferDesc *init_desc, CtxtHandle *new_ctx_handle)
 {
     SecBuffer out_buf = {0, SECBUFFER_TOKEN, NULL}, in_bufs[2] = {{0, SECBUFFER_TOKEN}, {0, SECBUFFER_EMPTY}};
     SecBufferDesc out_desc = {SECBUFFER_VERSION, 1, &out_buf}, in_desc = {SECBUFFER_VERSION, 2, in_bufs};
     BYTE *read_buf;
     SIZE_T read_buf_size = 2048;
     ULONG attrs = 0;
-    CtxtHandle ctx;
     SSIZE_T size;
-    const CERT_CONTEXT *cert;
     SECURITY_STATUS status;
-    DWORD res = ERROR_SUCCESS;
-
-    const DWORD isc_req_flags = ISC_REQ_ALLOCATE_MEMORY|ISC_REQ_USE_SESSION_KEY|ISC_REQ_CONFIDENTIALITY
-        |ISC_REQ_SEQUENCE_DETECT|ISC_REQ_REPLAY_DETECT|ISC_REQ_MANUAL_CRED_VALIDATION;

if (!(read_buf = heap_alloc( read_buf_size ))) return ERROR_OUTOFMEMORY;

-    status = InitializeSecurityContextW(cred_handle, NULL, hostname, isc_req_flags, 0, 0, NULL, 0,
-            &ctx, &out_desc, &attrs, NULL);
+    status = InitializeSecurityContextW(cred_handle, ctx_handle, hostname, isc_req_flags, 0, 0, init_desc,
+            0, new_ctx_handle, &out_desc, &attrs, NULL);
+    if (!ctx_handle) ctx_handle = new_ctx_handle;

assert(status != SEC_E_OK);

@@ -292,7 +287,7 @@ DWORD netconn_secure_connect( struct netconn *conn, WCHAR *hostname, DWORD secur
             size = sock_send(conn->socket, out_buf.pvBuffer, out_buf.cbBuffer, 0);
             if(size != out_buf.cbBuffer) {
                 ERR("send failed\n");
-                res = ERROR_WINHTTP_SECURE_CHANNEL_ERROR;
+                status = ERROR_WINHTTP_SECURE_CHANNEL_ERROR;
                 break;
             }

@@ -338,7 +333,7 @@ DWORD netconn_secure_connect( struct netconn *conn, WCHAR *hostname, DWORD secur

in_bufs[0].cbBuffer += size;
         in_bufs[0].pvBuffer = read_buf;
-        status = InitializeSecurityContextW(cred_handle, &ctx, hostname,  isc_req_flags, 0, 0, &in_desc,
+        status = InitializeSecurityContextW(cred_handle, ctx_handle, hostname, isc_req_flags, 0, 0, &in_desc,
                 0, NULL, &out_desc, &attrs, NULL);
         TRACE("InitializeSecurityContext ret %08x\n", status);
         if(status == SEC_E_OK && in_bufs[1].BufferType == SECBUFFER_EXTRA)
@@ -347,6 +342,21 @@ DWORD netconn_secure_connect( struct netconn *conn, WCHAR *hostname, DWORD secur

heap_free(read_buf);

+    return status;
+}
+
+DWORD netconn_secure_connect( struct netconn *conn, WCHAR *hostname, DWORD security_flags, CredHandle *cred_handle,
+                              BOOL check_revocation )
+{
+    CtxtHandle ctx;
+    const CERT_CONTEXT *cert;
+    SECURITY_STATUS status;
+    DWORD res = ERROR_SUCCESS;
+
+    const DWORD isc_req_flags = ISC_REQ_ALLOCATE_MEMORY|ISC_REQ_USE_SESSION_KEY|ISC_REQ_CONFIDENTIALITY
+        |ISC_REQ_SEQUENCE_DETECT|ISC_REQ_REPLAY_DETECT|ISC_REQ_MANUAL_CRED_VALIDATION;
+
+    status = netconn_negotiate(conn, cred_handle, NULL, hostname, isc_req_flags, NULL, &ctx);
     if(status != SEC_E_OK)
         goto failed;

[PATCH 3/5] winhttp: Handle SEC_I_RENEGOTIATE after DecryptMessage.

[Rémi Bernon]

By performing renegotiation as we should, instead of incorrectly
returning ERROR_WINHTTP_CLIENT_AUTH_CERT_NEEDED.

MSDN says we should pass returned SECBUFFER_EXTRA as SECBUFFER_TOKEN,
so we also do that, although it's usually empty.

Signed-off-by: Rémi Bernon 
---
 dlls/winhttp/net.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/dlls/winhttp/net.c b/dlls/winhttp/net.c
index 2ba9b2dc65a..ba262a83391 100644
--- a/dlls/winhttp/net.c
+++ b/dlls/winhttp/net.c
@@ -454,8 +454,10 @@ DWORD netconn_send( struct netconn *conn, const void *msg, size_t len, int *sent

static DWORD read_ssl_chunk( struct netconn *conn, void *buf, SIZE_T buf_size, SIZE_T *ret_size, BOOL *eof )
 {
+    const DWORD isc_req_flags = ISC_REQ_ALLOCATE_MEMORY|ISC_REQ_USE_SESSION_KEY|ISC_REQ_CONFIDENTIALITY
+        |ISC_REQ_SEQUENCE_DETECT|ISC_REQ_REPLAY_DETECT|ISC_REQ_USE_SUPPLIED_CREDS;
     const SIZE_T ssl_buf_size = conn->ssl_sizes.cbHeader+conn->ssl_sizes.cbMaximumMessage+conn->ssl_sizes.cbTrailer;
-    SecBuffer bufs[4];
+    SecBuffer bufs[4], tmp;
     SecBufferDesc buf_desc = {SECBUFFER_VERSION, ARRAY_SIZE(bufs), bufs};
     SSIZE_T size, buf_len;
     unsigned int i;
@@ -496,7 +498,16 @@ static DWORD read_ssl_chunk( struct netconn *conn, void *buf, SIZE_T buf_size, S

case SEC_I_RENEGOTIATE:
             TRACE("renegotiate\n");
-            return ERROR_WINHTTP_CLIENT_AUTH_CERT_NEEDED;
+            memset(&tmp, 0, sizeof(tmp));
+            for(i = 0; i < ARRAY_SIZE(bufs); i++) {
+                if(bufs[i].BufferType == SECBUFFER_EXTRA) tmp = bufs[i];
+            }
+            memset(bufs, 0, sizeof(bufs));
+            bufs[0] = tmp;
+            bufs[0].BufferType = SECBUFFER_TOKEN;
+            res = netconn_negotiate(conn, NULL, &conn->ssl_ctx, conn->host->hostname, isc_req_flags, &buf_desc, NULL);
+            if (res != SEC_E_OK) return res;
+            break;

case SEC_I_CONTEXT_EXPIRED:
             TRACE("context expired\n");

[PATCH 4/5] secur32: Perform TLS handshake even if input is empty.

[Rémi Bernon]

Instead of immediately returning even if we don't know how much.

This may be the case if we received SEC_I_RENEGOTIATE status, and in any
case the handshake will tell us if more data is needed.

Signed-off-by: Rémi Bernon 
---
 dlls/secur32/schannel.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/dlls/secur32/schannel.c b/dlls/secur32/schannel.c
index 2d135a85227..07d3ea5216a 100644
--- a/dlls/secur32/schannel.c
+++ b/dlls/secur32/schannel.c
@@ -881,7 +881,7 @@ static SECURITY_STATUS SEC_ENTRY schan_InitializeSecurityContextW(
             ptr += record_size;
         }

-        if (!expected_size)
+        if (!expected_size && record_size)
         {
             TRACE("Expected at least %lu bytes, but buffer only contains %u bytes.\n",
                     max(6, record_size), buffer->cbBuffer);
@@ -912,6 +912,8 @@ static SECURITY_STATUS SEC_ENTRY schan_InitializeSecurityContextW(
     {
         SecBuffer *buffer = &out_buffers->desc->pBuffers[0];
         buffer->cbBuffer = 0;
+        /* Nothing to read or to send, but we got SEC_I_CONTINUE_NEEDED, it means missing input */
+        if (!expected_size && ret == SEC_I_CONTINUE_NEEDED) ret = SEC_E_INCOMPLETE_MESSAGE;
     }

if(ctx->transport.in.offset && ctx->transport.in.offset != pInput->pBuffers[0].cbBuffer) {

[PATCH 4/5] secur32: Perform TLS handshake even if input is empty.

[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=85600

Your paranoid android.

=== debiant2 (32 bit report) ===

secur32:
schannel.c:818: Test failed: Output buffer size changed.
schannel.c:833: Test failed: Output buffer size changed.
schannel.c:842: Test failed: Output buffer size changed.
schannel: Timeout

=== debiant2 (32 bit Chinese:China report) ===

secur32:
schannel.c:818: Test failed: Output buffer size changed.
schannel.c:833: Test failed: Output buffer size changed.
schannel.c:842: Test failed: Output buffer size changed.
schannel: Timeout

=== debiant2 (32 bit WoW report) ===

secur32:
schannel.c:818: Test failed: Output buffer size changed.
schannel.c:833: Test failed: Output buffer size changed.
schannel.c:842: Test failed: Output buffer size changed.
schannel: Timeout

=== debiant2 (64 bit WoW report) ===

secur32:
schannel.c:818: Test failed: Output buffer size changed.
schannel.c:833: Test failed: Output buffer size changed.
schannel.c:842: Test failed: Output buffer size changed.
schannel: Timeout

[PATCH 4/5] secur32: Perform TLS handshake even if input is empty.

[Rémi Bernon]

On 2/15/21 1:22 PM, Marvin wrote:
> 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=85600
>
> Your paranoid android.
>
> === debiant2 (32 bit report) ===
>
> secur32:
> schannel.c:818: Test failed: Output buffer size changed.
> schannel.c:833: Test failed: Output buffer size changed.
> schannel.c:842: Test failed: Output buffer size changed.
> schannel: Timeout
>
> === debiant2 (32 bit Chinese:China report) ===
>
> secur32:
> schannel.c:818: Test failed: Output buffer size changed.
> schannel.c:833: Test failed: Output buffer size changed.
> schannel.c:842: Test failed: Output buffer size changed.
> schannel: Timeout
>
> === debiant2 (32 bit WoW report) ===
>
> secur32:
> schannel.c:818: Test failed: Output buffer size changed.
> schannel.c:833: Test failed: Output buffer size changed.
> schannel.c:842: Test failed: Output buffer size changed.
> schannel: Timeout
>
> === debiant2 (64 bit WoW report) ===
>
> secur32:
> schannel.c:818: Test failed: Output buffer size changed.
> schannel.c:833: Test failed: Output buffer size changed.
> schannel.c:842: Test failed: Output buffer size changed.
> schannel: Timeout

I guess that speaks for itself, but I'm still interested to discuss how 
to properly test the re-handshake, if there are ways.

[PATCH 4/5] secur32: Perform TLS handshake even if input is empty.

[Hans Leidekker]

On Mon, 2021-02-15 at 13:26 +0100, Rémi Bernon wrote:
> On 2/15/21 1:22 PM, Marvin wrote:
> > === debiant2 (64 bit WoW report) ===
> >
> > secur32:
> > schannel.c:818: Test failed: Output buffer size changed.
> > schannel.c:833: Test failed: Output buffer size changed.
> > schannel.c:842: Test failed: Output buffer size changed.
> > schannel: Timeout
>
> I guess that speaks for itself, but I'm still interested to discuss how 
> to properly test the re-handshake, if there are ways.

We could set up a test on test.winehq.org. Apache docs suggest that
changing SSL parameters trigger a re-handshake. Maybe something like
this will work:

<Directory clientcert>
SSLVerifyClient require
</Directory>

[PATCH 4/5] secur32: Perform TLS handshake even if input is empty.

[Rémi Bernon]

On 2/15/21 3:32 PM, Hans Leidekker wrote:
> On Mon, 2021-02-15 at 13:26 +0100, Rémi Bernon wrote:
> > On 2/15/21 1:22 PM, Marvin wrote:
> > > === debiant2 (64 bit WoW report) ===
> > >
> > > secur32:
> > > schannel.c:818: Test failed: Output buffer size changed.
> > > schannel.c:833: Test failed: Output buffer size changed.
> > > schannel.c:842: Test failed: Output buffer size changed.
> > > schannel: Timeout
> >
> > I guess that speaks for itself, but I'm still interested to discuss how
> > to properly test the re-handshake, if there are ways.
>
> We could set up a test on test.winehq.org. Apache docs suggest that
> changing SSL parameters trigger a re-handshake. Maybe something like
> this will work:
>
> <Directory clientcert>
> SSLVerifyClient require
> </Directory>

I don't really know much about Apache configuration, but isn't this 
going to require a client certificate, which we don't have? Is it going 
to fail but after the re-handshake (which would allow us to test it)?

[PATCH 4/5] secur32: Perform TLS handshake even if input is empty.

[Hans Leidekker]

On Wed, 2021-02-17 at 12:23 +0100, Rémi Bernon wrote:
> On 2/15/21 3:32 PM, Hans Leidekker wrote:
> > On Mon, 2021-02-15 at 13:26 +0100, Rémi Bernon wrote:
> > > On 2/15/21 1:22 PM, Marvin wrote:
> > > > === debiant2 (64 bit WoW report) ===
> > > >
> > > > secur32:
> > > > schannel.c:818: Test failed: Output buffer size changed.
> > > > schannel.c:833: Test failed: Output buffer size changed.
> > > > schannel.c:842: Test failed: Output buffer size changed.
> > > > schannel: Timeout
> > >
> > > I guess that speaks for itself, but I'm still interested to discuss how
> > > to properly test the re-handshake, if there are ways.
> >
> > We could set up a test on test.winehq.org. Apache docs suggest that
> > changing SSL parameters trigger a re-handshake. Maybe something like
> > this will work:
> >
> > <Directory clientcert>
> > SSLVerifyClient require
> > </Directory>
>
> I don't really know much about Apache configuration, but isn't this 
> going to require a client certificate, which we don't have? Is it going 
> to fail but after the re-handshake (which would allow us to test it)?

It's supposed to fail with ERROR_WINHTTP_CLIENT_AUTH_CERT_NEEDED, after
which the client should set the cert context with 
WinHttpSetOption(WINHTTP_OPTION_CLIENT_CERT_CONTEXT) and try again. I think
that's an interesting test case too.

We could send a self-signed certificate, which of course makes the re-handshake
fail. If we really need the re-handshake to succeed 'optional_no_ca' instead of
'require' may help. I'll see if I can make this work.

[PATCH 5/5] secur32: Return STREAM_HEADER/TRAILER buffers on SEC_I_RENEGOTIATE.

[Rémi Bernon]

And don't print an error anymore.

Signed-off-by: Rémi Bernon 
---
 dlls/secur32/schannel.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/dlls/secur32/schannel.c b/dlls/secur32/schannel.c
index 07d3ea5216a..272cb6dabd6 100644
--- a/dlls/secur32/schannel.c
+++ b/dlls/secur32/schannel.c
@@ -1338,6 +1338,7 @@ static void schan_decrypt_fill_buffer(PSecBufferDesc message, ULONG buffer_type,
 static SECURITY_STATUS SEC_ENTRY schan_DecryptMessage(PCtxtHandle context_handle,
         PSecBufferDesc message, ULONG message_seq_no, PULONG quality)
 {
+    SECURITY_STATUS status = SEC_E_OK;
     struct schan_context *ctx;
     SecBuffer *buffer;
     SIZE_T data_size;
@@ -1387,10 +1388,16 @@ static SECURITY_STATUS SEC_ENTRY schan_DecryptMessage(PCtxtHandle context_handle
     while (received < data_size)
     {
         SIZE_T length = data_size - received;
-        SECURITY_STATUS status = schan_imp_recv(ctx->session, data + received, &length);
+        status = schan_imp_recv(ctx->session, data + received, &length);
+
+        if (status == SEC_I_RENEGOTIATE)
+            break;

if (status == SEC_I_CONTINUE_NEEDED)
+        {
+            status = SEC_E_OK;
             break;
+        }

if (status != SEC_E_OK)
         {
@@ -1423,7 +1430,7 @@ static SECURITY_STATUS SEC_ENTRY schan_DecryptMessage(PCtxtHandle context_handle
     buffer->BufferType = SECBUFFER_STREAM_HEADER;
     buffer->cbBuffer = 5;

-    return SEC_E_OK;
+    return status;
 }

static SECURITY_STATUS SEC_ENTRY schan_DeleteSecurityContext(PCtxtHandle context_handle)

[PATCH 5/5] secur32: Return STREAM_HEADER/TRAILER buffers on SEC_I_RENEGOTIATE.

[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=85601

Your paranoid android.

=== debiant2 (32 bit report) ===

secur32:
schannel.c:818: Test failed: Output buffer size changed.
schannel.c:833: Test failed: Output buffer size changed.
schannel.c:842: Test failed: Output buffer size changed.
schannel: Timeout

=== debiant2 (32 bit Chinese:China report) ===

secur32:
schannel.c:818: Test failed: Output buffer size changed.
schannel.c:833: Test failed: Output buffer size changed.
schannel.c:842: Test failed: Output buffer size changed.
schannel: Timeout

=== debiant2 (32 bit WoW report) ===

secur32:
schannel.c:818: Test failed: Output buffer size changed.
schannel.c:833: Test failed: Output buffer size changed.
schannel.c:842: Test failed: Output buffer size changed.
schannel: Timeout

=== debiant2 (64 bit WoW report) ===

secur32:
schannel.c:818: Test failed: Output buffer size changed.
schannel.c:833: Test failed: Output buffer size changed.
schannel.c:842: Test failed: Output buffer size changed.
schannel: Timeout