Fix incorrect buffer reallocation in AcceptSecurityContext #72184
Conversation
ghost
added
area-System.Net.Security
community-contribution
|
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue DetailsFixes #72152
|
|
Can you help me understand what the issue was and why this fixes it? Thanks! |
There are multiple implementation of AcceptSecurityContext in Windows API. The one used in TLS negotiations ("Schannel" in the MSDN documentation) gets empty buffers to start with and a flag that tells the native code to allocate all the buffers with appropriate size. You get back the allocated buffers. These have to be copied and returned as new When doing Negotiate authentication through the AcceptSecurityContext API it behaves differently. The caller provides a pre-allocated buffer and the same buffer is echoed in the output along with the actual length of the data that was written into the buffer. There's only single managed wrapper for AcceptSecurityContext that handles both situations. In the context of Negotiate authentication it incorrectly ignored the allocation flag and created a copy of the data in the output buffer into a new There was an assumption that on Windows the subsequent call to AcceptSecurityContext from NegotiateAuthentication will reuse the same buffer that is guaranteed to be big enough. This assumption was broken because the buffer was incorrectly replaced with the newly allocated buffer containing the first token. If the next authentication token was bigger than the first one then the server-side of the authentication failed with an exception. |
|
FWIW I think this mess is consequence of mixing Schannel interop bindings with the SSPI ones. Similarly, the mixing of Unix and Windows code for the |
|
Thanks. Very helpful explanation. Given "This assumption was broken because the buffer was incorrectly replaced with the newly allocated buffer containing the first token. If the next authentication token was bigger than the first one then the server-side of the authentication failed with an exception.", is there a way to trigger this in some way in a test? |
It's tricky given the infrastructure and system dependencies. I have an open PR that tests the Unix version of the code path. I cannot think of an easy test for Windows right now but I will definitely give it a second thought. What I can do with little effort is add |
|
Sounds good, thanks! |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
It seems like there is enough difference in Windows 11 / Server 2022 that it may be worth of adding at least one of them to "normal" PR test run. It is easy to miss the runtime-extra-platforms. any thought on that @danmoseley ? |
Can we switch one of the default pipelines over instead? I find it hard to figure out from the yml what the default pipelines use, but I thought Windows 11 was in the mix. Eg., here? Didn't we do that last fall? |
|
can you please try back-port it to P7 @rzikm? |
|
I think that ship has sailed already, but I can try tomorrow morning |
|
/backport to release/7.0-preview7 |
|
Started backporting to release/7.0-preview7: https://github.com/dotnet/runtime/actions/runs/2685763716 |
|
(I doubt I have the appropriate GitHub Actions rights but it should work for you, I can fill in the template if necessary) |
|
@filipnavara an error occurred while backporting to release/7.0-preview7, please check the run log for details! Error: @filipnavara is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=filipnavara |
|
/backport to release/7.0-preview7 |
|
Started backporting to release/7.0-preview7: https://github.com/dotnet/runtime/actions/runs/2685774511 |
|
So, yeah, funnily enough I can trigger the backport but I cannot edit the top comment :-) |
ghost
locked as resolved and limited conversation to collaborators
Fixes #72152