Skip to content

Conversation

@filipnavara
Copy link
Member

@filipnavara filipnavara commented Feb 6, 2024

Partial backport of PR #96712.

Fixes #97942.

Customer Impact

  • Customer reported
  • Found internally

The NegotiateStream API sends incorrectly wrapped data on Windows when ProtectionLevel.Sign is used and NTLM is used as the underlying algorithm. Other platforms are not affected, other protection levels like ProtectionLevel.EncryptAndSign are not affected either.

Regression

  • Yes
  • No

Regression was introduced with PR #86948 in .NET 8. We lacked sufficient testing which was rectified in #96712 in .NET 9 when public APIs were added to expose the internal method. This PR backports the bug fix that was merged as part of PR #96712.

Testing

Tests were added in #96712 in main branch when the functionality was exposed as public API. The bug in the Windows implementation was uncovered by those tests and fixed. The customer who reported this issue in #97942 for .NET 8 has verified that the scenario is now fixed on .NET 9 nightly builds.

Specific changes in this PR have also been tested against the repro provided in #97942.

Risk

Low

The error is well understood, and the fix was already done in main branch last month. .NET 8 exposed this implementation detail only through the NegotiateStream API in a specific combination of parameters.

@ghost ghost added area-System.Net.Security community-contribution Indicates that the PR has been added by a community member labels Feb 6, 2024
@ghost
Copy link

ghost commented Feb 6, 2024

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

Issue Details

Fixes #97942.

Customer Impact

  • Customer reported
  • Found internally

The NegotiateStream API sends incorrectly wrapped data on Windows when ProtectionLevel.Sign is used and NTLM is used as the underlying algorithm. Other platforms are not affected, other protection levels like ProtectionLevel.EncryptAndSign are not affected either.

Regression

  • Yes
  • No

Regression was introduced in PR #86948 in .NET 8. We lacked sufficient testing which was rectified in #96712 in .NET 9 when public APIs were added to expose the internal method. This PR backports the bug fix that was merged as part of PR #96712.

Testing

Tests were added in #96712 in main branch when the functionality was exposed as public API. The bug in the Windows implementation was uncovered by those tests and fixed. The customer who reported this issue in #97942 for .NET 8 has verified that the scenario is now fixed on .NET 9 nightly builds.

Risk

Low

The error is well understood, and the fix was already done in main branch last month.

Author: filipnavara
Assignees: -
Labels:

area-System.Net.Security

Milestone: -

@rzikm rzikm requested a review from wfurt February 6, 2024 12:35
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

@filipnavara filipnavara added the Servicing-consider Issue for next servicing release review label Feb 6, 2024
@akoeplinger akoeplinger added this to the 8.0.x milestone Feb 7, 2024
@danmoseley
Copy link
Member

please get approval by mail for this one

@carlossanlop
Copy link
Contributor

Friendly reminder that Monday February 12th is the Code Complete deadline for the March Release.

@rzikm rzikm changed the title [release/8.0-staging] Fix Windows implementation of NegotiateAuthenticationPal.GetMIC [release/8.0] Fix Windows implementation of NegotiateAuthenticationPal.GetMIC Feb 9, 2024
@rzikm rzikm self-assigned this Feb 9, 2024
@stephentoub
Copy link
Member

We don't want to backport the tests as well?

@filipnavara
Copy link
Member Author

We don't want to backport the tests as well?

The tests use new public API that was introduced in .NET 9. It makes it rather non-trivial to port them.

@rzikm
Copy link
Member

rzikm commented Feb 12, 2024

Approved via email

@rzikm rzikm added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 12, 2024
@rzikm
Copy link
Member

rzikm commented Feb 12, 2024

Ci is green

@rzikm rzikm merged commit 2190e9b into dotnet:release/8.0-staging Feb 12, 2024
@filipnavara
Copy link
Member Author

Thanks for taking care of it!

@github-actions github-actions bot locked and limited conversation to collaborators Mar 13, 2024
@karelz karelz modified the milestones: 8.0.x, 8.0.3 Jun 25, 2024
@filipnavara filipnavara deleted the getmic-backport branch June 5, 2025 07:34
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 Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants