[PATCH v2 0/1] MR5286: kerberos: Allocate memory for the returned token if requested.

[Dmitry Timoshkov (＠dmitry)]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56400

--
  v2: kerberos: Allocate memory for the output token if requested.

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

[PATCH v2 1/1] kerberos: Allocate memory for the output token if requested.

[Dmitry Timoshkov]

From: Dmitry Timoshkov

Signed-off-by: Dmitry Timoshkov 
---
 dlls/kerberos/krb5_ap.c | 10 +++++++++-
 dlls/kerberos/unixlib.c |  2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/dlls/kerberos/krb5_ap.c b/dlls/kerberos/krb5_ap.c
index ae68c4b90c2..6d18aae28b2 100644
--- a/dlls/kerberos/krb5_ap.c
+++ b/dlls/kerberos/krb5_ap.c
@@ -477,7 +477,7 @@ static NTSTATUS NTAPI kerberos_SpInitLsaModeContext( LSA_SEC_HANDLE credential,
 {
     static const ULONG supported = ISC_REQ_CONFIDENTIALITY | ISC_REQ_INTEGRITY | ISC_REQ_SEQUENCE_DETECT |
                                    ISC_REQ_REPLAY_DETECT | ISC_REQ_MUTUAL_AUTH | ISC_REQ_USE_DCE_STYLE |
-                                   ISC_REQ_IDENTIFY | ISC_REQ_CONNECTION | ISC_REQ_DELEGATE;
+                                   ISC_REQ_IDENTIFY | ISC_REQ_CONNECTION | ISC_REQ_DELEGATE | ISC_REQ_ALLOCATE_MEMORY;
     char *target = NULL;
     NTSTATUS status;
     ULONG exptime;
@@ -513,6 +513,14 @@ static NTSTATUS NTAPI kerberos_SpInitLsaModeContext( LSA_SEC_HANDLE credential,
         }

if ((idx = get_buffer_index( output, SECBUFFER_TOKEN )) == -1) return SEC_E_INVALID_TOKEN;
+        if (context_req & ISC_REQ_ALLOCATE_MEMORY)
+        {
+            output->pBuffers[idx].pvBuffer = RtlAllocateHeap( GetProcessHeap(), 0, KERBEROS_MAX_BUF );
+            if (!output->pBuffers[idx].pvBuffer) return STATUS_NO_MEMORY;
+            output->pBuffers[idx].cbBuffer = KERBEROS_MAX_BUF;
+            if (context_attr) *context_attr = ISC_RET_ALLOCATED_MEMORY;
+        }
+        else if (context_attr) *context_attr = 0;
         params.output_token = output->pBuffers[idx].pvBuffer;
         params.output_token_length = &output->pBuffers[idx].cbBuffer;

diff --git a/dlls/kerberos/unixlib.c b/dlls/kerberos/unixlib.c
index 5bc1f72981f..b1f25e8bbcc 100644
--- a/dlls/kerberos/unixlib.c
+++ b/dlls/kerberos/unixlib.c
@@ -770,7 +770,7 @@ static NTSTATUS initialize_context( void *args )
         pgss_release_buffer( &minor_status, &output_token );

ctxhandle_gss_to_sspi( ctx_handle, params->new_context );
-        if (params->context_attr) *params->context_attr = flags_gss_to_isc_ret( ret_flags );
+        if (params->context_attr) *params->context_attr |= flags_gss_to_isc_ret( ret_flags );
         *params->expiry = expiry_time;
     }

[Hans Leidekker (＠hans)]

Hans Leidekker (@hans) commented about dlls/kerberos/krb5_ap.c:
>      }
>
>      if ((idx = get_buffer_index( output, SECBUFFER_TOKEN )) == -1) return SEC_E_INVALID_TOKEN;
> -
>    if (context_req & ISC_REQ_ALLOCATE_MEMORY)
> -
>    {
> -
>        output->pBuffers[idx].pvBuffer = RtlAllocateHeap( GetProcessHeap(), 0, KERBEROS_MAX_BUF );
> -
>        if (!output->pBuffers[idx].pvBuffer) return STATUS_NO_MEMORY;

'target' should be freed before returning.

[Hans Leidekker (＠hans)]

Hans Leidekker (@hans) commented about dlls/kerberos/unixlib.c:
>      pgss_release_buffer( &minor_status, &output_token );
>
>      ctxhandle_gss_to_sspi( ctx_handle, params->new_context );
> -
>    if (params->context_attr) *params->context_attr = flags_gss_to_isc_ret( ret_flags );
> -
>    if (params->context_attr) *params->context_attr |= flags_gss_to_isc_ret( ret_flags );

It would be better to add the flag in kerberos_SpInitLsaModeContext() when the context handle is created.

[Dmitry Timoshkov (＠dmitry)]

> >      if ((idx = get_buffer_index( output, SECBUFFER_TOKEN )) == -1) return SEC_E_INVALID_TOKEN;
> > -
> >    if (context_req & ISC_REQ_ALLOCATE_MEMORY)
> > -
> >    {
> > -
> >        output->pBuffers[idx].pvBuffer = RtlAllocateHeap( GetProcessHeap(), 0, KERBEROS_MAX_BUF );
> > -
> >        if (!output->pBuffers[idx].pvBuffer) return STATUS_NO_MEMORY;
>
> 'target' should be freed before returning.

Same problem already exists when get_buffer_index() above fails, it probably
should be fixed as well.
> >      ctxhandle_gss_to_sspi( ctx_handle, params->new_context );
> > -
> >    if (params->context_attr) *params->context_attr = flags_gss_to_isc_ret( ret_flags );
> > -
> >    if (params->context_attr) *params->context_attr |= flags_gss_to_isc_ret( ret_flags );
>
> It would be better to add the flag in kerberos_SpInitLsaModeContext() when the context handle is created.

It's not clear what should happen in the case of a failure. Should the token
buffer be freed or just the flag set for the caller?