Skip to content

Commit 4aa5460

Browse files
ManickaPManickaP
authored
[HTTP] Stricter header value validation (#116634)
* Validate headers for latin 1 and exclude \0 * Fix and tests * More tests / fixes, WIP * Post rebase fix * Moved and fixed tests, fixed cookies * Fixed windows part * Feedback * Fix tests - do not run on browser * Add cookie specific tests * Assert header value in possitive tests * Expanded and fixed tests on Windows. * Fix tests. * Fix for huffman * Damn this H/2 Huffman non-sense, give me some strings please, not this binary soup
1 parent f5fee8f commit 4aa5460

File tree

23 files changed

+268
-41
lines changed

23 files changed

+268
-41
lines changed

‎src/libraries/Common/tests/System/Net/Http/GenericLoopbackServer.cs

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -199,14 +199,16 @@ public struct HttpHeaderData
199199
public string Value { get; }
200200
public bool HuffmanEncoded { get; }
201201
public byte[] Raw { get; }
202+
public int RawValueStart { get; }
202203
public Encoding ValueEncoding { get; }
203204

204-
public HttpHeaderData(string name, string value, bool huffmanEncoded = false, byte[] raw = null, Encoding valueEncoding = null)
205+
public HttpHeaderData(string name, string value, bool huffmanEncoded = false, byte[] raw = null, int rawValueStart = 0, Encoding valueEncoding = null)
205206
{
206207
Name = name;
207208
Value = value;
208209
HuffmanEncoded = huffmanEncoded;
209210
Raw = raw;
211+
RawValueStart = rawValueStart;
210212
ValueEncoding = valueEncoding;
211213
}
212214

@@ -258,6 +260,24 @@ public static async Task<HttpRequestData> FromHttpRequestMessageAsync(System.Net
258260
return result;
259261
}
260262

263+
public HttpHeaderData[] GetHeaderData(string headerName)
264+
{
265+
return Headers.Where(h => h.Name.Equals(headerName, StringComparison.OrdinalIgnoreCase)).ToArray();
266+
}
267+
268+
public HttpHeaderData GetSingleHeaderData(string headerName)
269+
{
270+
HttpHeaderData[] data = GetHeaderData(headerName);
271+
if (data.Length != 1)
272+
{
273+
throw new Exception(
274+
$"Expected single value for {headerName} header, actual count: {data.Length}{Environment.NewLine}" +
275+
$"{"\t"}{string.Join(Environment.NewLine + "\t", data)}");
276+
}
277+
278+
return data[0];
279+
}
280+
261281
public string[] GetHeaderValues(string headerName)
262282
{
263283
return Headers.Where(h => h.Name.Equals(headerName, StringComparison.OrdinalIgnoreCase))

‎src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -425,7 +425,7 @@ private static (int bytesConsumed, int value) DecodeInteger(ReadOnlySpan<byte> h
425425
return QPackTestDecoder.DecodeInteger(headerBlock, prefixMask);
426426
}
427427

428-
private static (int bytesConsumed, string value) DecodeString(ReadOnlySpan<byte> headerBlock)
428+
private static (int bytesConsumed, string value, bool huffmanEncoded, int valueStart) DecodeString(ReadOnlySpan<byte> headerBlock)
429429
{
430430
(int bytesConsumed, int stringLength) = DecodeInteger(headerBlock, 0b01111111);
431431
if ((headerBlock[0] & 0b10000000) != 0)
@@ -434,12 +434,12 @@ private static (int bytesConsumed, string value) DecodeString(ReadOnlySpan<byte>
434434
byte[] buffer = new byte[stringLength * 2];
435435
int bytesDecoded = HuffmanDecoder.Decode(headerBlock.Slice(bytesConsumed, stringLength), buffer);
436436
string value = Encoding.ASCII.GetString(buffer, 0, bytesDecoded);
437-
return (bytesConsumed + stringLength, value);
437+
return (bytesConsumed + stringLength, value, true, bytesConsumed);
438438
}
439439
else
440440
{
441441
string value = Encoding.ASCII.GetString(headerBlock.Slice(bytesConsumed, stringLength).ToArray());
442-
return (bytesConsumed + stringLength, value);
442+
return (bytesConsumed + stringLength, value, false, bytesConsumed);
443443
}
444444
}
445445

@@ -523,7 +523,7 @@ private static (int bytesConsumed, HttpHeaderData headerData) DecodeLiteralHeade
523523
string name;
524524
if (index == 0)
525525
{
526-
(bytesConsumed, name) = DecodeString(headerBlock.Slice(i));
526+
(bytesConsumed, name, _, _) = DecodeString(headerBlock.Slice(i));
527527
i += bytesConsumed;
528528
}
529529
else
@@ -532,10 +532,11 @@ private static (int bytesConsumed, HttpHeaderData headerData) DecodeLiteralHeade
532532
}
533533

534534
string value;
535-
(bytesConsumed, value) = DecodeString(headerBlock.Slice(i));
535+
(bytesConsumed, value, bool huffmanEncoded, int valueStart) = DecodeString(headerBlock.Slice(i));
536+
valueStart += i;
536537
i += bytesConsumed;
537538

538-
return (i, new HttpHeaderData(name, value));
539+
return (i, new HttpHeaderData(name, value, huffmanEncoded, rawValueStart: valueStart));
539540
}
540541

541542
private static (int bytesConsumed, HttpHeaderData headerData) DecodeHeader(ReadOnlySpan<byte> headerBlock)
@@ -680,7 +681,7 @@ public async IAsyncEnumerable<Frame> ReadRequestHeadersFrames()
680681
(int bytesConsumed, HttpHeaderData headerData) = DecodeHeader(data.Span.Slice(i));
681682

682683
byte[] headerRaw = data.Span.Slice(i, bytesConsumed).ToArray();
683-
headerData = new HttpHeaderData(headerData.Name, headerData.Value, headerData.HuffmanEncoded, headerRaw);
684+
headerData = new HttpHeaderData(headerData.Name, headerData.Value, headerData.HuffmanEncoded, headerRaw, headerData.RawValueStart);
684685

685686
requestData.Headers.Add(headerData);
686687
i += bytesConsumed;

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

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2150,5 +2150,185 @@ public async Task SendAsync_InvalidRequestUri_Throws()
21502150
request = new HttpRequestMessage(HttpMethod.Get, new Uri("foo://foo.bar"));
21512151
await Assert.ThrowsAsync<NotSupportedException>(() => invoker.SendAsync(request, CancellationToken.None));
21522152
}
2153+
2154+
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))]
2155+
[InlineData('\r', HeaderType.Request)]
2156+
[InlineData('\n', HeaderType.Request)]
2157+
[InlineData('\0', HeaderType.Request)]
2158+
[InlineData('\u0100', HeaderType.Request)]
2159+
[InlineData('\u0080', HeaderType.Request)]
2160+
[InlineData('\u009F', HeaderType.Request)]
2161+
[InlineData('\r', HeaderType.Content)]
2162+
[InlineData('\n', HeaderType.Content)]
2163+
[InlineData('\0', HeaderType.Content)]
2164+
[InlineData('\u0100', HeaderType.Content)]
2165+
[InlineData('\u0080', HeaderType.Content)]
2166+
[InlineData('\u009F', HeaderType.Content)]
2167+
[InlineData('\r', HeaderType.Cookie)]
2168+
[InlineData('\n', HeaderType.Cookie)]
2169+
[InlineData('\0', HeaderType.Cookie)]
2170+
[InlineData('\u0100', HeaderType.Cookie)]
2171+
[InlineData('\u0080', HeaderType.Cookie)]
2172+
[InlineData('\u009F', HeaderType.Cookie)]
2173+
public async Task SendAsync_RequestWithDangerousControlHeaderValue_ThrowsHttpRequestException(char dangerousChar, HeaderType headerType)
2174+
{
2175+
string uri = "https://example.com"; // URI doesn't matter, the request should never leave the client
2176+
var handler = CreateHttpClientHandler();
2177+
2178+
var request = new HttpRequestMessage(HttpMethod.Get, uri);
2179+
request.Version = UseVersion;
2180+
try
2181+
{
2182+
switch (headerType)
2183+
{
2184+
case HeaderType.Request:
2185+
request.Headers.Add("Custom-Header", $"HeaderValue{dangerousChar}WithControlChar");
2186+
break;
2187+
case HeaderType.Content:
2188+
request.Content = new StringContent("test content");
2189+
request.Content.Headers.Add("Custom-Content-Header", $"ContentValue{dangerousChar}WithControlChar");
2190+
break;
2191+
case HeaderType.Cookie:
2192+
#if WINHTTPHANDLER_TEST
2193+
handler.CookieUsePolicy = CookieUsePolicy.UseSpecifiedCookieContainer;
2194+
#endif
2195+
handler.CookieContainer = new CookieContainer();
2196+
handler.CookieContainer.Add(new Uri(uri), new Cookie("CustomCookie", $"Value{dangerousChar}WithControlChar"));
2197+
break;
2198+
}
2199+
}
2200+
catch (FormatException fex) when (fex.Message.Contains("New-line or NUL") && dangerousChar != '\u0100')
2201+
{
2202+
return;
2203+
}
2204+
catch (CookieException) when (dangerousChar != '\u0100')
2205+
{
2206+
return;
2207+
}
2208+
2209+
using (var client = new HttpClient(handler))
2210+
{
2211+
var ex = await Assert.ThrowsAnyAsync<Exception>(() => client.SendAsync(request));
2212+
_output.WriteLine(ex.ToString());
2213+
if (IsWinHttpHandler)
2214+
{
2215+
var fex = Assert.IsType<FormatException>(ex);
2216+
Assert.Contains("Latin-1", fex.Message);
2217+
}
2218+
else
2219+
{
2220+
var hrex = Assert.IsType<HttpRequestException>(ex);
2221+
var message = UseVersion == HttpVersion30 ? hrex.InnerException.Message : hrex.Message;
2222+
Assert.Contains("ASCII", message);
2223+
}
2224+
}
2225+
}
2226+
2227+
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))]
2228+
[InlineData('\u0001', HeaderType.Request)]
2229+
[InlineData('\u0007', HeaderType.Request)]
2230+
[InlineData('\u007F', HeaderType.Request)]
2231+
[InlineData('\u00A0', HeaderType.Request)]
2232+
[InlineData('\u00A9', HeaderType.Request)]
2233+
[InlineData('\u00FF', HeaderType.Request)]
2234+
[InlineData('\u0001', HeaderType.Content)]
2235+
[InlineData('\u0007', HeaderType.Content)]
2236+
[InlineData('\u007F', HeaderType.Content)]
2237+
[InlineData('\u00A0', HeaderType.Content)]
2238+
[InlineData('\u00A9', HeaderType.Content)]
2239+
[InlineData('\u00FF', HeaderType.Content)]
2240+
[InlineData('\u0001', HeaderType.Cookie)]
2241+
[InlineData('\u0007', HeaderType.Cookie)]
2242+
[InlineData('\u007F', HeaderType.Cookie)]
2243+
[InlineData('\u00A0', HeaderType.Cookie)]
2244+
[InlineData('\u00A9', HeaderType.Cookie)]
2245+
[InlineData('\u00FF', HeaderType.Cookie)]
2246+
public async Task SendAsync_RequestWithLatin1HeaderValue_Succeeds(char safeChar, HeaderType headerType)
2247+
{
2248+
if (!IsWinHttpHandler && safeChar > 0x7F)
2249+
{
2250+
return; // SocketsHttpHandler doesn't support Latin-1 characters in headers without setting header encoding.
2251+
}
2252+
var headerValue = $"HeaderValue{safeChar}WithSafeChar";
2253+
await LoopbackServerFactory.CreateClientAndServerAsync(async uri =>
2254+
{
2255+
var handler = CreateHttpClientHandler();
2256+
using (var client = new HttpClient(handler))
2257+
{
2258+
var request = new HttpRequestMessage(HttpMethod.Get, uri);
2259+
request.Version = UseVersion;
2260+
switch (headerType)
2261+
{
2262+
case HeaderType.Request:
2263+
request.Headers.Add("Custom-Header", headerValue);
2264+
break;
2265+
case HeaderType.Content:
2266+
request.Content = new StringContent("test content");
2267+
request.Content.Headers.Add("Custom-Content-Header", headerValue);
2268+
break;
2269+
case HeaderType.Cookie:
2270+
#if WINHTTPHANDLER_TEST
2271+
handler.CookieUsePolicy = CookieUsePolicy.UseSpecifiedCookieContainer;
2272+
#endif
2273+
handler.CookieContainer = new CookieContainer();
2274+
handler.CookieContainer.Add(uri, new Cookie("CustomCookie", headerValue));
2275+
break;
2276+
}
2277+
2278+
using (HttpResponseMessage response = await client.SendAsync(request))
2279+
{
2280+
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
2281+
}
2282+
}
2283+
}, async server =>
2284+
{
2285+
var data = await server.AcceptConnectionSendResponseAndCloseAsync();
2286+
switch (headerType)
2287+
{
2288+
case HeaderType.Request:
2289+
{
2290+
var headerLine = DecodeHeaderValue("Custom-Header");
2291+
var receivedHeaderValue = headerLine.Substring(headerLine.IndexOf("HeaderValue"));
2292+
Assert.Equal(headerValue, receivedHeaderValue);
2293+
break;
2294+
}
2295+
case HeaderType.Content:
2296+
{
2297+
var headerLine = DecodeHeaderValue("Custom-Content-Header");
2298+
var receivedHeaderValue = headerLine.Substring(headerLine.IndexOf("HeaderValue"));
2299+
Assert.Equal(headerValue, receivedHeaderValue);
2300+
break;
2301+
}
2302+
case HeaderType.Cookie:
2303+
{
2304+
var headerLine = DecodeHeaderValue("cookie");
2305+
var receivedHeaderValue = headerLine.Substring(headerLine.IndexOf("HeaderValue"));
2306+
Assert.Equal(headerValue, receivedHeaderValue);
2307+
break;
2308+
}
2309+
}
2310+
2311+
string DecodeHeaderValue(string headerName)
2312+
{
2313+
var encoding = Encoding.GetEncoding("ISO-8859-1");
2314+
HttpHeaderData headerData = data.GetSingleHeaderData(headerName);
2315+
ReadOnlySpan<byte> raw = headerData.Raw.AsSpan().Slice(headerData.RawValueStart);
2316+
if (headerData.HuffmanEncoded)
2317+
{
2318+
byte[] buffer = new byte[raw.Length * 2];
2319+
int length = HuffmanDecoder.Decode(raw, buffer);
2320+
raw = buffer.AsSpan().Slice(0, length);
2321+
}
2322+
return encoding.GetString(raw.ToArray());
2323+
}
2324+
});
2325+
}
2326+
2327+
public enum HeaderType
2328+
{
2329+
Request,
2330+
Content,
2331+
Cookie
2332+
}
21532333
}
21542334
}

‎src/libraries/Common/tests/System/Net/Http/LoopbackServer.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -812,7 +812,7 @@ public override async Task<HttpRequestData> ReadRequestDataAsync(bool readBody =
812812
int offset = line.IndexOf(':');
813813
string name = line.Substring(0, offset);
814814
string value = line.Substring(offset + 1).TrimStart();
815-
requestData.Headers.Add(new HttpHeaderData(name, value, raw: lineBytes));
815+
requestData.Headers.Add(new HttpHeaderData(name, value, raw: lineBytes, rawValueStart: offset + 1));
816816
}
817817

818818
if (requestData.Method != "GET")

‎src/libraries/Common/tests/System/Net/Http/QPackTestDecoder.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ public static (int bytesConsumed, HttpHeaderData) DecodeHeader(ReadOnlySpan<byte
4242
(int valueLength, string value) = DecodeString(buffer.Slice(nameLength), 0b0111_1111);
4343

4444
int headerLength = nameLength + valueLength;
45-
var header = new HttpHeaderData(s_staticTable[staticIndex].Name, value, raw: buffer.Slice(0, headerLength).ToArray());
45+
var header = new HttpHeaderData(s_staticTable[staticIndex].Name, value, raw: buffer.Slice(0, headerLength).ToArray(), rawValueStart: nameLength);
4646

4747
return (headerLength, header);
4848
}
@@ -52,7 +52,7 @@ public static (int bytesConsumed, HttpHeaderData) DecodeHeader(ReadOnlySpan<byte
5252
(int valueLength, string value) = DecodeString(buffer.Slice(nameLength), 0b0111_1111);
5353

5454
int headerLength = nameLength + valueLength;
55-
var header = new HttpHeaderData(name, value, raw: buffer.Slice(0, headerLength).ToArray());
55+
var header = new HttpHeaderData(name, value, raw: buffer.Slice(0, headerLength).ToArray(), rawValueStart: nameLength);
5656

5757
return (headerLength, header);
5858
}

‎src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/HttpHeadersFuzzer.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,15 +78,15 @@ static void Test(HttpHeaders headers, string name, string value)
7878
{
7979
foreach (string headerValue in values)
8080
{
81-
Assert.False(headerValue.ContainsAny('\r', '\n'));
81+
Assert.False(headerValue.ContainsAny('\r', '\n', '\0'));
8282
}
8383
}
8484

8585
foreach ((_, IEnumerable<string> values) in headers)
8686
{
8787
foreach (string headerValue in values)
8888
{
89-
Assert.False(headerValue.ContainsAny('\r', '\n'));
89+
Assert.False(headerValue.ContainsAny('\r', '\n', '\0'));
9090
}
9191
}
9292
}

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,4 +135,7 @@
135135
<data name="net_http_unsupported_version" xml:space="preserve">
136136
<value>Request version value must be one of 1.0, 1.1, 2.0, or 3.0.</value>
137137
</data>
138+
<data name="net_http_invalid_header_value" xml:space="preserve">
139+
<value>Request headers must be valid Latin-1 characters.</value>
140+
</data>
138141
</root>

‎src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpCookieContainerAdapter.cs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,15 @@ public static void ResetCookieRequestHeaders(WinHttpRequestState state, Uri redi
8080
(uint)cookieHeader.Length,
8181
Interop.WinHttp.WINHTTP_ADDREQ_FLAG_ADD))
8282
{
83-
WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpAddRequestHeaders));
83+
int lastError = Marshal.GetLastWin32Error();
84+
if (lastError == Interop.WinHttp.ERROR_INVALID_PARAMETER)
85+
{
86+
throw new FormatException(SR.net_http_invalid_header_value);
87+
}
88+
else
89+
{
90+
throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpAddRequestHeaders));
91+
}
8492
}
8593
}
8694
}

‎src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -769,7 +769,15 @@ private static void AddRequestHeaders(
769769
(uint)requestHeadersBuffer.Length,
770770
Interop.WinHttp.WINHTTP_ADDREQ_FLAG_ADD))
771771
{
772-
WinHttpException.ThrowExceptionUsingLastError(nameof(Interop.WinHttp.WinHttpAddRequestHeaders));
772+
int lastError = Marshal.GetLastWin32Error();
773+
if (lastError == Interop.WinHttp.ERROR_INVALID_PARAMETER)
774+
{
775+
throw new FormatException(SR.net_http_invalid_header_value);
776+
}
777+
else
778+
{
779+
throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpAddRequestHeaders));
780+
}
773781
}
774782
}
775783

‎src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/WinHttpHandlerTest.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ public void TcpKeepalive_WhenDisabled_DoesntSetOptions()
121121

122122
SendRequestHelper.Send(
123123
handler,
124-
() => handler.TcpKeepAliveEnabled = false );
124+
() => handler.TcpKeepAliveEnabled = false);
125125
Assert.Null(APICallHistory.WinHttpOptionTcpKeepAlive);
126126
}
127127

0 commit comments

Comments
 (0)