[PATCH v2 0/4] MR3199: winhttp: Handle SEC_I_RENEGOTIATE after DecryptMessage.

List overview All Threads
Download

newer

older

[PATCH v4 0/1] MR2274: ntdll: Add...

[PATCH v6 0/5] MR2698: winex11:...

Rémi Bernon (＠rbernon)

29 Jun 2023 29 Jun '23

3 p.m.

This is some very old patch series, initially aimed at speeding up Gears of Wars online connection, but it wasn't clear if it was actually necessary. We now confirmed that it is also required for Gears 5 online mode, and without it, connection fails with an error message. I updated the last change to specifically handle TLS rehandshake vs empty input, to avoid breaking some tests. I think having empty input when renegotiating is a valid scenario, although I'm not sure how to write tests for that (it will require some server-side part to request a re-handshake to the client). In any case, this has nothing to do with client certificate as the returned ERROR_WINHTTP_CLIENT_AUTH_CERT_NEEDED error in the code seems to suggest. -- v2: secur32: Perform TLS re-handshake after SEC_I_RENEGOTIATE was returned. winhttp: Handle SEC_I_RENEGOTIATE after DecryptMessage. winhttp: Introduce new netconn_negotiate helper. winhttp: Move connect end checks out of the loop. https://gitlab.winehq.org/wine/wine/-/merge_requests/3199

Show replies by date

Rémi Bernon

29 Jun 29 Jun

3 p.m.

New subject: [PATCH v2 1/4] winhttp: Move connect end checks out of the loop.

From: Rémi Bernon <rbernon(a)codeweavers.com> 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. --- dlls/winhttp/net.c | 80 ++++++++++++++++++++++------------------------ 1 file changed, 39 insertions(+), 41 deletions(-) diff --git a/dlls/winhttp/net.c b/dlls/winhttp/net.c index 1be78d126d4..2e655bed5a5 100644 --- a/dlls/winhttp/net.c +++ b/dlls/winhttp/net.c @@ -384,60 +384,58 @@ 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 %#lx\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; - } + 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: %lu\n", res ); - break; - } - }else { - WARN("Could not get cert\n"); - break; - } + if(status != SEC_E_OK || res != ERROR_SUCCESS) + goto failed; - conn->ssl_read_buf = malloc(conn->ssl_sizes.cbHeader + conn->ssl_sizes.cbMaximumMessage + conn->ssl_sizes.cbTrailer); - if(!conn->ssl_read_buf) { - res = ERROR_OUTOFMEMORY; - break; - } - conn->ssl_write_buf = malloc(conn->ssl_sizes.cbHeader + conn->ssl_sizes.cbMaximumMessage + conn->ssl_sizes.cbTrailer); - if(!conn->ssl_write_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; } - 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: %#lx\n", status ); - free(conn->ssl_read_buf); - conn->ssl_read_buf = NULL; - free(conn->ssl_write_buf); - conn->ssl_write_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: %lu\n", res ); + goto failed; } + conn->ssl_read_buf = malloc(conn->ssl_sizes.cbHeader + conn->ssl_sizes.cbMaximumMessage + conn->ssl_sizes.cbTrailer); + if(!conn->ssl_read_buf) { + res = ERROR_OUTOFMEMORY; + goto failed; + } + conn->ssl_write_buf = malloc(conn->ssl_sizes.cbHeader + conn->ssl_sizes.cbMaximumMessage + conn->ssl_sizes.cbTrailer); + if(!conn->ssl_write_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: %#lx\n", status ); + free(conn->ssl_read_buf); + conn->ssl_read_buf = NULL; + free(conn->ssl_write_buf); + conn->ssl_write_buf = NULL; + DeleteSecurityContext(&ctx); + return ERROR_WINHTTP_SECURE_CHANNEL_ERROR; } static DWORD send_ssl_chunk( struct netconn *conn, const void *msg, size_t size, WSAOVERLAPPED *ovr ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3199

Rémi Bernon

3 p.m.

New subject: [PATCH v2 2/4] winhttp: Introduce new netconn_negotiate helper.

From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winhttp/net.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/dlls/winhttp/net.c b/dlls/winhttp/net.c index 2e655bed5a5..4eb7b601d0e 100644 --- a/dlls/winhttp/net.c +++ b/dlls/winhttp/net.c @@ -303,28 +303,24 @@ void netconn_release( struct netconn *conn ) 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, WCHAR *hostname, CredHandle *cred_handle, + CtxtHandle *ctx ) { 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 = malloc( read_buf_size ))) return ERROR_OUTOFMEMORY; - memset( &ctx, 0, sizeof(ctx) ); status = InitializeSecurityContextW(cred_handle, NULL, hostname, isc_req_flags, 0, 0, NULL, 0, - &ctx, &out_desc, &attrs, NULL); + ctx, &out_desc, &attrs, NULL); assert(status != SEC_E_OK); @@ -337,7 +333,7 @@ DWORD netconn_secure_connect( struct netconn *conn, WCHAR *hostname, DWORD secur size = sock_send(conn->socket, out_buf.pvBuffer, out_buf.cbBuffer, NULL); if(size != out_buf.cbBuffer) { ERR("send failed\n"); - res = ERROR_WINHTTP_SECURE_CHANNEL_ERROR; + status = ERROR_WINHTTP_SECURE_CHANNEL_ERROR; break; } @@ -381,7 +377,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, hostname, isc_req_flags, 0, 0, &in_desc, 0, NULL, &out_desc, &attrs, NULL); TRACE( "InitializeSecurityContext ret %#lx\n", status ); if(status == SEC_E_OK && in_bufs[1].BufferType == SECBUFFER_EXTRA) @@ -390,6 +386,18 @@ DWORD netconn_secure_connect( struct netconn *conn, WCHAR *hostname, DWORD secur 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 = {0}; + const CERT_CONTEXT *cert; + SECURITY_STATUS status; + DWORD res = ERROR_SUCCESS; + + status = netconn_negotiate(conn, hostname, cred_handle, &ctx); if(status != SEC_E_OK || res != ERROR_SUCCESS) goto failed; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3199

Rémi Bernon

3 p.m.

New subject: [PATCH v2 3/4] winhttp: Handle SEC_I_RENEGOTIATE after DecryptMessage.

From: Rémi Bernon <rbernon(a)codeweavers.com> 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. --- dlls/winhttp/net.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/dlls/winhttp/net.c b/dlls/winhttp/net.c index 4eb7b601d0e..c10438df624 100644 --- a/dlls/winhttp/net.c +++ b/dlls/winhttp/net.c @@ -304,7 +304,7 @@ void netconn_release( struct netconn *conn ) } static DWORD netconn_negotiate( struct netconn *conn, WCHAR *hostname, CredHandle *cred_handle, - CtxtHandle *ctx ) + CtxtHandle *prev_ctx, SecBufferDesc *prev_buf, CtxtHandle *ctx ) { 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}; @@ -319,8 +319,9 @@ static DWORD netconn_negotiate( struct netconn *conn, WCHAR *hostname, CredHandl if (!(read_buf = malloc( read_buf_size ))) return ERROR_OUTOFMEMORY; - status = InitializeSecurityContextW(cred_handle, NULL, hostname, isc_req_flags, 0, 0, NULL, 0, + status = InitializeSecurityContextW(cred_handle, prev_ctx, hostname, isc_req_flags, 0, 0, prev_buf, 0, ctx, &out_desc, &attrs, NULL); + if (!ctx) ctx = prev_ctx; assert(status != SEC_E_OK); @@ -397,7 +398,7 @@ DWORD netconn_secure_connect( struct netconn *conn, WCHAR *hostname, DWORD secur SECURITY_STATUS status; DWORD res = ERROR_SUCCESS; - status = netconn_negotiate(conn, hostname, cred_handle, &ctx); + status = netconn_negotiate(conn, hostname, cred_handle, NULL, NULL, &ctx); if(status != SEC_E_OK || res != ERROR_SUCCESS) goto failed; @@ -558,8 +559,23 @@ static DWORD read_ssl_chunk( struct netconn *conn, void *buf, SIZE_T buf_size, S break; case SEC_I_RENEGOTIATE: + { + SecBuffer out_buf = {0, SECBUFFER_TOKEN, NULL}; + SecBufferDesc out_desc = {SECBUFFER_VERSION, 1, &out_buf}; + TRACE("renegotiate\n"); - return ERROR_WINHTTP_CLIENT_AUTH_CERT_NEEDED; + + for(i = 0; i < ARRAY_SIZE(bufs); i++) { + if(bufs[i].BufferType == SECBUFFER_EXTRA) { + out_buf.cbBuffer = bufs[i].cbBuffer; + out_buf.pvBuffer = bufs[i].pvBuffer; + } + } + + res = netconn_negotiate(conn, conn->host->hostname, NULL, &conn->ssl_ctx, &out_desc, NULL); + if (res != SEC_E_OK) return res; + continue; + } case SEC_I_CONTEXT_EXPIRED: TRACE("context expired\n"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3199

Rémi Bernon

3 p.m.

New subject: [PATCH v2 4/4] secur32: Perform TLS re-handshake after SEC_I_RENEGOTIATE was returned.

From: Rémi Bernon <rbernon(a)codeweavers.com> Even if input buffer is empty, as this is often the case. --- dlls/secur32/schannel.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/dlls/secur32/schannel.c b/dlls/secur32/schannel.c index 8d446af8fe1..74c84ec33cb 100644 --- a/dlls/secur32/schannel.c +++ b/dlls/secur32/schannel.c @@ -66,6 +66,7 @@ struct schan_context enum control_token control_token; unsigned int alert_type; unsigned int alert_number; + BOOL rehandshake_requested; }; static struct schan_handle *schan_handle_table; @@ -877,7 +878,7 @@ static SECURITY_STATUS establish_context( buffer = &pInput->pBuffers[idx]; ptr = buffer->pvBuffer; - if (buffer->cbBuffer < ctx->header_size) + if (buffer->cbBuffer < ctx->header_size && !ctx->rehandshake_requested) { TRACE("Expected at least %Iu bytes, but buffer only contains %lu bytes.\n", ctx->header_size, buffer->cbBuffer); @@ -894,7 +895,7 @@ static SECURITY_STATUS establish_context( ptr += record_size; } - if (!expected_size) + if (!expected_size && !ctx->rehandshake_requested) { TRACE("Expected at least %Iu bytes, but buffer only contains %lu bytes.\n", max(ctx->header_size, record_size), buffer->cbBuffer); @@ -946,6 +947,7 @@ static SECURITY_STATUS establish_context( params.alert_type = ctx->alert_type; params.alert_number = ctx->alert_number; ctx->control_token = CONTROL_TOKEN_NONE; + ctx->rehandshake_requested = FALSE; ret = GNUTLS_CALL( handshake, &params ); if (output_buffer_idx != -1) @@ -1567,6 +1569,7 @@ static SECURITY_STATUS SEC_ENTRY schan_DecryptMessage(PCtxtHandle context_handle buffer->BufferType = SECBUFFER_STREAM_HEADER; buffer->cbBuffer = ctx->header_size; + if (status == SEC_I_RENEGOTIATE) ctx->rehandshake_requested = TRUE; return status; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3199

Marvin

3:25 p.m.

New subject: [PATCH v2 4/4] secur32: Perform TLS re-handshake after SEC_I_RENEGOTIATE was returned.

Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=134306 Your paranoid android. === debian11 (32 bit report) === winhttp: winhttp.c:5745: Test failed: got 2148074244 winhttp.c:5764: Test succeeded inside todo block: unexpected success === debian11 (32 bit zh:CN report) === winhttp: winhttp.c:5745: Test failed: got 2148074244 winhttp.c:5764: Test succeeded inside todo block: unexpected success === debian11b (64 bit WoW report) === winhttp: winhttp.c:5745: Test failed: got 2148074244 winhttp.c:5764: Test succeeded inside todo block: unexpected success

Hans Leidekker (＠hans)

3:43 p.m.

...

In any case, this has nothing to do with client certificate as the returned ERROR_WINHTTP_CLIENT_AUTH_CERT_NEEDED error in the code seems to suggest.

That may be true for Gears 5 but the winhttp tests show that renegotiate can also be initiated because the server wants to authenticate the client. ERROR_WINHTTP_CLIENT_AUTH_CERT_NEEDED should be returned to the app in that case to allow it to supply a certificate. I believe in other cases renegotiate should be handled transparently like in your patches, but it's not clear to me how to distinguish them. Note that this is also discussed in bug https://bugs.winehq.org/show_bug.cgi?id=55105 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3199#note_37343

Rémi Bernon (＠rbernon)

7:08 p.m.

On Thu Jun 29 19:08:39 2023 +0000, Hans Leidekker wrote:

...

In any case, this has nothing to do with client certificate as the

returned ERROR_WINHTTP_CLIENT_AUTH_CERT_NEEDED error in the code seems to suggest. That may be true for Gears 5 but the winhttp tests show that renegotiate can also be initiated because the server wants to authenticate the client. ERROR_WINHTTP_CLIENT_AUTH_CERT_NEEDED should be returned to the app in that case to allow it to supply a certificate. I believe in other cases renegotiate should be handled transparently like in your patches, but it's not clear to me how to distinguish them. Note that this is also discussed in bug https://bugs.winehq.org/show_bug.cgi?id=55105

Ah yeah I remember now that you mentioned that detail in the past. Then it doesn't look obvious from the secur32 API either how to decide between the TLS re-handshake and a renegotiation because of a missing certificate. Writing a similar test against the game servers, the same call sequence seems to both satisfy test.winehq.org and the game server, as in: they both succeed a second InitializeSecurityContextA call sequence (even if I remove the part that adds a client certificate from the WineHQ test). There's a visible difference in the content of the stream header / tail buffers returned by DecryptMessage, especially on Windows (not so much different on Wine, but still a bit), but I don't know if we can interpret this data somehow. There's also a difference on Windows where the second InitializeSecurityContextA sequence completes with SEC_E_OK for the WineHQ test, and with SEC_E_INCOMPLETE_MESSAGE for the game server (which I understand meaning that the HTTP request should continue). It may be meaningless though, it's just a quick and dirty test. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3199#note_37397

Rémi Bernon (＠rbernon)

29 Feb 29 Feb

8:55 a.m.

New subject: [PATCH v2 0/4] MR3199: winhttp: Handle SEC_I_RENEGOTIATE after DecryptMessage. - closed

This merge request was closed by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3199

days inactive

323

days old

[email protected]

Manage subscription

8 comments

4 participants

tags (0)

참여자 (4)

Hans Leidekker (＠hans)
Marvin
Rémi Bernon
Rémi Bernon (＠rbernon)