From 716e5f7d2d1ff8a4cff43ad0edcb20cb338a99b3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 3 Jun 2025 12:09:28 +0000 Subject: [PATCH 1/5] Initial plan for issue From 30569cdd9c42498c939922ba29cf9757c026a119 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 3 Jun 2025 12:40:08 +0000 Subject: [PATCH 2/5] Add ASCII validation for header values in WinHttpHandler Co-authored-by: ManickaP <11718369+ManickaP@users.noreply.github.com> --- .../src/System/Net/Http/WinHttpHandler.cs | 33 ++++++++-- .../tests/UnitTests/WinHttpHandlerTest.cs | 62 +++++++++++++++++++ 2 files changed, 91 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs index 00617aee48378e..1e19211f8d8b63 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs +++ b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs @@ -709,6 +709,26 @@ private static WinHttpChunkMode GetChunkedModeForSend(HttpRequestMessage request return chunkedMode; } + private static bool IsAscii(string value) + { + for (int i = 0; i < value.Length; i++) + { + if (value[i] > 127) + { + return false; + } + } + return true; + } + + private static void ValidateHeadersForAscii(string headers) + { + if (!IsAscii(headers)) + { + throw new HttpRequestException("Request headers must contain only ASCII characters."); + } + } + private static void AddRequestHeaders( SafeWinHttpHandle requestHandle, HttpRequestMessage requestMessage, @@ -734,14 +754,17 @@ private static void AddRequestHeaders( string? cookieHeader = WinHttpCookieContainerAdapter.GetCookieHeader(requestMessage.RequestUri, cookies); if (!string.IsNullOrEmpty(cookieHeader)) { + ValidateHeadersForAscii(cookieHeader); requestHeadersBuffer.AppendLine(cookieHeader); } } - // Serialize general request headers. - requestHeadersBuffer.AppendLine(requestMessage.Headers.ToString()); + // Serialize general request headers and validate for ASCII. + string generalHeaders = requestMessage.Headers.ToString(); + ValidateHeadersForAscii(generalHeaders); + requestHeadersBuffer.Append(generalHeaders); - // Serialize entity-body (content) headers. + // Serialize entity-body (content) headers and validate for ASCII. if (requestMessage.Content != null) { // TODO https://github.com/dotnet/runtime/issues/16162: @@ -754,7 +777,9 @@ private static void AddRequestHeaders( requestMessage.Content.Headers.ContentLength = contentLength; } - requestHeadersBuffer.AppendLine(requestMessage.Content.Headers.ToString()); + string contentHeaders = requestMessage.Content.Headers.ToString(); + ValidateHeadersForAscii(contentHeaders); + requestHeadersBuffer.Append(contentHeaders); } // Add request headers to WinHTTP request handle. diff --git a/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/WinHttpHandlerTest.cs b/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/WinHttpHandlerTest.cs index 15f7accca5d46f..8a7490eead6d82 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/WinHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/WinHttpHandlerTest.cs @@ -837,6 +837,68 @@ public void SendAsync_MultipleCallsWithDispose_NoHandleLeaksManuallyVerifiedUsin } } + [Fact] + public void SendAsync_RequestWithNonAsciiHeaderValue_ThrowsHttpRequestException() + { + using (var handler = new WinHttpHandler()) + { + TestServer.SetResponse(DecompressionMethods.None, TestServer.ExpectedResponseBody); + + var invoker = new HttpMessageInvoker(handler, false); + var request = new HttpRequestMessage(HttpMethod.Get, TestServer.FakeServerEndpoint); + request.Headers.Add("Custom-Header", "HeaderValue\u00A9WithNon-ASCII"); + + var ex = Assert.Throws(() => + { + Task task = invoker.SendAsync(request, CancellationToken.None); + task.GetAwaiter().GetResult(); + }); + + Assert.Contains("ASCII", ex.Message); + } + } + + [Fact] + public void SendAsync_RequestWithAsciiHeaderValue_Succeeds() + { + using (var handler = new WinHttpHandler()) + { + TestServer.SetResponse(DecompressionMethods.None, TestServer.ExpectedResponseBody); + + var invoker = new HttpMessageInvoker(handler, false); + var request = new HttpRequestMessage(HttpMethod.Get, TestServer.FakeServerEndpoint); + request.Headers.Add("Custom-Header", "ValidASCIIHeaderValue123"); + + Task task = invoker.SendAsync(request, CancellationToken.None); + using (HttpResponseMessage response = task.GetAwaiter().GetResult()) + { + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + } + } + + [Fact] + public void SendAsync_RequestWithNonAsciiContentHeader_ThrowsHttpRequestException() + { + using (var handler = new WinHttpHandler()) + { + TestServer.SetResponse(DecompressionMethods.None, TestServer.ExpectedResponseBody); + + var invoker = new HttpMessageInvoker(handler, false); + var request = new HttpRequestMessage(HttpMethod.Post, TestServer.FakeServerEndpoint); + request.Content = new StringContent("test content"); + request.Content.Headers.Add("Custom-Content-Header", "ContentValue\u00A9WithNon-ASCII"); + + var ex = Assert.Throws(() => + { + Task task = invoker.SendAsync(request, CancellationToken.None); + task.GetAwaiter().GetResult(); + }); + + Assert.Contains("ASCII", ex.Message); + } + } + // Commented out as the test relies on finalizer for cleanup and only has value as written // when run on its own and manual analysis is done of logs. //[Fact] From f0f0158e62c85f69f854473ea06b128a5a0fdab7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 3 Jun 2025 14:03:16 +0000 Subject: [PATCH 3/5] Use Ascii.IsValid when targeting .NET for header validation Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com> --- .../src/System/Net/Http/WinHttpHandler.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs index 1e19211f8d8b63..681b6f13bfaa82 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs +++ b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs @@ -711,6 +711,9 @@ private static WinHttpChunkMode GetChunkedModeForSend(HttpRequestMessage request private static bool IsAscii(string value) { +#if NET + return Ascii.IsValid(value); +#else for (int i = 0; i < value.Length; i++) { if (value[i] > 127) @@ -719,6 +722,7 @@ private static bool IsAscii(string value) } } return true; +#endif } private static void ValidateHeadersForAscii(string headers) From 00a1e0422c1a780e7f7464022f2609821b4db6af Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 3 Jun 2025 14:26:26 +0000 Subject: [PATCH 4/5] Move exception message to resource strings (resx) Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com> --- .../System.Net.Http.WinHttpHandler/src/Resources/Strings.resx | 3 +++ .../src/System/Net/Http/WinHttpHandler.cs | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/Resources/Strings.resx b/src/libraries/System.Net.Http.WinHttpHandler/src/Resources/Strings.resx index 82e7445d67e742..57b3f919d40392 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/src/Resources/Strings.resx +++ b/src/libraries/System.Net.Http.WinHttpHandler/src/Resources/Strings.resx @@ -135,4 +135,7 @@ Request version value must be one of 1.0, 1.1, 2.0, or 3.0. + + Request headers must contain only ASCII characters. + diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs index 681b6f13bfaa82..ef8491a6033d36 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs +++ b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs @@ -729,7 +729,7 @@ private static void ValidateHeadersForAscii(string headers) { if (!IsAscii(headers)) { - throw new HttpRequestException("Request headers must contain only ASCII characters."); + throw new HttpRequestException(SR.net_http_headers_must_be_ascii); } } From ab6c173b58c6dcd4ef431068e0bebad2326bf54a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 3 Jun 2025 17:39:11 +0000 Subject: [PATCH 5/5] Update header validation to allow Latin-1 characters while still blocking dangerous controls Co-authored-by: ManickaP <11718369+ManickaP@users.noreply.github.com> --- .../src/Resources/Strings.resx | 3 ++ .../src/System/Net/Http/WinHttpHandler.cs | 37 ++++++++------ .../tests/UnitTests/WinHttpHandlerTest.cs | 51 ++++++++++++++++--- 3 files changed, 69 insertions(+), 22 deletions(-) diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/Resources/Strings.resx b/src/libraries/System.Net.Http.WinHttpHandler/src/Resources/Strings.resx index 57b3f919d40392..5a6dd27e582283 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/src/Resources/Strings.resx +++ b/src/libraries/System.Net.Http.WinHttpHandler/src/Resources/Strings.resx @@ -138,4 +138,7 @@ Request headers must contain only ASCII characters. + + Request headers must not contain CR, LF, or NUL characters. + diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs index ef8491a6033d36..09d04cbe59b70f 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs +++ b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs @@ -709,27 +709,31 @@ private static WinHttpChunkMode GetChunkedModeForSend(HttpRequestMessage request return chunkedMode; } - private static bool IsAscii(string value) + private static bool IsValidHeaderChar(char c) { -#if NET - return Ascii.IsValid(value); -#else - for (int i = 0; i < value.Length; i++) + // Allow Latin-1 characters (0-255) but reject dangerous control characters + return c <= 255 && c != '\0' && c != '\r' && c != '\n'; + } + + private static void ValidateHeaderValues(System.Net.Http.Headers.HttpHeaders headers) + { + foreach (var header in headers) { - if (value[i] > 127) + foreach (var value in header.Value) { - return false; + ValidateHeaderValue(value); } } - return true; -#endif } - private static void ValidateHeadersForAscii(string headers) + private static void ValidateHeaderValue(string value) { - if (!IsAscii(headers)) + for (int i = 0; i < value.Length; i++) { - throw new HttpRequestException(SR.net_http_headers_must_be_ascii); + if (!IsValidHeaderChar(value[i])) + { + throw new HttpRequestException(SR.net_http_headers_invalid_chars); + } } } @@ -758,14 +762,14 @@ private static void AddRequestHeaders( string? cookieHeader = WinHttpCookieContainerAdapter.GetCookieHeader(requestMessage.RequestUri, cookies); if (!string.IsNullOrEmpty(cookieHeader)) { - ValidateHeadersForAscii(cookieHeader); + ValidateHeaderValue(cookieHeader); requestHeadersBuffer.AppendLine(cookieHeader); } } - // Serialize general request headers and validate for ASCII. + // Validate and serialize general request headers + ValidateHeaderValues(requestMessage.Headers); string generalHeaders = requestMessage.Headers.ToString(); - ValidateHeadersForAscii(generalHeaders); requestHeadersBuffer.Append(generalHeaders); // Serialize entity-body (content) headers and validate for ASCII. @@ -781,8 +785,9 @@ private static void AddRequestHeaders( requestMessage.Content.Headers.ContentLength = contentLength; } + // Validate and serialize entity-body (content) headers + ValidateHeaderValues(requestMessage.Content.Headers); string contentHeaders = requestMessage.Content.Headers.ToString(); - ValidateHeadersForAscii(contentHeaders); requestHeadersBuffer.Append(contentHeaders); } diff --git a/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/WinHttpHandlerTest.cs b/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/WinHttpHandlerTest.cs index 8a7490eead6d82..91220735e6bf07 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/WinHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/WinHttpHandlerTest.cs @@ -838,7 +838,7 @@ public void SendAsync_MultipleCallsWithDispose_NoHandleLeaksManuallyVerifiedUsin } [Fact] - public void SendAsync_RequestWithNonAsciiHeaderValue_ThrowsHttpRequestException() + public void SendAsync_RequestWithDangerousControlHeaderValue_ThrowsHttpRequestException() { using (var handler = new WinHttpHandler()) { @@ -846,7 +846,7 @@ public void SendAsync_RequestWithNonAsciiHeaderValue_ThrowsHttpRequestException( var invoker = new HttpMessageInvoker(handler, false); var request = new HttpRequestMessage(HttpMethod.Get, TestServer.FakeServerEndpoint); - request.Headers.Add("Custom-Header", "HeaderValue\u00A9WithNon-ASCII"); + request.Headers.Add("Custom-Header", "HeaderValue\0WithNUL"); var ex = Assert.Throws(() => { @@ -854,7 +854,26 @@ public void SendAsync_RequestWithNonAsciiHeaderValue_ThrowsHttpRequestException( task.GetAwaiter().GetResult(); }); - Assert.Contains("ASCII", ex.Message); + Assert.Contains("CR, LF, or NUL", ex.Message); + } + } + + [Fact] + public void SendAsync_RequestWithLatin1HeaderValue_Succeeds() + { + using (var handler = new WinHttpHandler()) + { + TestServer.SetResponse(DecompressionMethods.None, TestServer.ExpectedResponseBody); + + var invoker = new HttpMessageInvoker(handler, false); + var request = new HttpRequestMessage(HttpMethod.Get, TestServer.FakeServerEndpoint); + request.Headers.Add("Custom-Header", "HeaderValue\u00A9WithLatin1"); + + Task task = invoker.SendAsync(request, CancellationToken.None); + using (HttpResponseMessage response = task.GetAwaiter().GetResult()) + { + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } } } @@ -878,7 +897,7 @@ public void SendAsync_RequestWithAsciiHeaderValue_Succeeds() } [Fact] - public void SendAsync_RequestWithNonAsciiContentHeader_ThrowsHttpRequestException() + public void SendAsync_RequestWithDangerousControlContentHeader_ThrowsHttpRequestException() { using (var handler = new WinHttpHandler()) { @@ -887,7 +906,7 @@ public void SendAsync_RequestWithNonAsciiContentHeader_ThrowsHttpRequestExceptio var invoker = new HttpMessageInvoker(handler, false); var request = new HttpRequestMessage(HttpMethod.Post, TestServer.FakeServerEndpoint); request.Content = new StringContent("test content"); - request.Content.Headers.Add("Custom-Content-Header", "ContentValue\u00A9WithNon-ASCII"); + request.Content.Headers.Add("Custom-Content-Header", "ContentValue\0WithNUL"); var ex = Assert.Throws(() => { @@ -895,7 +914,27 @@ public void SendAsync_RequestWithNonAsciiContentHeader_ThrowsHttpRequestExceptio task.GetAwaiter().GetResult(); }); - Assert.Contains("ASCII", ex.Message); + Assert.Contains("CR, LF, or NUL", ex.Message); + } + } + + [Fact] + public void SendAsync_RequestWithLatin1ContentHeader_Succeeds() + { + using (var handler = new WinHttpHandler()) + { + TestServer.SetResponse(DecompressionMethods.None, TestServer.ExpectedResponseBody); + + var invoker = new HttpMessageInvoker(handler, false); + var request = new HttpRequestMessage(HttpMethod.Post, TestServer.FakeServerEndpoint); + request.Content = new StringContent("test content"); + request.Content.Headers.Add("Custom-Content-Header", "ContentValue\u00A9WithLatin1"); + + Task task = invoker.SendAsync(request, CancellationToken.None); + using (HttpResponseMessage response = task.GetAwaiter().GetResult()) + { + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } } }