Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NegotiateStream connections between Linux clients and Windows servers #99909

Merged
merged 7 commits into from
Mar 26, 2024

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Mar 18, 2024

Fixes #99227

Managed NTLM: Send the NegotiateSeal NTLM flag when client asks for ProtectionLevel.EncryptAndSign.

NegotiateStream: Process the last handshake done message. In case of SPNEGO protocol it may contain message integrity check. Additionally, if the negotiated protocol is NTLM then we need to reset the encryption keys after the message integrity check is verified.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 18, 2024
Comment on lines 886 to 891
if (message.Length > 0)
{
Debug.Assert(_context != null);
_context.GetOutgoingBlob(message, out NegotiateAuthenticationStatusCode statusCode);
_remoteOk = statusCode is NegotiateAuthenticationStatusCode.Completed;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review note:

This was likely broken for quite a long time and went unnoticed. Windows clients send raw NTLM instead of SPNEGO if NTLM is the only protocol available. Linux/macOS GSSAPI libraries are aware of this quirk and handle it too. In that case the last handshake message will be empty.

Skipping the processing of the last handshake message is mostly harmless. In SPNEGO it would contain the final message integrity check. Skipping this check could enable some man-in-the-middle attacks, at least in theory. In case of managed NTLM/SPNEGO, however, we reset the NTLM keys only after this very last message is processed. If this is skipped the client and server will have mismatched encryption keys. This is how the bug was discovered.

In theory we can align with GSSAPI by calling (_mechanism as ManagedNtlmNegotiateAuthenticationPal)?.ResetKeys(); here:

_mechanism.GetMIC(_spnegoMechList, micBuffer);
writer.WriteOctetString(micBuffer.WrittenSpan);

We would reset the key both after GetMIC and VerifyMIC instead of doing it only after VerifyMIC. It may not be desirable to do so though; when the API is used correctly and securely this is not an issue.

@filipnavara filipnavara marked this pull request as ready for review March 19, 2024 11:34
@filipnavara filipnavara requested review from wfurt and rzikm March 19, 2024 11:34
Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Seems like test is failing on some platforms.

@filipnavara
Copy link
Member Author

Seems like test is failing on some platforms.

I will have a look. Helix was rather unhealthy today so I forgot to check the test results.

ProtectionLevel.EncryptAndSign.

Process the last handshake done message in NegotiateStream. In case of
SPNEGO protocol it may contain message integrity check. Additionally,
if the negotiated protocol is NTLM then we need to reset the encryption
key after the message integrity check is verified.
@filipnavara
Copy link
Member Author

Turns out that I had one check order bug and one incorrect assumption in the test. The incorrect assumption was that I tried to check the scenario where the server doesn't support the NegotiateSeal attribute. The specification doesn't allow that:

If set, requests session key negotiation for message confidentiality. If the client sends NTLMSSP_NEGOTIATE_SEAL to the server in the NEGOTIATE_MESSAGE, the server MUST return NTLMSSP_NEGOTIATE_SEAL to the client in the CHALLENGE_MESSAGE.

Linux and the managed implementation handle it by returning error. Windows implementation silently assumes that the specification is followed and ignores that the server stripped NTLMSSP_NEGOTIATE_SEAL. I dropped the part of test since that's not what we are trying to fix anyway.

@filipnavara
Copy link
Member Author

Just when I thought the CI cannot be any more borken... it fails to start any pipelines at all.

@wfurt
Copy link
Member

wfurt commented Mar 19, 2024

thanks @filipnavara for digging deep into this.

  /repo/artifacts/bin/System.Net.Security.Enterprise.Tests/Debug/net9.0-unix /repo/src/libraries/System.Net.Security/tests/EnterpriseTests
    Discovering: System.Net.Security.Enterprise.Tests (method display = ClassAndMethod, method display options = None)
    Discovered:  System.Net.Security.Enterprise.Tests (found 6 test cases)
    Starting:    System.Net.Security.Enterprise.Tests (parallel test collections = on [4 threads], stop on fail = off)
  Process terminated. Assertion failed.
     at System.Net.Security.NegotiateStream.WriteAsync[TIOAdapter](ReadOnlyMemory`1 buffer, CancellationToken cancellationToken) in /_/src/libraries/System.Net.Security/src/System/Net/Security/NegotiateStream.cs:line 507
     at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine) in /_/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncMethodBuilderCore.cs:line 38
     at System.Net.Security.NegotiateStream.WriteAsync[TIOAdapter](ReadOnlyMemory`1 buffer, CancellationToken cancellationToken)

while your chance did not touched WriteAsync I'm wondering something changed. The buffer seems to be initialized upon successful authentication (but I only skimmed the code)

            else if (statusCode == NegotiateAuthenticationStatusCode.Completed)
            {
                _writeBuffer = new ArrayBufferWriter<byte>();

@filipnavara
Copy link
Member Author

while your chance did not touched WriteAsync I'm wondering something changed.

These correlate with a SecurityQosFailed error. I'll have a deeper look.

Also, the osx-x64 tests should be using Managed NTLM and should not fail like that.

…shakeComplete.

If HandshakeComplete is not true, then the authentication blob will get processed with the normal flow.
@filipnavara filipnavara requested review from wfurt and rzikm March 20, 2024 12:15
@filipnavara
Copy link
Member Author

Helix is in quite a disrepair for the past few days (auto-scaler fails to scale several queues; already reported). The relevant pipelines succeeded though (win-x86, osx-x64).

Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wfurt wfurt merged commit be1b035 into dotnet:main Mar 26, 2024
85 of 87 checks passed
@filipnavara filipnavara deleted the seal branch March 26, 2024 16:07
@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 2024
@karelz karelz added this to the 9.0.0 milestone May 14, 2024
@rzikm
Copy link
Member

rzikm commented May 14, 2024

/backport to release/8.0-staging

@github-actions github-actions bot unlocked this conversation May 14, 2024
Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/9084265804

Copy link
Contributor

@rzikm backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Send the NegotiateSeal NTLM flag when client asked for ProtectionLevel.EncryptAndSign.
Applying: Add test for the NegotiateSeal flag
Using index info to reconstruct a base tree...
M	src/libraries/System.Net.Security/tests/UnitTests/NegotiateAuthenticationTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Net.Security/tests/UnitTests/NegotiateAuthenticationTests.cs
Applying: Fix the test
Patch is empty.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To record the empty patch as an empty commit, run "git am --allow-empty".
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

@rzikm an error occurred while backporting to release/8.0-staging, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2024
@karelz
Copy link
Member

karelz commented May 30, 2024

Backport to 8.0 in PR #102216

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Security community-contribution Indicates that the PR has been added by a community member
Projects
None yet
4 participants