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 SSPI ComputeIntegrityCheck with Sign level #105605

Merged
merged 2 commits into from
Aug 20, 2024
Merged

Conversation

jborean93
Copy link
Contributor

Fix calling NegotiateAuthentication.ComputeIntegrityCheck on SSPI when the negotiation context was built with ProtectionLevel.Sign. The SECQOP_WRAP_NO_ENCRYPT QoP flag should not be set when calling GetMIC as no encryption is involved and some authentication providers fail when this is set.

Fix #103461

Fix calling NegotiateAuthentication.ComputeIntegrityCheck on SSPI when
the negotiation context was built with ProtectionLevel.Sign. The
SECQOP_WRAP_NO_ENCRYPT QoP flag should not be set when calling GetMIC as
no encryption is involved and some authentication providers fail when
this is set.

Fix dotnet#103461
@jborean93
Copy link
Contributor Author

The failure occurred when using Kerberos on SSPI which isn't possible to test on CI. I've verified it manually with the reproducer in #103461 as well as the SshClient I was originally using to test this scenario out.

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, Thanks!

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. Is there risk that this can impact typical authentication @filipnavara ?

@filipnavara
Copy link
Member

LGTM. Is there risk that this can impact typical authentication @filipnavara ?

TL;DR: No, we don't use this API in the runtime itself.

The API is used in the runtime only for implementing SPNEGO (Unix only managed impl.) and for NegotiateStream. In NegotiateStream it's only used for NTLM wrapping. This should not affect it at all, and it's covered by unit tests.

@jborean93
Copy link
Contributor Author

jborean93 commented Aug 19, 2024

Just checking if this is waiting on anything from myself like rebasing the commit from the latest. Just hoping this can be preset in .NET 9 where this API was introduced publicly.

@rzikm
Copy link
Member

rzikm commented Aug 20, 2024

Just checking if this is waiting on anything from myself like rebasing the commit from the latest. Just hoping this can be preset in .NET 9 where this API was introduced publicly.

Sorry, this has slipped our attention. It looks good to merge. Unfortunately, we are past the .NET 9.0 branch off, so now we need to treat this as regular servicing.

@jborean93 how much does the bug affect you? Is there a workaround?

@karelz this is fixing a bug in new API introduced in .NET 9, how strong business justification do we need to get this serviced?

@rzikm rzikm merged commit 3622bfa into dotnet:main Aug 20, 2024
79 of 84 checks passed
@jborean93 jborean93 deleted the sspi-qop branch August 20, 2024 09:02
@jborean93
Copy link
Contributor Author

It’s not the end of the world, I am not familiar with the back porting policies and assumed it would have been easy to do so. If it’s not then that’s fine, especially since it’s not an LTS release.

@wfurt
Copy link
Member

wfurt commented Aug 20, 2024

I think we should try to get it in. I'll try to figure it with @karelz

@karelz karelz added this to the 9.0.0 milestone Sep 3, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2024
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
Development

Successfully merging this pull request may close these issues.

NegotiateAuthentication.ComputeIntegrityCheck on Windows sets an invalid QOP
5 participants