[PATCH 1/5] winhttp: Move connect end checks out of the loop.
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 <rbernon(a)codeweavers.com> --- 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 ) -- 2.30.0
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- 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; -- 2.30.0
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 <rbernon(a)codeweavers.com> --- 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"); -- 2.30.0
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 <rbernon(a)codeweavers.com> --- 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) { -- 2.30.0
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
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. -- Rémi Bernon <rbernon(a)codeweavers.com>
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>
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)? -- Rémi Bernon <rbernon(a)codeweavers.com>
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.
And don't print an error anymore. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- 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) -- 2.30.0
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
participants (3)
-
Hans Leidekker -
Marvin -
Rémi Bernon