Fix NegotiateStream connections between Linux clients and Windows servers by filipnavara · Pull Request #99909 · dotnet/runtime

filipnavara · 2024-03-18T15:48:40Z

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.

dotnet-policy-service · 2024-03-18T15:51:58Z

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

filipnavara · 2024-03-18T16:02:20Z

+ if (message.Length > 0)
+ {
+ Debug.Assert(_context != null);
+ _context.GetOutgoingBlob(message, out NegotiateAuthenticationStatusCode statusCode);
+ _remoteOk = statusCode is NegotiateAuthenticationStatusCode.Completed;
+ }


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:
runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedSpnego.cs
Lines 350 to 351 in 1d2b1ae
_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.

rzikm

LGTM

wfurt

LGTM. Seems like test is failing on some platforms.

filipnavara · 2024-03-19T19:36:18Z

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 · 2024-03-19T21:12:59Z

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 · 2024-03-19T21:18:07Z

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

wfurt · 2024-03-19T22:42:18Z

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 · 2024-03-20T07:54:57Z

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.

… Managed NTLM

filipnavara · 2024-03-20T19:04:24Z

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).

rzikm

LGTM

wfurt

LGTM

rzikm · 2024-05-14T18:18:51Z

/backport to release/8.0-staging

github-actions · 2024-05-14T18:19:03Z

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

github-actions · 2024-05-14T18:20:34Z

@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 testfor 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!

github-actions · 2024-05-14T18:20:35Z

@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.

karelz · 2024-05-30T08:17:15Z

Backport to 8.0 in PR #102216

filipnavara · 2024-03-18T15:48:40Z

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.

dotnet-policy-service · 2024-03-18T15:51:58Z

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

filipnavara · 2024-03-18T16:02:20Z

+ if (message.Length > 0)
+ {
+ Debug.Assert(_context != null);
+ _context.GetOutgoingBlob(message, out NegotiateAuthenticationStatusCode statusCode);
+ _remoteOk = statusCode is NegotiateAuthenticationStatusCode.Completed;
+ }


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:
runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedSpnego.cs
Lines 350 to 351 in 1d2b1ae
_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.

rzikm

LGTM

wfurt

LGTM. Seems like test is failing on some platforms.

filipnavara · 2024-03-19T19:36:18Z

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 · 2024-03-19T21:12:59Z

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 · 2024-03-19T21:18:07Z

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

wfurt · 2024-03-19T22:42:18Z

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 · 2024-03-20T07:54:57Z

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.

… Managed NTLM

filipnavara · 2024-03-20T19:04:24Z

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).

rzikm

LGTM

wfurt

LGTM

rzikm · 2024-05-14T18:18:51Z

/backport to release/8.0-staging

github-actions · 2024-05-14T18:19:03Z

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

github-actions · 2024-05-14T18:20:34Z

@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 testfor 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!

github-actions · 2024-05-14T18:20:35Z

@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.

karelz · 2024-05-30T08:17:15Z

Backport to 8.0 in PR #102216

filipnavara · 2024-03-18T15:48:40Z

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.

dotnet-policy-service · 2024-03-18T15:51:58Z

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

filipnavara · 2024-03-18T16:02:20Z

+ if (message.Length > 0)
+ {
+ Debug.Assert(_context != null);
+ _context.GetOutgoingBlob(message, out NegotiateAuthenticationStatusCode statusCode);
+ _remoteOk = statusCode is NegotiateAuthenticationStatusCode.Completed;
+ }


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:
runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedSpnego.cs
Lines 350 to 351 in 1d2b1ae
_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.

rzikm

LGTM

wfurt

LGTM. Seems like test is failing on some platforms.

filipnavara · 2024-03-19T19:36:18Z

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 · 2024-03-19T21:12:59Z

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 · 2024-03-19T21:18:07Z

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

wfurt · 2024-03-19T22:42:18Z

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 · 2024-03-20T07:54:57Z

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.

… Managed NTLM

filipnavara · 2024-03-20T19:04:24Z

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).

rzikm

LGTM

wfurt

LGTM

rzikm · 2024-05-14T18:18:51Z

/backport to release/8.0-staging

github-actions · 2024-05-14T18:19:03Z

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

github-actions · 2024-05-14T18:20:34Z

@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 testfor 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!

github-actions · 2024-05-14T18:20:35Z

@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.

karelz · 2024-05-30T08:17:15Z

Backport to 8.0 in PR #102216

filipnavara · 2024-03-18T15:48:40Z

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.

dotnet-policy-service · 2024-03-18T15:51:58Z

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

filipnavara · 2024-03-18T16:02:20Z

+ if (message.Length > 0)
+ {
+ Debug.Assert(_context != null);
+ _context.GetOutgoingBlob(message, out NegotiateAuthenticationStatusCode statusCode);
+ _remoteOk = statusCode is NegotiateAuthenticationStatusCode.Completed;
+ }


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:
runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedSpnego.cs
Lines 350 to 351 in 1d2b1ae
_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.

rzikm

LGTM

wfurt

LGTM. Seems like test is failing on some platforms.

filipnavara · 2024-03-19T19:36:18Z

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 · 2024-03-19T21:12:59Z

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 · 2024-03-19T21:18:07Z

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

wfurt · 2024-03-19T22:42:18Z

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 · 2024-03-20T07:54:57Z

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.

… Managed NTLM

filipnavara · 2024-03-20T19:04:24Z

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).

rzikm

LGTM

wfurt

LGTM

rzikm · 2024-05-14T18:18:51Z

/backport to release/8.0-staging

github-actions · 2024-05-14T18:19:03Z

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

github-actions · 2024-05-14T18:20:34Z

@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 testfor 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!

github-actions · 2024-05-14T18:20:35Z

@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.

karelz · 2024-05-30T08:17:15Z

Backport to 8.0 in PR #102216

filipnavara · 2024-03-18T15:48:40Z

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.

dotnet-policy-service · 2024-03-18T15:51:58Z

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

filipnavara · 2024-03-18T16:02:20Z

+ if (message.Length > 0)
+ {
+ Debug.Assert(_context != null);
+ _context.GetOutgoingBlob(message, out NegotiateAuthenticationStatusCode statusCode);
+ _remoteOk = statusCode is NegotiateAuthenticationStatusCode.Completed;
+ }


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:
runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedSpnego.cs
Lines 350 to 351 in 1d2b1ae
_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.

rzikm

LGTM

wfurt

LGTM. Seems like test is failing on some platforms.

filipnavara · 2024-03-19T19:36:18Z

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 · 2024-03-19T21:12:59Z

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 · 2024-03-19T21:18:07Z

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

wfurt · 2024-03-19T22:42:18Z

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 · 2024-03-20T07:54:57Z

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.

… Managed NTLM

filipnavara · 2024-03-20T19:04:24Z

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).

rzikm

LGTM

wfurt

LGTM

rzikm · 2024-05-14T18:18:51Z

/backport to release/8.0-staging

github-actions · 2024-05-14T18:19:03Z

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

github-actions · 2024-05-14T18:20:34Z

@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 testfor 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!

github-actions · 2024-05-14T18:20:35Z

@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.

karelz · 2024-05-30T08:17:15Z

Backport to 8.0 in PR #102216

filipnavara · 2024-03-18T15:48:40Z

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.

dotnet-policy-service · 2024-03-18T15:51:58Z

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

filipnavara · 2024-03-18T16:02:20Z

+ if (message.Length > 0)
+ {
+ Debug.Assert(_context != null);
+ _context.GetOutgoingBlob(message, out NegotiateAuthenticationStatusCode statusCode);
+ _remoteOk = statusCode is NegotiateAuthenticationStatusCode.Completed;
+ }


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:
runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedSpnego.cs
Lines 350 to 351 in 1d2b1ae
_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.

rzikm

LGTM

wfurt

LGTM. Seems like test is failing on some platforms.

filipnavara · 2024-03-19T19:36:18Z

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 · 2024-03-19T21:12:59Z

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 · 2024-03-19T21:18:07Z

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

wfurt · 2024-03-19T22:42:18Z

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 · 2024-03-20T07:54:57Z

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.

… Managed NTLM

filipnavara · 2024-03-20T19:04:24Z

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).

rzikm

LGTM

wfurt

LGTM

rzikm · 2024-05-14T18:18:51Z

/backport to release/8.0-staging

github-actions · 2024-05-14T18:19:03Z

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

github-actions · 2024-05-14T18:20:34Z

@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 testfor 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!

github-actions · 2024-05-14T18:20:35Z

@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.

karelz · 2024-05-30T08:17:15Z

Backport to 8.0 in PR #102216

filipnavara · 2024-03-18T15:48:40Z

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.

dotnet-policy-service · 2024-03-18T15:51:58Z

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

filipnavara · 2024-03-18T16:02:20Z

+ if (message.Length > 0)
+ {
+ Debug.Assert(_context != null);
+ _context.GetOutgoingBlob(message, out NegotiateAuthenticationStatusCode statusCode);
+ _remoteOk = statusCode is NegotiateAuthenticationStatusCode.Completed;
+ }


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:
runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedSpnego.cs
Lines 350 to 351 in 1d2b1ae
_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.

rzikm

LGTM

wfurt

LGTM. Seems like test is failing on some platforms.

filipnavara · 2024-03-19T19:36:18Z

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 · 2024-03-19T21:12:59Z

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 · 2024-03-19T21:18:07Z

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

wfurt · 2024-03-19T22:42:18Z

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 · 2024-03-20T07:54:57Z

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.

… Managed NTLM

filipnavara · 2024-03-20T19:04:24Z

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).

rzikm

LGTM

wfurt

LGTM

rzikm · 2024-05-14T18:18:51Z

/backport to release/8.0-staging

github-actions · 2024-05-14T18:19:03Z

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

github-actions · 2024-05-14T18:20:34Z

@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 testfor 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!

github-actions · 2024-05-14T18:20:35Z

@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.

karelz · 2024-05-30T08:17:15Z

Backport to 8.0 in PR #102216

filipnavara · 2024-03-18T15:48:40Z

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.

dotnet-policy-service · 2024-03-18T15:51:58Z

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

filipnavara · 2024-03-18T16:02:20Z

+ if (message.Length > 0)
+ {
+ Debug.Assert(_context != null);
+ _context.GetOutgoingBlob(message, out NegotiateAuthenticationStatusCode statusCode);
+ _remoteOk = statusCode is NegotiateAuthenticationStatusCode.Completed;
+ }


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:
runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedSpnego.cs
Lines 350 to 351 in 1d2b1ae
_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.

rzikm

LGTM

wfurt

LGTM. Seems like test is failing on some platforms.

filipnavara · 2024-03-19T19:36:18Z

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 · 2024-03-19T21:12:59Z

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 · 2024-03-19T21:18:07Z

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

wfurt · 2024-03-19T22:42:18Z

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 · 2024-03-20T07:54:57Z

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.

… Managed NTLM

filipnavara · 2024-03-20T19:04:24Z

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).

rzikm

LGTM

wfurt

LGTM

rzikm · 2024-05-14T18:18:51Z

/backport to release/8.0-staging

github-actions · 2024-05-14T18:19:03Z

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

github-actions · 2024-05-14T18:20:34Z

@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 testfor 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!

github-actions · 2024-05-14T18:20:35Z

@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.

karelz · 2024-05-30T08:17:15Z

Backport to 8.0 in PR #102216

ghost added the area-System.Net.Security label Mar 18, 2024

dotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Mar 18, 2024

filipnavara commented Mar 18, 2024
View reviewed changes

filipnavara marked this pull request as ready for review March 19, 2024 11:34

filipnavara requested review from rzikm and wfurt March 19, 2024 11:34

rzikm approved these changes Mar 19, 2024
View reviewed changes

This was referenced Mar 19, 2024
Tracking issue for CI build timeouts #76454
Closed
RestApiException`1: The response contained an invalid status code 500 Internal Server Error dotnet/dnceng#2298
Closed
StackOverflowException on System.Runtime.Serialization.Xml.ReflectionOnly.Tests #99963
Closed

wfurt approved these changes Mar 19, 2024
View reviewed changes

filipnavara added 3 commits March 19, 2024 22:09

Add test for the NegotiateSeal flag
b1ce3de

Fix the test
a031ed4

filipnavara force-pushed the seal branch from 8d36989 to a031ed4Compare March 19, 2024 21:09

Dummy commit
a953c48

build-analysisBot mentioned this pull request Mar 20, 2024
"We stopped hearing from agent Azure Pipelines 32. Verify the agent machine is running and has a healthy network connection" dotnet/dnceng#1886
Open
3 tasks

filipnavara added 2 commits March 20, 2024 09:40

Fix the new _remoteOk logic in NegotiateStream to fire only when Hand…
48f3277
…shakeComplete. If HandshakeComplete is not true, then the authentication blob will get processed with the normal flow.

Fix the value of NegotiateSeal in the final authentication message of…
b7378f3
… Managed NTLM

filipnavara requested review from rzikm and wfurt March 20, 2024 12:15

Merge branch 'main' into seal
73faed6

build-analysisBot mentioned this pull request Mar 25, 2024
STATUS_UNSUCCESSFUL in RsaCryptRoundtrip_OaepSHA1 #29683
Open

rzikm approved these changes Mar 26, 2024
View reviewed changes

wfurt approved these changes Mar 26, 2024
View reviewed changes

wfurt merged commit be1b035 into dotnet:mainMar 26, 2024

filipnavara deleted the seal branch March 26, 2024 16:07

wfurt mentioned this pull request Apr 16, 2024
NET8 WcfClient (net-tcp binding) running on linux cannot be authenticated on WCF Server hosted on windows if UseManagedNtlm is set #99227
Closed

github-actionsBot locked and limited conversation to collaborators Apr 26, 2024

karelz added this to the 9.0.0 milestone May 14, 2024

github-actionsBot unlocked this conversation May 14, 2024

github-actionsBot locked as resolved and limited conversation to collaborators May 14, 2024