Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -135,4 +135,7 @@
<data name="net_http_unsupported_version" xml:space="preserve">
<value>Request version value must be one of 1.0, 1.1, 2.0, or 3.0.</value>
</data>
<data name="net_http_invalid_header_value" xml:space="preserve">
<value>{0} headers must be valid Latin-1 characters.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,20 @@ private static WinHttpChunkMode GetChunkedModeForSend(HttpRequestMessage request
return chunkedMode;
}

private static bool ContainsOnlyValidLatin1(string headerString)
{
foreach (char c in headerString)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would vectorization help here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has to build on .NET Framework as well. So I opted for shared, simple implementation, rather than trying to micro-optimize for a subset of targets.

{
// See https://www.rfc-editor.org/rfc/rfc9110.html#section-5.5-5:
// "Field values containing CR, LF, or NUL characters are invalid and dangerous"
if (c <= 0 || c > 255 || c == '\r' || c == '\n')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (c <= 0 || c > 255 || c == '\r' || c == '\n')
if (c is <= 0 or > 255 or '\r' or '\n')

In the future Roslyn may be able to optimize this with pattern matching.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to filter for CRLF here, we'd have to do it on a per-header basis.
The HttpHeadeers.ToString() will always fail this check.

Including new lines is also a valid scenario for TryAddWithoutValidation if someone wants to use the (obsolete) line folding feature (header value split across multiple lines as long as new lines are followed by a space or tab).
I don't know if WinHttp supports sending those, but if it does I think we should let those through.

My preference would be to rely on the header collection to do the relevant checks here on modern .NET, and you only check that values are <= 255.
For framework you can't know whether a value was added with validation or not, so we might have to fall back to checking line folds manually if we want to filter out new lines.

{
return false;
}
}
return true;
}

private static void AddRequestHeaders(
SafeWinHttpHandle requestHandle,
HttpRequestMessage requestMessage,
Expand Down Expand Up @@ -739,7 +753,12 @@ private static void AddRequestHeaders(
}

// Serialize general request headers.
requestHeadersBuffer.AppendLine(requestMessage.Headers.ToString());
string requestHeaders = requestMessage.Headers.ToString();
if (!ContainsOnlyValidLatin1(requestHeaders))
{
throw new FormatException(SR.Format(SR.net_http_invalid_header_value, nameof(HttpRequestMessage)));
}
requestHeadersBuffer.AppendLine(requestHeaders);

// Serialize entity-body (content) headers.
if (requestMessage.Content != null)
Expand All @@ -754,7 +773,12 @@ private static void AddRequestHeaders(
requestMessage.Content.Headers.ContentLength = contentLength;
}

requestHeadersBuffer.AppendLine(requestMessage.Content.Headers.ToString());
string contentHeaders = requestMessage.Content.Headers.ToString();
if (!ContainsOnlyValidLatin1(contentHeaders))
{
throw new FormatException(SR.Format(SR.net_http_invalid_header_value, nameof(HttpContent)));
}
requestHeadersBuffer.AppendLine(contentHeaders);
}

// Add request headers to WinHTTP request handle.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public void TcpKeepalive_WhenDisabled_DoesntSetOptions()

SendRequestHelper.Send(
handler,
() => handler.TcpKeepAliveEnabled = false );
() => handler.TcpKeepAliveEnabled = false);
Assert.Null(APICallHistory.WinHttpOptionTcpKeepAlive);
}

Expand Down Expand Up @@ -837,6 +837,94 @@ public void SendAsync_MultipleCallsWithDispose_NoHandleLeaksManuallyVerifiedUsin
}
}

[Theory]
[InlineData('\r', HeaderType.Request)]
[InlineData('\n', HeaderType.Request)]
[InlineData('\0', HeaderType.Request)]
[InlineData('\r', HeaderType.Content)]
[InlineData('\n', HeaderType.Content)]
[InlineData('\0', HeaderType.Content)]
[InlineData('\r', HeaderType.Cookie)]
[InlineData('\n', HeaderType.Cookie)]
[InlineData('\0', HeaderType.Cookie)]
public async Task SendAsync_RequestWithDangerousControlHeaderValue_ThrowsHttpRequestException(char dangerousChar, HeaderType headerType)
{
var handler = new WinHttpHandler();
using (var client = new HttpClient(handler))
{
TestServer.SetResponse(DecompressionMethods.None, TestServer.ExpectedResponseBody);

var request = new HttpRequestMessage(HttpMethod.Get, TestServer.FakeServerEndpoint);
switch (headerType)
{
case HeaderType.Request:
request.Headers.Add("Custom-Header", $"HeaderValue{dangerousChar}WithControlChar");
break;
case HeaderType.Content:
request.Content = new StringContent("test content");
request.Content.Headers.Add("Custom-Content-Header", $"ContentValue{dangerousChar}WithControlChar");
break;
case HeaderType.Cookie:
handler.CookieUsePolicy = CookieUsePolicy.UseSpecifiedCookieContainer;
handler.CookieContainer = new CookieContainer();
handler.CookieContainer.Add(new Uri(TestServer.FakeServerEndpoint), new Cookie("CustomCookie", $"Value{dangerousChar}WithControlChar"));
break;
}

var ex = await Assert.ThrowsAsync<HttpRequestException>(() => client.SendAsync(request));
var fex = Assert.IsType<FormatException>(ex.InnerException);
Assert.Contains("Latin-1", fex.Message);
}
}

[Theory]
[InlineData('\u00A9', HeaderType.Request)]
[InlineData('\u00FF', HeaderType.Request)]
[InlineData('\u0001', HeaderType.Request)]
[InlineData('\u00A9', HeaderType.Content)]
[InlineData('\u00FF', HeaderType.Content)]
[InlineData('\u0001', HeaderType.Content)]
[InlineData('\u00A9', HeaderType.Cookie)]
[InlineData('\u00FF', HeaderType.Cookie)]
[InlineData('\u0001', HeaderType.Cookie)]
public async Task SendAsync_RequestWithLatin1HeaderValue_Succeeds(char safeChar, HeaderType headerType)
{
var handler = new WinHttpHandler();
using (var client = new HttpClient(handler))
{
TestServer.SetResponse(DecompressionMethods.None, TestServer.ExpectedResponseBody);

var request = new HttpRequestMessage(HttpMethod.Get, TestServer.FakeServerEndpoint);
switch (headerType)
{
case HeaderType.Request:
request.Headers.Add("Custom-Header", $"HeaderValue{safeChar}WithSafeChar");
break;
case HeaderType.Content:
request.Content = new StringContent("test content");
request.Content.Headers.Add("Custom-Content-Header", $"ContentValue{safeChar}WithSafeChar");
break;
case HeaderType.Cookie:
handler.CookieUsePolicy = CookieUsePolicy.UseSpecifiedCookieContainer;
handler.CookieContainer = new CookieContainer();
handler.CookieContainer.Add(new Uri(TestServer.FakeServerEndpoint), new Cookie("CustomCookie", $"Value{safeChar}WithSafeChar"));
break;
}

using (HttpResponseMessage response = await client.SendAsync(request))
{
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
}
}
}

public enum HeaderType
{
Request,
Content,
Cookie
}

// 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]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ internal static int GetAuthenticationLength(string? input, int startIndex, out o

parsedValue = null;

if (string.IsNullOrEmpty(input) || (startIndex >= input.Length) || HttpRuleParser.ContainsNewLine(input, startIndex))
if (string.IsNullOrEmpty(input) || (startIndex >= input.Length) || HttpRuleParser.ContainsNewLineOrNull(input, startIndex))
{
return 0;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ private static int ParseMultipleEntityTags(string value, int startIndex, out obj
/// </summary>
private static int ParseWithoutValidation(string value, int startIndex, out object? parsedValue)
{
if (HttpRuleParser.ContainsNewLine(value, startIndex))
if (HttpRuleParser.ContainsNewLineOrNull(value, startIndex))
{
parsedValue = null;
return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,7 @@ private static void ParseSingleRawHeaderValue(HeaderStoreItemInfo info, HeaderDe
Debug.Assert(Monitor.IsEntered(info));
if (descriptor.Parser == null)
{
if (HttpRuleParser.ContainsNewLine(rawValue))
if (HttpRuleParser.ContainsNewLineOrNull(rawValue))
{
if (NetEventSource.Log.IsEnabled()) NetEventSource.Error(null, SR.Format(SR.net_http_log_headers_no_newlines, descriptor.Name, rawValue));
AddInvalidValue(info, rawValue);
Expand Down Expand Up @@ -1024,7 +1024,7 @@ private static void ParseAndAddValue(HeaderDescriptor descriptor, HeaderStoreIte
if (descriptor.Parser == null)
{
// If we don't have a parser for the header, we consider the value valid if it doesn't contains
// newline characters. We add the values as "parsed value". Note that we allow empty values.
// newline or \0 characters. We add the values as "parsed value". Note that we allow empty values.
CheckContainsNewLine(value);
AddParsedValue(info, value ?? string.Empty);
return;
Expand Down Expand Up @@ -1134,7 +1134,7 @@ internal static void CheckContainsNewLine(string? value)
return;
}

if (HttpRuleParser.ContainsNewLine(value))
if (HttpRuleParser.ContainsNewLineOrNull(value))
{
throw new FormatException(SR.net_http_headers_no_newlines);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ private static void CheckValueFormat(string? value)
ThrowFormatException(value);
}
}
else if (HttpRuleParser.ContainsNewLine(value))
else if (HttpRuleParser.ContainsNewLineOrNull(value))
{
ThrowFormatException(value);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public string? ReasonPhrase
}
set
{
if ((value != null) && HttpRuleParser.ContainsNewLine(value))
if ((value != null) && HttpRuleParser.ContainsNewLineOrNull(value))
{
throw new FormatException(SR.net_http_reasonphrase_format_error);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,10 @@ internal static int GetWhitespaceLength(string input, int startIndex)
return input.Length - startIndex;
}

internal static bool ContainsNewLine(string value, int startIndex = 0) =>
value.AsSpan(startIndex).ContainsAny('\r', '\n');
// See https://www.rfc-editor.org/rfc/rfc9110.html#section-5.5-5:
// "Field values containing CR, LF, or NUL characters are invalid and dangerous"
internal static bool ContainsNewLineOrNull(string value, int startIndex = 0) =>
value.AsSpan(startIndex).ContainsAny('\r', '\n', '\0');

internal static int GetNumberLength(string input, int startIndex, bool allowDecimal)
{
Expand Down
Loading