[PATCH 4/5] kerberos: Allocate a buffer for the package info structure in kerberos_SpQueryContextAttributes.
Signed-off-by: Hans Leidekker <hans(a)codeweavers.com> --- dlls/kerberos/krb5_ap.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/dlls/kerberos/krb5_ap.c b/dlls/kerberos/krb5_ap.c index 8d02e97832..e2ac7d375a 100644 --- a/dlls/kerberos/krb5_ap.c +++ b/dlls/kerberos/krb5_ap.c @@ -46,6 +46,7 @@ #include "wine/heap.h" #include "wine/library.h" #include "wine/debug.h" +#include "wine/unicode.h" WINE_DEFAULT_DEBUG_CHANNEL(kerberos); @@ -1060,6 +1061,24 @@ static NTSTATUS NTAPI kerberos_SpDeleteContext( LSA_SEC_HANDLE context ) #endif } +static SecPkgInfoW *build_package_info( const SecPkgInfoW *info ) +{ + SecPkgInfoW *ret; + DWORD size_name = (strlenW(info->Name) + 1) * sizeof(WCHAR); + DWORD size_comment = (strlenW(info->Comment) + 1) * sizeof(WCHAR); + + if (!(ret = heap_alloc( sizeof(*ret) + size_name + size_comment ))) return NULL; + ret->fCapabilities = info->fCapabilities; + ret->wVersion = info->wVersion; + ret->wRPCID = info->wRPCID; + ret->cbMaxToken = info->cbMaxToken; + ret->Name = (SEC_WCHAR *)(ret + 1); + memcpy( ret->Name, info->Name, size_name ); + ret->Comment = (SEC_WCHAR *)((char *)ret->Name + size_name); + memcpy( ret->Comment, info->Comment, size_comment ); + return ret; +} + static NTSTATUS NTAPI kerberos_SpQueryContextAttributes( LSA_SEC_HANDLE context, ULONG attribute, void *buffer ) { TRACE( "(%lx %u %p)\n", context, attribute, buffer ); @@ -1104,7 +1123,7 @@ static NTSTATUS NTAPI kerberos_SpQueryContextAttributes( LSA_SEC_HANDLE context, case SECPKG_ATTR_NEGOTIATION_INFO: { SecPkgContext_NegotiationInfoW *info = (SecPkgContext_NegotiationInfoW *)buffer; - info->PackageInfo = (SecPkgInfoW *)&infoW; + if (!(info->PackageInfo = build_package_info( &infoW ))) return SEC_E_INSUFFICIENT_MEMORY; info->NegotiationState = SECPKG_NEGOTIATION_COMPLETE; return SEC_E_OK; } -- 2.11.0
Hans Leidekker <hans(a)codeweavers.com> wrote:
static NTSTATUS NTAPI kerberos_SpQueryContextAttributes( LSA_SEC_HANDLE context, ULONG attribute, void *buffer ) { TRACE( "(%lx %u %p)\n", context, attribute, buffer ); @@ -1104,7 +1123,7 @@ static NTSTATUS NTAPI kerberos_SpQueryContextAttributes( LSA_SEC_HANDLE context, case SECPKG_ATTR_NEGOTIATION_INFO: { SecPkgContext_NegotiationInfoW *info = (SecPkgContext_NegotiationInfoW *)buffer; - info->PackageInfo = (SecPkgInfoW *)&infoW; + if (!(info->PackageInfo = build_package_info( &infoW ))) return SEC_E_INSUFFICIENT_MEMORY; info->NegotiationState = SECPKG_NEGOTIATION_COMPLETE; return SEC_E_OK; }
I'd assume same thing as MSDN states in the SpGetInfo() notes: the provider is free to return pointers to dynamic and constant data in the returned buffer, and it's responsibility of LSA to copy data to a flat buffer before returning it to a client. Same comment applies to a similar patch for the NTLM provider. -- Dmitry.
On Thu, 2018-02-08 at 22:17 +0800, Dmitry Timoshkov wrote:
Hans Leidekker <hans(a)codeweavers.com> wrote:
static NTSTATUS NTAPI kerberos_SpQueryContextAttributes( LSA_SEC_HANDLE context, ULONG attribute, void *buffer ) { TRACE( "(%lx %u %p)\n", context, attribute, buffer ); @@ -1104,7 +1123,7 @@ static NTSTATUS NTAPI kerberos_SpQueryContextAttributes( LSA_SEC_HANDLE context, case SECPKG_ATTR_NEGOTIATION_INFO: { SecPkgContext_NegotiationInfoW *info = (SecPkgContext_NegotiationInfoW *)buffer; - info->PackageInfo = (SecPkgInfoW *)&infoW; + if (!(info->PackageInfo = build_package_info( &infoW ))) return SEC_E_INSUFFICIENT_MEMORY; info->NegotiationState = SECPKG_NEGOTIATION_COMPLETE; return SEC_E_OK; }
I'd assume same thing as MSDN states in the SpGetInfo() notes: the provider is free to return pointers to dynamic and constant data in the returned buffer, and it's responsibility of LSA to copy data to a flat buffer before returning it to a client. Same comment applies to a similar patch for the NTLM provider.
This buffer can currently be retrieved directly from NTLM, without involving LSA. This way we can free the buffer unconditionally in the negotiate tests. Things would change if NTLM was moved behing the LSA interface too, but in that case it's still not wrong to do it here, as long as the LSA wrapper and the provider agree.
Hans Leidekker <hans(a)codeweavers.com> wrote:
I'd assume same thing as MSDN states in the SpGetInfo() notes: the provider is free to return pointers to dynamic and constant data in the returned buffer, and it's responsibility of LSA to copy data to a flat buffer before returning it to a client. Same comment applies to a similar patch for the NTLM provider.
This buffer can currently be retrieved directly from NTLM, without involving LSA. This way we can free the buffer unconditionally in the negotiate tests. Things would change if NTLM was moved behing the LSA interface too, but in that case it's still not wrong to do it here, as long as the LSA wrapper and the provider agree.
In that case a buffer allocated by the provider will be leaked since LSA will do nothing with it. -- Dmitry.
Hans Leidekker <hans(a)codeweavers.com> wrote:
This buffer can currently be retrieved directly from NTLM, without involving LSA. This way we can free the buffer unconditionally in the negotiate tests. Things would change if NTLM was moved behing the LSA interface too, but in that case it's still not wrong to do it here, as long as the LSA wrapper and the provider agree.
Speaking of moving NTLM provider to SSP/AP msv1_0.dll, probably it's worth discussing how to do that. Do you think that using gss-ntlmssp instead of samba's ntlm_auth is an acceptable approach? -- Dmitry.
On Fri, 2018-02-09 at 12:28 +0800, Dmitry Timoshkov wrote:
Hans Leidekker <hans(a)codeweavers.com> wrote:
This buffer can currently be retrieved directly from NTLM, without involving LSA. This way we can free the buffer unconditionally in the negotiate tests. Things would change if NTLM was moved behing the LSA interface too, but in that case it's still not wrong to do it here, as long as the LSA wrapper and the provider agree.
Speaking of moving NTLM provider to SSP/AP msv1_0.dll, probably it's worth discussing how to do that. Do you think that using gss-ntlmssp instead of samba's ntlm_auth is an acceptable approach?
I haven't looked at gss-ntlmssp in detail but it would be interesting to use a proper library instead of Samba's ntlm_auth. In any case, considering the potential for regressions it's probably better to do such a change as a separate step.
Hans Leidekker <hans(a)codeweavers.com> wrote:
This buffer can currently be retrieved directly from NTLM, without involving LSA. This way we can free the buffer unconditionally in the negotiate tests. Things would change if NTLM was moved behing the LSA interface too, but in that case it's still not wrong to do it here, as long as the LSA wrapper and the provider agree.
Speaking of moving NTLM provider to SSP/AP msv1_0.dll, probably it's worth discussing how to do that. Do you think that using gss-ntlmssp instead of samba's ntlm_auth is an acceptable approach?
I haven't looked at gss-ntlmssp in detail but it would be interesting to use a proper library instead of Samba's ntlm_auth. In any case, considering the potential for regressions it's probably better to do such a change as a separate step.
Sure, if desired it could be even possible to have both: an old implementation in secur32 and a new one in msv1_0, and a temporary switch (at compile or run time) to choose one of them. -- Dmitry.
On Fri, 2018-02-09 at 17:13 +0800, Dmitry Timoshkov wrote:
Hans Leidekker <hans(a)codeweavers.com> wrote:
This buffer can currently be retrieved directly from NTLM, without involving LSA. This way we can free the buffer unconditionally in the negotiate tests. Things would change if NTLM was moved behing the LSA interface too, but in that case it's still not wrong to do it here, as long as the LSA wrapper and the provider agree.
Speaking of moving NTLM provider to SSP/AP msv1_0.dll, probably it's worth discussing how to do that. Do you think that using gss-ntlmssp instead of samba's ntlm_auth is an acceptable approach?
I haven't looked at gss-ntlmssp in detail but it would be interesting to use a proper library instead of Samba's ntlm_auth. In any case, considering the potential for regressions it's probably better to do such a change as a separate step.
Sure, if desired it could be even possible to have both: an old implementation in secur32 and a new one in msv1_0, and a temporary switch (at compile or run time) to choose one of them.
Keeping the old implementation lowers the pressure to fix the new implementation. I think we should to build an extensive test suite, make sure it passes and then have a flag day, with ample time before the next stable release.
Hans Leidekker <hans(a)codeweavers.com> wrote:
This buffer can currently be retrieved directly from NTLM, without involving LSA. This way we can free the buffer unconditionally in the negotiate tests. Things would change if NTLM was moved behing the LSA interface too, but in that case it's still not wrong to do it here, as long as the LSA wrapper and the provider agree.
Speaking of moving NTLM provider to SSP/AP msv1_0.dll, probably it's worth discussing how to do that. Do you think that using gss-ntlmssp instead of samba's ntlm_auth is an acceptable approach?
I haven't looked at gss-ntlmssp in detail but it would be interesting to use a proper library instead of Samba's ntlm_auth. In any case, considering the potential for regressions it's probably better to do such a change as a separate step.
Sure, if desired it could be even possible to have both: an old implementation in secur32 and a new one in msv1_0, and a temporary switch (at compile or run time) to choose one of them.
Keeping the old implementation lowers the pressure to fix the new implementation.
Something suggests me that it mostly depends on the default choice :)
I think we should to build an extensive test suite, make sure it passes and then have a flag day, with ample time before the next stable release.
Sounds like a nice answer: "Don't look at me!" :) -- Dmitry.
participants (2)
-
Dmitry Timoshkov -
Hans Leidekker