Skip to content

Commit 3bb45af

Browse files
filipnavarafilipnavara
authored
Avoid throwing Win32Exception from HTTP authentication (#70474)
* Avoid throwing Win32Exception from HTTP authentication When server sends malformed NTLM challenge the NT authentication processing would throw an unexpected Win32Exception from HttpClientHandler.Send[Async] calls. This aligns the behavior to WinHTTP handler where the Unauthorized reply with challenge token is returned back to the client. Similarly, failure to validate the last MIC token in Negotiate scheme could result in Win32Exception. Handle it by throwing HttpRequestException instead. * Make the unit test more resilient * Add trace to Negotiate authentication * Dispose connection instead of draining the response * Remove outdated ActiveIssue
1 parent e5acd4d commit 3bb45af

6 files changed

Lines changed: 137 additions & 21 deletions

File tree

‎src/libraries/Common/src/System/Net/NTAuthentication.Common.cs‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,11 @@ internal int MakeSignature(byte[] buffer, int offset, int count, [AllowNull] ref
163163
}
164164

165165
internal string? GetOutgoingBlob(string? incomingBlob)
166+
{
167+
return GetOutgoingBlob(incomingBlob, throwOnError: true, out _);
168+
}
169+
170+
internal string? GetOutgoingBlob(string? incomingBlob, bool throwOnError, out SecurityStatusPal statusCode)
166171
{
167172
byte[]? decodedIncomingBlob = null;
168173
if (incomingBlob != null && incomingBlob.Length > 0)
@@ -176,10 +181,11 @@ internal int MakeSignature(byte[] buffer, int offset, int count, [AllowNull] ref
176181
// we tried auth previously, now we got a null blob, we're done. this happens
177182
// with Kerberos & valid credentials on the domain but no ACLs on the resource
178183
_isCompleted = true;
184+
statusCode = new SecurityStatusPal(SecurityStatusPalErrorCode.OK);
179185
}
180186
else
181187
{
182-
decodedOutgoingBlob = GetOutgoingBlob(decodedIncomingBlob, true);
188+
decodedOutgoingBlob = GetOutgoingBlob(decodedIncomingBlob, throwOnError, out statusCode);
183189
}
184190

185191
string? outgoingBlob = null;

‎src/libraries/Common/src/System/Net/NTAuthentication.Managed.cs‎

Lines changed: 57 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,14 @@ internal sealed partial class NTAuthentication
5252

5353
private static readonly byte[] s_workstation = Encoding.Unicode.GetBytes(Environment.MachineName);
5454

55+
private static SecurityStatusPal SecurityStatusPalOk = new SecurityStatusPal(SecurityStatusPalErrorCode.OK);
56+
private static SecurityStatusPal SecurityStatusPalContinueNeeded = new SecurityStatusPal(SecurityStatusPalErrorCode.ContinueNeeded);
57+
private static SecurityStatusPal SecurityStatusPalInvalidToken = new SecurityStatusPal(SecurityStatusPalErrorCode.InvalidToken);
58+
private static SecurityStatusPal SecurityStatusPalInternalError = new SecurityStatusPal(SecurityStatusPalErrorCode.InternalError);
59+
private static SecurityStatusPal SecurityStatusPalPackageNotFound = new SecurityStatusPal(SecurityStatusPalErrorCode.PackageNotFound);
60+
private static SecurityStatusPal SecurityStatusPalMessageAltered = new SecurityStatusPal(SecurityStatusPalErrorCode.MessageAltered);
61+
private static SecurityStatusPal SecurityStatusPalLogonDenied = new SecurityStatusPal(SecurityStatusPalErrorCode.LogonDenied);
62+
5563
private const Flags s_requiredFlags =
5664
Flags.NegotiateNtlm2 | Flags.NegotiateNtlm | Flags.NegotiateUnicode | Flags.TargetName |
5765
Flags.NegotiateVersion | Flags.NegotiateKeyExchange | Flags.Negotiate128 |
@@ -291,7 +299,12 @@ internal void CloseContext()
291299
IsCompleted = true;
292300
}
293301

294-
internal unsafe string? GetOutgoingBlob(string? incomingBlob)
302+
internal string? GetOutgoingBlob(string? incomingBlob)
303+
{
304+
return GetOutgoingBlob(incomingBlob, throwOnError: true, out _);
305+
}
306+
307+
internal unsafe string? GetOutgoingBlob(string? incomingBlob, bool throwOnError, out SecurityStatusPal statusCode)
295308
{
296309
Debug.Assert(!IsCompleted);
297310

@@ -311,6 +324,7 @@ internal void CloseContext()
311324
CreateNtlmNegotiateMessage(_negotiateMessage);
312325

313326
decodedOutgoingBlob = _isSpNego ? CreateSpNegoNegotiateMessage(_negotiateMessage) : _negotiateMessage;
327+
statusCode = SecurityStatusPalContinueNeeded;
314328
}
315329
else
316330
{
@@ -319,9 +333,12 @@ internal void CloseContext()
319333
if (!_isSpNego)
320334
{
321335
IsCompleted = true;
336+
decodedOutgoingBlob = ProcessChallenge(decodedIncomingBlob, out statusCode);
337+
}
338+
else
339+
{
340+
decodedOutgoingBlob = ProcessSpNegoChallenge(decodedIncomingBlob, out statusCode);
322341
}
323-
324-
decodedOutgoingBlob = _isSpNego ? ProcessSpNegoChallenge(decodedIncomingBlob) : ProcessChallenge(decodedIncomingBlob);
325342
}
326343

327344
string? outgoingBlob = null;
@@ -604,7 +621,7 @@ private static byte[] DeriveKey(ReadOnlySpan<byte> exportedSessionKey, ReadOnlyS
604621
}
605622

606623
// This gets decoded byte blob and returns response in binary form.
607-
private unsafe byte[]? ProcessChallenge(byte[] blob)
624+
private unsafe byte[]? ProcessChallenge(byte[] blob, out SecurityStatusPal statusCode)
608625
{
609626
// TODO: Validate size and offsets
610627

@@ -615,6 +632,7 @@ private static byte[] DeriveKey(ReadOnlySpan<byte> exportedSessionKey, ReadOnlyS
615632
if (challengeMessage.Header.MessageType != MessageType.Challenge ||
616633
!NtlmHeader.SequenceEqual(asBytes.Slice(0, NtlmHeader.Length)))
617634
{
635+
statusCode = SecurityStatusPalInvalidToken;
618636
return null;
619637
}
620638

@@ -627,6 +645,7 @@ private static byte[] DeriveKey(ReadOnlySpan<byte> exportedSessionKey, ReadOnlyS
627645
// that is used for MIC.
628646
if ((flags & s_requiredFlags) != s_requiredFlags)
629647
{
648+
statusCode = SecurityStatusPalInvalidToken;
630649
return null;
631650
}
632651

@@ -638,6 +657,7 @@ private static byte[] DeriveKey(ReadOnlySpan<byte> exportedSessionKey, ReadOnlyS
638657
// Confidentiality is TRUE, then return STATUS_LOGON_FAILURE ([MS-ERREF] section 2.3.1).
639658
if (!hasNbNames && (flags & (Flags.NegotiateSign | Flags.NegotiateSeal)) != 0)
640659
{
660+
statusCode = SecurityStatusPalInvalidToken;
641661
return null;
642662
}
643663

@@ -733,6 +753,7 @@ private static byte[] DeriveKey(ReadOnlySpan<byte> exportedSessionKey, ReadOnlyS
733753

734754
Debug.Assert(payloadOffset == responseBytes.Length);
735755

756+
statusCode = SecurityStatusPalOk;
736757
return responseBytes;
737758
}
738759

@@ -834,7 +855,7 @@ private unsafe byte[] CreateSpNegoNegotiateMessage(ReadOnlySpan<byte> ntlmNegoti
834855
return writer.Encode();
835856
}
836857

837-
private unsafe byte[] ProcessSpNegoChallenge(byte[] challenge)
858+
private unsafe byte[]? ProcessSpNegoChallenge(byte[] challenge, out SecurityStatusPal statusCode)
838859
{
839860
NegState state = NegState.Unknown;
840861
string? mech = null;
@@ -894,9 +915,10 @@ private unsafe byte[] ProcessSpNegoChallenge(byte[] challenge)
894915

895916
challengeReader.ThrowIfNotEmpty();
896917
}
897-
catch (AsnContentException e)
918+
catch (AsnContentException)
898919
{
899-
throw new Win32Exception(NTE_FAIL, e.Message);
920+
statusCode = SecurityStatusPalInvalidToken;
921+
return null;
900922
}
901923

902924
if (blob?.Length > 0)
@@ -905,11 +927,19 @@ private unsafe byte[] ProcessSpNegoChallenge(byte[] challenge)
905927
// message with the challenge blob.
906928
if (!NtlmOid.Equals(mech))
907929
{
908-
throw new Win32Exception(NTE_FAIL, SR.Format(SR.net_nego_mechanism_not_supported, mech));
930+
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"Server requested unknown mechanism {mech}");
931+
statusCode = SecurityStatusPalPackageNotFound;
932+
return null;
909933
}
910934

911935
// Process decoded NTLM blob.
912-
byte[]? response = ProcessChallenge(blob);
936+
byte[]? response = ProcessChallenge(blob, out statusCode);
937+
938+
if (statusCode.ErrorCode != SecurityStatusPalErrorCode.OK)
939+
{
940+
return null;
941+
}
942+
913943
if (response?.Length > 0)
914944
{
915945
AsnWriter writer = new AsnWriter(AsnEncodingRules.DER);
@@ -930,21 +960,35 @@ private unsafe byte[] ProcessSpNegoChallenge(byte[] challenge)
930960
}
931961
}
932962

963+
statusCode = state == NegState.RequestMic ? SecurityStatusPalContinueNeeded : SecurityStatusPalOk;
933964
return writer.Encode();
934965
}
935966
}
936967

937968
if (mechListMIC != null)
938969
{
939-
if (_spnegoMechList == null || state != NegState.AcceptCompleted || !VerifyMIC(_spnegoMechList, mechListMIC))
970+
if (_spnegoMechList == null || state != NegState.AcceptCompleted)
940971
{
941-
throw new Win32Exception(NTE_FAIL);
972+
statusCode = SecurityStatusPalInternalError;
973+
return null;
974+
}
975+
976+
if (!VerifyMIC(_spnegoMechList, mechListMIC))
977+
{
978+
statusCode = SecurityStatusPalMessageAltered;
979+
return null;
942980
}
943981
}
944982

945983
IsCompleted = state == NegState.AcceptCompleted || state == NegState.Reject;
946-
947-
return Array.Empty<byte>();
984+
statusCode = state switch {
985+
NegState.AcceptCompleted => SecurityStatusPalOk,
986+
NegState.AcceptIncomplete => SecurityStatusPalContinueNeeded,
987+
NegState.Reject => SecurityStatusPalLogonDenied,
988+
_ => SecurityStatusPalInternalError
989+
};
990+
991+
return null;
948992
}
949993
}
950994
}

‎src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Authentication.cs‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System.Net.Sockets;
77
using System.Net.Test.Common;
88
using System.Text;
9+
using System.Threading;
910
using System.Threading.Tasks;
1011

1112
using Microsoft.DotNet.XUnitExtensions;
@@ -689,5 +690,64 @@ await LoopbackServerFactory.CreateClientAndServerAsync(
689690
_output.WriteLine(authHeaderValue);
690691
});
691692
}
693+
694+
[ConditionalFact(nameof(IsNtlmInstalled))]
695+
public async Task Credentials_BrokenNtlmFromServer()
696+
{
697+
if (IsWinHttpHandler && UseVersion >= HttpVersion20.Value)
698+
{
699+
return;
700+
}
701+
702+
await LoopbackServer.CreateClientAndServerAsync(
703+
async uri =>
704+
{
705+
using (HttpClientHandler handler = CreateHttpClientHandler())
706+
using (HttpClient client = CreateHttpClient(handler))
707+
{
708+
handler.Credentials = new NetworkCredential("username", "password");
709+
var response = await client.GetAsync(uri);
710+
Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode);
711+
}
712+
},
713+
async server =>
714+
{
715+
var responseHeader = new HttpHeaderData[] { new HttpHeaderData("WWW-Authenticate", "NTLM") };
716+
HttpRequestData requestData = await server.HandleRequestAsync(HttpStatusCode.Unauthorized, responseHeader);
717+
Assert.Equal(0, requestData.GetHeaderValueCount("Authorization"));
718+
719+
// Establish a session connection
720+
using var connection = await server.EstablishConnectionAsync();
721+
requestData = await connection.ReadRequestDataAsync();
722+
string authHeaderValue = requestData.GetSingleHeaderValue("Authorization");
723+
Assert.Contains("NTLM", authHeaderValue);
724+
_output.WriteLine(authHeaderValue);
725+
726+
// Incorrect NTLMv1 challenge from server (generated by Cyrus HTTP)
727+
responseHeader = new HttpHeaderData[] {
728+
new HttpHeaderData("WWW-Authenticate", "NTLM TlRMTVNTUAACAAAAHAAcADAAAACV/wIAUwCrhitz1vsAAAAAAAAAAAAAAAAAAAAASgAuAEUATQBDAEwASQBFAE4AVAAuAEMATwBNAA=="),
729+
new HttpHeaderData("Connection", "keep-alive")
730+
};
731+
await connection.SendResponseAsync(HttpStatusCode.Unauthorized, responseHeader);
732+
connection.CompleteRequestProcessing();
733+
734+
// Wait for the client to close the connection
735+
try
736+
{
737+
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(1000);
738+
await connection.WaitForCloseAsync(cancellationTokenSource.Token);
739+
}
740+
catch (OperationCanceledException)
741+
{
742+
// On Linux the GSSAPI NTLM provider may try to continue with the authentication, so go along with it
743+
requestData = await connection.ReadRequestDataAsync();
744+
authHeaderValue = requestData.GetSingleHeaderValue("Authorization");
745+
Assert.Contains("NTLM", authHeaderValue);
746+
_output.WriteLine(authHeaderValue);
747+
await connection.SendResponseAsync(HttpStatusCode.Unauthorized);
748+
connection.CompleteRequestProcessing();
749+
}
750+
});
751+
}
692752
}
693753
}

‎src/libraries/System.Net.Http/src/Resources/Strings.resx‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -441,15 +441,15 @@
441441
<data name="net_http_authconnectionfailure" xml:space="preserve">
442442
<value>Authentication failed because the connection could not be reused.</value>
443443
</data>
444+
<data name="net_http_authvalidationfailure" xml:space="preserve">
445+
<value>Authentication validation failed with error - {0}.</value>
446+
</data>
444447
<data name="net_nego_server_not_supported" xml:space="preserve">
445448
<value>Server implementation is not supported</value>
446449
</data>
447450
<data name="net_nego_protection_level_not_supported" xml:space="preserve">
448451
<value>Requested protection level is not supported with the GSSAPI implementation currently installed.</value>
449452
</data>
450-
<data name="net_nego_mechanism_not_supported" xml:space="preserve">
451-
<value>The security package '{0}' is not supported.</value>
452-
</data>
453453
<data name="net_context_buffer_too_small" xml:space="preserve">
454454
<value>Insufficient buffer space. Required: {0} Actual: {1}.</value>
455455
</data>

‎src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/AuthenticationHelper.NtAuth.cs‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -172,8 +172,9 @@ private static async Task<HttpResponseMessage> SendWithNtAuthAsync(HttpRequestMe
172172
{
173173
while (true)
174174
{
175-
string? challengeResponse = authContext.GetOutgoingBlob(challengeData);
176-
if (challengeResponse == null)
175+
SecurityStatusPal statusCode;
176+
string? challengeResponse = authContext.GetOutgoingBlob(challengeData, throwOnError: false, out statusCode);
177+
if (statusCode.ErrorCode > SecurityStatusPalErrorCode.TryAgain || challengeResponse == null)
177178
{
178179
// Response indicated denial even after login, so stop processing and return current response.
179180
break;
@@ -195,7 +196,13 @@ private static async Task<HttpResponseMessage> SendWithNtAuthAsync(HttpRequestMe
195196
if (!IsAuthenticationChallenge(response, isProxyAuth))
196197
{
197198
// Tail response for Negoatiate on successful authentication. Validate it before we proceed.
198-
authContext.GetOutgoingBlob(challengeData);
199+
authContext.GetOutgoingBlob(challengeData, throwOnError: false, out statusCode);
200+
if (statusCode.ErrorCode != SecurityStatusPalErrorCode.OK)
201+
{
202+
isNewConnection = false;
203+
connection.Dispose();
204+
throw new HttpRequestException(SR.Format(SR.net_http_authvalidationfailure, statusCode.ErrorCode), null, HttpStatusCode.Unauthorized);
205+
}
199206
break;
200207
}
201208

‎src/libraries/System.Net.Http/tests/EnterpriseTests/HttpClientAuthenticationTest.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ public void HttpClient_ValidAuthentication_Success(string url, bool useDomain, b
3838
}, url, useAltPort ? "true" : "" , useDomain ? "true" : "").Dispose();
3939
}
4040

41-
[ActiveIssue("https://github.com/dotnet/runtime/issues/416")]
4241
[Fact]
4342
public async Task HttpClient_InvalidAuthentication_Failure()
4443
{

0 commit comments

Comments
 (0)
⚡