Fix implementation of NegotiateAuthentication.Wrap for Kerberos on Windows

[filipnavara]

Fixes PowerShell/PowerShell#20168

The bug existed in .NET 7 but it was not uncovered until #86948 exposed it through NegotiateStream APIs.

Unfortunately, we lack testing infrastructure for testing Kerberos on Windows. NegotiateStreamKerberosTest outer loop test is supposed to cover it but I am not sure if it's even executed in any reasonable fashion. Thanks to @jborean93 for manually testing it on the affected use case in PowerShell.

[ghost]

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

Issue Details

---

Fixes PowerShell/PowerShell#20168

The bug existed in .NET 7 but it was not uncovered until #86948 exposed it through NegotiateStream APIs.

Unfortunately, we lack testing infrastructure for testing Kerberos on Windows. NegotiateStreamKerberosTest outer loop test is supposed to cover it but I am not sure if it's even executed in any reasonable fashion. Thanks to @jborean93 for manually testing it on the affected use case in PowerShell.

Author:	filipnavara
Assignees:	-
Labels:	area-System.Net.Security, community-contribution
Milestone:	-

[rzikm]

LGTM, Thanks!

[rzikm]

/azp run runtime libraries-coreclr-outerloop

[azure-pipelines]

No pipelines are associated with this pull request.

[rzikm]

/azp list

[azure-pipelines]

CI/CD Pipelines for this repository:

• runtime-coreclr outerloop
• runtime-coreclr jitstress
• runtime-coreclr jitstressregs
• runtime-coreclr jitstress2-jitstressregs
• runtime-coreclr gcstress0x3-gcstress0xc
• runtime-coreclr gcstress-extra
• runtime-coreclr r2r-extra
• runtime-coreclr jitstress-isas-x86
• runtime-coreclr jitstress-isas-arm
• runtime-coreclr jitstressregs-x86
• runtime-coreclr libraries-jitstressregs
• runtime-coreclr libraries-jitstress2-jitstressregs
• runtime-coreclr r2r
• runtime-coreclr runincontext
• runtime-coreclr crossgen2
• runtime-libraries-coreclr outerloop
• runtime-libraries-coreclr outerloop-windows
• runtime-libraries-coreclr outerloop-linux
• runtime-libraries-coreclr outerloop-osx
• runtime
• runtime-libraries enterprise-linux
• runtime-libraries stress-http
• runtime-libraries stress-ssl
• runtime-dev-innerloop
• runtime-coreclr crossgen2 outerloop
• coreclr-release-outerloop-nightly
• runtime-coreclr crossgen2-composite
• runtime-jit-experimental
• runtime-coreclr libraries-jitstress
• dotnet-linker-tests
• runtime-coreclr ilasm
• runtime-coreclr crossgen2-composite gcstress
• runtime-staging
• runtime-coreclr pgo
• runtime-coreclr libraries-pgo
• Antigen
• runtime-community
• Fuzzlyn
• runtime-coreclr superpmi-replay
• runtime-wasm
• runtime-coreclr superpmi-diffs
• runtime-coreclr superpmi-asmdiffs-checked-release
• runtime-extra-platforms
• jit-cfg
• runtime-wasm-perf
• runtime-llvm
• runtime-coreclr jitstress-random
• runtime-coreclr libraries-jitstress-random
• runtime-android-grpc-client-tests
• runtime-wasm-libtests
• runtime-wasm-non-libtests
• runtime-android
• runtime-androidemulator
• runtime-ioslike
• runtime-ioslikesimulator
• runtime-linuxbionic
• runtime-maccatalyst
• runtime-coreclr pgostress
• runtime-coreclr jitstress-isas-avx512
• runtime-tools-tests
• runtime-libraries-mono outerloop
• runtime-sanitized
• runtime-wasm-dbgtests
• runtime-wasm-optional

[rzikm]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[rzikm]

CI failures are unrelated, we are good to merge. I will give @wfurt a chance to have a look at this before merging. Thanks, Filip!

[filipnavara]

Thanks for review and running the tests!

[wfurt]

LGTM

[wfurt]

should we try to take it to 8.0 @filipnavara? The fact that it impacts Powershell is concerning IMHO @karelz

[filipnavara]

should we try to take it to 8.0 @filipnavara? The fact that it impacts Powershell is concerning IMHO @karelz

Almost certainly yes.

[karelz]

@filipnavara is there a summary explanation, why we the bug is now exposed more in 8.0? (that will help to decide if we should take it for 8.0)

[filipnavara]

Sure. Here's the simple explanation, I can stretch it out if necessary... ;-)

In .NET 7 the NegotiateStream implementation used internal NegotiateStreamPal.Encrypt method to encrypt messages on Windows. PR #86948 switched the encryption to use the NegotiateAuthentication.Wrap public method instead. The only difference between these two methods should have been that Encrypt produces extra 4-byte length prefix. Unfortunately, the implementation of Wrap turned out to have more unintended differences which got exposed by this switch.

Wrap as implemented in .NET 7 and .NET 8 before this fix worked for NTLM because the size of signature/encryption prefix is fixed and there's no encryption padding. In Kerberos the prefix length can be different, and some encryption schemes need additional padding. This PR matches what Encrypt method did in .NET 7 in terms of the buffer management.

[rzikm]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6021734007