[PATCH 0/1] MR8116: kerberos: Add support for SECBUFFER_STREAM to SpUnsealMessage().
This is an alternative approach to https://gitlab.winehq.org/wine/wine/-/merge_requests/8114. Once this is accepted SECBUFFER_STREAM could be used for for Kerberos/DecryptMessage() in libs/ldap. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/kerberos/krb5_ap.c | 28 ++++++++++++++++++++++++---- dlls/kerberos/unixlib.c | 29 +++++++++++++++++++---------- dlls/kerberos/unixlib.h | 2 ++ 3 files changed, 45 insertions(+), 14 deletions(-) diff --git a/dlls/kerberos/krb5_ap.c b/dlls/kerberos/krb5_ap.c index 5a5878faf02..46c12586406 100644 --- a/dlls/kerberos/krb5_ap.c +++ b/dlls/kerberos/krb5_ap.c @@ -867,16 +867,36 @@ static NTSTATUS NTAPI kerberos_SpUnsealMessage( LSA_SEC_HANDLE context, SecBuffe { struct context_handle *context_handle = (void *)context; struct unseal_message_params params; - int data_idx, token_idx; + int stream_idx, data_idx, token_idx; + if ((stream_idx = get_buffer_index( message, SECBUFFER_STREAM )) == -1) + if ((token_idx = get_buffer_index( message, SECBUFFER_TOKEN )) == -1) return SEC_E_INVALID_TOKEN; if ((data_idx = get_buffer_index( message, SECBUFFER_DATA )) == -1) return SEC_E_INVALID_TOKEN; - if ((token_idx = get_buffer_index( message, SECBUFFER_TOKEN )) == -1) return SEC_E_INVALID_TOKEN; params.context = context_handle->handle; + + if (stream_idx == -1) + { + params.stream_length = 0; + params.stream = NULL; + params.token_length = message->pBuffers[token_idx].cbBuffer; + params.token = message->pBuffers[token_idx].pvBuffer; + } + else + { + if (!message->pBuffers[data_idx].pvBuffer) + { + message->pBuffers[data_idx].pvBuffer = RtlAllocateHeap( GetProcessHeap(), 0, KERBEROS_MAX_BUF ); + if (!message->pBuffers[data_idx].pvBuffer) return STATUS_NO_MEMORY; + message->pBuffers[data_idx].cbBuffer = KERBEROS_MAX_BUF; + } + params.stream_length = message->pBuffers[stream_idx].cbBuffer; + params.stream = message->pBuffers[stream_idx].pvBuffer; + params.token_length = 0; + params.token = NULL; + } params.data_length = message->pBuffers[data_idx].cbBuffer; params.data = message->pBuffers[data_idx].pvBuffer; - params.token_length = message->pBuffers[token_idx].cbBuffer; - params.token = message->pBuffers[token_idx].pvBuffer; params.qop = quality_of_protection; return KRB5_CALL( unseal_message, ¶ms ); diff --git a/dlls/kerberos/unixlib.c b/dlls/kerberos/unixlib.c index 93acecc1567..b4379fe0c34 100644 --- a/dlls/kerberos/unixlib.c +++ b/dlls/kerberos/unixlib.c @@ -1012,16 +1012,25 @@ static NTSTATUS unseal_message_no_vector( gss_ctx_id_t ctx, const struct unseal_ { gss_buffer_desc input, output; OM_uint32 ret, minor_status; - DWORD len_data, len_token; int conf_state; - len_data = params->data_length; - len_token = params->token_length; - - input.length = len_data + len_token; - if (!(input.value = malloc( input.length ))) return SEC_E_INSUFFICIENT_MEMORY; - memcpy( input.value, params->data, len_data ); - memcpy( (char *)input.value + len_data, params->token, len_token ); + if (params->stream_length) + { + input.length = params->stream_length; + if (!(input.value = malloc( input.length ))) return SEC_E_INSUFFICIENT_MEMORY; + memcpy( input.value, params->stream, params->stream_length ); + } + else + { + DWORD len_data, len_token; + + len_data = params->data_length; + len_token = params->token_length; + input.length = len_data + len_token; + if (!(input.value = malloc( input.length ))) return SEC_E_INSUFFICIENT_MEMORY; + memcpy( input.value, params->data, len_data ); + memcpy( (char *)input.value + len_data, params->token, len_token ); + } ret = pgss_unwrap( &minor_status, ctx, &input, &output, &conf_state, NULL ); free( input.value ); @@ -1030,7 +1039,7 @@ static NTSTATUS unseal_message_no_vector( gss_ctx_id_t ctx, const struct unseal_ if (ret == GSS_S_COMPLETE) { if (params->qop) *params->qop = (conf_state ? 0 : SECQOP_WRAP_NO_ENCRYPT); - memcpy( params->data, output.value, len_data ); + memcpy( params->data, output.value, output.length ); pgss_release_buffer( &minor_status, &output ); } @@ -1042,7 +1051,7 @@ static NTSTATUS unseal_message( void *args ) struct unseal_message_params *params = args; gss_ctx_id_t ctx = ctxhandle_sspi_to_gss( params->context ); - if (is_dce_style_context( ctx )) return unseal_message_vector( ctx, params ); + if (is_dce_style_context( ctx ) && !params->stream_length) return unseal_message_vector( ctx, params ); return unseal_message_no_vector( ctx, params ); } diff --git a/dlls/kerberos/unixlib.h b/dlls/kerberos/unixlib.h index 87516859991..1599d7de4f8 100644 --- a/dlls/kerberos/unixlib.h +++ b/dlls/kerberos/unixlib.h @@ -106,6 +106,8 @@ struct seal_message_params struct unseal_message_params { UINT64 context; + BYTE *stream; + ULONG stream_length; BYTE *data; ULONG data_length; BYTE *token; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8116
Yes, this would allow us to simplify sasl_decode() but only if we add support to NTLM as well. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116#note_104375
Hans Leidekker (@hans) commented about dlls/kerberos/unixlib.c:
struct unseal_message_params *params = args; gss_ctx_id_t ctx = ctxhandle_sspi_to_gss( params->context );
- if (is_dce_style_context( ctx )) return unseal_message_vector( ctx, params ); + if (is_dce_style_context( ctx ) && !params->stream_length) return unseal_message_vector( ctx, params );
This could use some testing. It should perhaps be an error if a SECBUFFER_STREAM is passed with a DCE context. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116#note_104378
Hans Leidekker (@hans) commented about dlls/kerberos/krb5_ap.c:
{ struct context_handle *context_handle = (void *)context; struct unseal_message_params params; - int data_idx, token_idx; + int stream_idx, data_idx, token_idx;
+ if ((stream_idx = get_buffer_index( message, SECBUFFER_STREAM )) == -1) + if ((token_idx = get_buffer_index( message, SECBUFFER_TOKEN )) == -1) return SEC_E_INVALID_TOKEN;
This introduces a compiler warning. We usually write this as an && condition BTW. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116#note_104376
Hans Leidekker (@hans) commented about dlls/kerberos/unixlib.c:
- DWORD len_data, len_token; int conf_state;
- len_data = params->data_length; - len_token = params->token_length; - - input.length = len_data + len_token; - if (!(input.value = malloc( input.length ))) return SEC_E_INSUFFICIENT_MEMORY; - memcpy( input.value, params->data, len_data ); - memcpy( (char *)input.value + len_data, params->token, len_token ); + if (params->stream_length) + { + input.length = params->stream_length; + if (!(input.value = malloc( input.length ))) return SEC_E_INSUFFICIENT_MEMORY; + memcpy( input.value, params->stream, params->stream_length ); + } We can actually avoid the buffer copy in this case.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116#note_104377
On Fri May 23 17:07:26 2025 +0000, Hans Leidekker wrote:
Yes, this would allow us to simplify sasl_decode() but only if we add support to NTLM as well. Since I don't have a working NTLM setup I was planning to use new code path only for Kerberos.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116#note_104381
On Fri May 23 17:07:27 2025 +0000, Hans Leidekker wrote:
This could use some testing. It should perhaps be an error if a SECBUFFER_STREAM is passed with a DCE context. I tried to write some code to see how SECBUFFER_STREAM support works under Windows. However I couldn't even get the code snippet for DecryptMessage() from https://learn.microsoft.com/en-us/windows/win32/secauthn/sspi-kerberos-inter... work. I guess that's because of Negotiate wrappers that insist to see DATA+TOKEN buffers as input.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116#note_104382
On Fri May 23 17:07:27 2025 +0000, Hans Leidekker wrote:
We can actually avoid the buffer copy in this case. Indeed, thanks.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116#note_104383
On Fri May 23 17:07:27 2025 +0000, Hans Leidekker wrote:
This introduces a compiler warning. We usually write this as an && condition BTW. Thanks.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116#note_104384
On Fri May 23 17:42:41 2025 +0000, Dmitry Timoshkov wrote:
Since I don't have a working NTLM setup I was planning to use new code path only for Kerberos. That would complicate sasl_decode() further, I don't think we want that. If we have NTLM support too we can actually simplify sasl_decode().
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116#note_104385
On Fri May 23 17:49:48 2025 +0000, Hans Leidekker wrote:
That would complicate sasl_decode() further, I don't think we want that. If we have NTLM support too we can actually simplify sasl_decode(). Since I can't test NTLM code I'd rather leave that task to someone who actually could test it. Can you help with it?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116#note_104386
On Fri May 23 17:48:45 2025 +0000, Dmitry Timoshkov wrote:
I tried to write some code to see how SECBUFFER_STREAM support works under Windows. However I couldn't even get the code snippet for DecryptMessage() from https://learn.microsoft.com/en-us/windows/win32/secauthn/sspi-kerberos-inter... work. I guess that's because of Negotiate wrappers that insist to see DATA+TOKEN buffers as input. I know, this stuff is hard to test. Among other things I'm using a modified wldap32 test and wldap32 dll (which includes a modified libldap) on Windows. This allows me to see how native SSPI behaves.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116#note_104387
On Fri May 23 17:56:25 2025 +0000, Dmitry Timoshkov wrote:
Since I can't test NTLM code I'd rather leave that task to someone who actually could test it. Can you help with it? I can take a look.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116#note_104388
On Fri May 23 18:01:00 2025 +0000, Hans Leidekker wrote:
I know, this stuff is hard to test. Among other things I'm using a modified wldap32 test and wldap32 dll (which includes a modified libldap) on Windows. This allows me to see how native SSPI behaves. I also tried to convince EncryptMessage() to produce a SECBUFFER_STREAM with a variety of inputs but failed. It doesn't appear to support it, which is a shame.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116#note_104390
On Fri May 23 18:14:57 2025 +0000, Hans Leidekker wrote:
I also tried to convince EncryptMessage() to produce a SECBUFFER_STREAM with a variety of inputs but failed. It doesn't appear to support it, which is a shame. Would something like https://web.mit.edu/freebsd/head/contrib/wpa/src/crypto/tls_schannel.c,tls_c...) work? Presumably composing a buffer as SECBUFFER_STREAM_HEADER+SECBUFFER_DATA+SECBUFFER_STREAM_TRAILER could be decoded as a single SECBUFFER_STREAM buffer?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116#note_104393
On Fri May 23 18:56:07 2025 +0000, Dmitry Timoshkov wrote:
Would something like https://web.mit.edu/freebsd/head/contrib/wpa/src/crypto/tls_schannel.c,tls_c...) work? Presumably composing a buffer as SECBUFFER_STREAM_HEADER+SECBUFFER_DATA+SECBUFFER_STREAM_TRAILER could be decoded as a single SECBUFFER_STREAM buffer? I think that's the buffer layout for SECPKG_FLAG_STREAM providers such as Schannel, Kerberos is not one of them.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8116#note_104394
participants (3)
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Hans Leidekker (＠hans)