From 8c8ee150f79d0ed16a14ece21d05e3736dbb83e9 Mon Sep 17 00:00:00 2001 From: Cesar Blum Silveira Date: Tue, 17 May 2016 10:09:55 -0700 Subject: [PATCH] Send 'Connection: close' in all 400 responses to HTTP/1.1 requests (#840). --- .../Http/Frame.cs | 38 ++--- .../BadHttpRequestTests.cs | 134 ++++++++++++------ .../ChunkedRequestTests.cs | 2 + .../EngineTests.cs | 16 +-- 4 files changed, 110 insertions(+), 80 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Http/Frame.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Http/Frame.cs index e0696f82f6..4cb38f317b 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Http/Frame.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Http/Frame.cs @@ -30,7 +30,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http private static readonly byte[] _bytesConnectionClose = Encoding.ASCII.GetBytes("\r\nConnection: close"); private static readonly byte[] _bytesConnectionKeepAlive = Encoding.ASCII.GetBytes("\r\nConnection: keep-alive"); private static readonly byte[] _bytesTransferEncodingChunked = Encoding.ASCII.GetBytes("\r\nTransfer-Encoding: chunked"); - private static readonly byte[] _bytesHttpVersion1_1 = Encoding.ASCII.GetBytes("HTTP/1.1 "); + private static readonly byte[] _bytesHttpVersion11 = Encoding.ASCII.GetBytes("HTTP/1.1 "); private static readonly byte[] _bytesContentLengthZero = Encoding.ASCII.GetBytes("\r\nContent-Length: 0"); private static readonly byte[] _bytesSpace = Encoding.ASCII.GetBytes(" "); private static readonly byte[] _bytesEndHeaders = Encoding.ASCII.GetBytes("\r\n\r\n"); @@ -87,29 +87,30 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http { get { - if (_httpVersion == HttpVersionType.Http1_1) + if (_httpVersion == HttpVersionType.Http11) { return "HTTP/1.1"; } - if (_httpVersion == HttpVersionType.Http1_0) + if (_httpVersion == HttpVersionType.Http10) { return "HTTP/1.0"; } + return string.Empty; } set { if (value == "HTTP/1.1") { - _httpVersion = HttpVersionType.Http1_1; + _httpVersion = HttpVersionType.Http11; } else if (value == "HTTP/1.0") { - _httpVersion = HttpVersionType.Http1_0; + _httpVersion = HttpVersionType.Http10; } else { - _httpVersion = HttpVersionType.Unknown; + _httpVersion = HttpVersionType.Unset; } } } @@ -229,7 +230,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http PathBase = null; Path = null; QueryString = null; - _httpVersion = HttpVersionType.Unknown; + _httpVersion = HttpVersionType.Unset; StatusCode = 200; ReasonPhrase = null; @@ -512,7 +513,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http } StringValues expect; - if (_httpVersion == HttpVersionType.Http1_1 && + if (_httpVersion == HttpVersionType.Http11 && RequestHeaders.TryGetValue("Expect", out expect) && (expect.FirstOrDefault() ?? "").Equals("100-continue", StringComparison.OrdinalIgnoreCase)) { @@ -595,6 +596,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http { // 400 Bad Request StatusCode = 400; + _keepAlive = false; } else { @@ -712,7 +714,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http // // A server MUST NOT send a response containing Transfer-Encoding unless the corresponding // request indicates HTTP/1.1 (or later). - if (_httpVersion == HttpVersionType.Http1_1) + if (_httpVersion == HttpVersionType.Http11) { _autoChunk = true; responseHeaders.SetRawTransferEncoding("chunked", _bytesTransferEncodingChunked); @@ -724,16 +726,16 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http } } - if (_keepAlive == false && responseHeaders.HasConnection == false && _httpVersion == HttpVersionType.Http1_1) + if (!_keepAlive && !responseHeaders.HasConnection && _httpVersion != HttpVersionType.Http10) { responseHeaders.SetRawConnection("close", _bytesConnectionClose); } - else if (_keepAlive && responseHeaders.HasConnection == false && _httpVersion == HttpVersionType.Http1_0) + else if (_keepAlive && !responseHeaders.HasConnection && _httpVersion == HttpVersionType.Http10) { responseHeaders.SetRawConnection("keep-alive", _bytesConnectionKeepAlive); } - end.CopyFrom(_bytesHttpVersion1_1); + end.CopyFrom(_bytesHttpVersion11); end.CopyFrom(statusBytes); responseHeaders.CopyTo(ref end); end.CopyFrom(_bytesEndHeaders, 0, _bytesEndHeaders.Length); @@ -838,13 +840,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http } else if (httpVersion != "HTTP/1.0" && httpVersion != "HTTP/1.1") { - RejectRequest("Malformed request."); + RejectRequest("Unrecognized HTTP version."); } } - // HttpVersion must be set here to send correct response when request is rejected - HttpVersion = httpVersion; - scan.Take(); var next = scan.Take(); if (next == -1) @@ -879,6 +878,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http Method = method; RequestUri = requestUrlPath; QueryString = queryString; + HttpVersion = httpVersion; bool caseMatches; @@ -1099,9 +1099,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http private enum HttpVersionType { - Unknown = -1, - Http1_0 = 0, - Http1_1 = 1 + Unset = -1, + Http10 = 0, + Http11 = 1 } protected enum RequestLineStatus diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/BadHttpRequestTests.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/BadHttpRequestTests.cs index 512e366688..adc90857db 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/BadHttpRequestTests.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/BadHttpRequestTests.cs @@ -1,7 +1,6 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System; using System.Threading.Tasks; using Xunit; @@ -9,64 +8,107 @@ namespace Microsoft.AspNetCore.Server.KestrelTests { public class BadHttpRequestTests { - [Theory(Skip = "This test fails intermittently and needs to be investigated.")] - [InlineData("/ HTTP/1.1\r\n\r\n")] - [InlineData(" / HTTP/1.1\r\n\r\n")] - [InlineData(" / HTTP/1.1\r\n\r\n")] - [InlineData("GET / HTTP/1.1\r\n\r\n")] - [InlineData("GET / HTTP/1.1\r\n\r\n")] - [InlineData("GET HTTP/1.1\r\n\r\n")] + // Don't send more data than necessary to fail, otherwise the test throws trying to + // send data after the server has already closed the connection. This would cause the + // test to fail on Windows, due to a winsock limitation: after the error when trying + // to write to the socket closed by the server, winsock disposes all resources used + // by that socket. The test then fails when we try to read the expected response + // from the server because, although it would have been buffered, it got discarded + // by winsock on the send() error. + // The solution for this is for the client to always try to receive before doing + // any sends, that way it can detect that the connection has been closed by the server + // and not try to send() on the closed connection, triggering the error that would cause + // any buffered received data to be lost. + // We do not deem necessary to mitigate this issue in TestConnection, since it would only + // be ensuring that we have a properly implemented HTTP client that can handle the + // winsock issue. There is nothing to be verified in Kestrel in this situation. + [Theory] + // Incomplete request lines + [InlineData("G")] + [InlineData("GE")] + [InlineData("GET")] + [InlineData("GET ")] [InlineData("GET /")] [InlineData("GET / ")] [InlineData("GET / H")] + [InlineData("GET / HT")] + [InlineData("GET / HTT")] + [InlineData("GET / HTTP")] + [InlineData("GET / HTTP/")] + [InlineData("GET / HTTP/1")] [InlineData("GET / HTTP/1.")] + [InlineData("GET / HTTP/1.1")] + [InlineData("GET / HTTP/1.1\r")] + [InlineData("GET / HTTP/1.0")] + [InlineData("GET / HTTP/1.0\r")] + // Missing method + [InlineData(" ")] + // Missing second space + [InlineData("/ HTTP/1.1\r\n\r\n")] [InlineData("GET /\r\n")] - [InlineData("GET / \r\n")] + // Missing target + [InlineData("GET ")] + // Missing version + [InlineData("GET / \r")] + // Missing CR [InlineData("GET / \n")] - [InlineData("GET / http/1.0\r\n\r\n")] - [InlineData("GET / http/1.1\r\n\r\n")] - [InlineData("GET / HTTP/1.1 \r\n\r\n")] - [InlineData("GET / HTTP/1.1a\r\n\r\n")] - [InlineData("GET / HTTP/1.0\n\r\n")] - [InlineData("GET / HTTP/3.0\r\n\r\n")] - [InlineData("GET / H\r\n\r\n")] - [InlineData("GET / HTTP/1.\r\n\r\n")] - [InlineData("GET / hello\r\n\r\n")] - [InlineData("GET / 8charact\r\n\r\n")] - public async Task TestBadRequests(string request) + // Unrecognized HTTP version + [InlineData("GET / http/1.0\r")] + [InlineData("GET / http/1.1\r")] + [InlineData("GET / HTTP/1.1 \r")] + [InlineData("GET / HTTP/1.1a\r")] + [InlineData("GET / HTTP/1.0\n\r")] + [InlineData("GET / HTTP/1.2\r")] + [InlineData("GET / HTTP/3.0\r")] + [InlineData("GET / H\r")] + [InlineData("GET / HTTP/1.\r")] + [InlineData("GET / hello\r")] + [InlineData("GET / 8charact\r")] + // Missing LF + [InlineData("GET / HTTP/1.0\rA")] + public async Task TestBadRequestLines(string request) { using (var server = new TestServer(context => { return Task.FromResult(0); })) { using (var connection = new TestConnection(server.Port)) { - var receiveTask = Task.Run(async () => - { - await connection.Receive( - "HTTP/1.1 400 Bad Request", - ""); - await connection.ReceiveStartsWith("Date: "); - await connection.ReceiveForcedEnd( - "Content-Length: 0", - "Server: Kestrel", - "", - ""); - }); - - try - { - await connection.SendEnd(request).ConfigureAwait(false); - } - catch (Exception) - { - // TestConnection.SendEnd will start throwing while sending characters - // in cases where the server rejects the request as soon as it - // determines the request line is malformed, even though there - // are more characters following. - } - - await receiveTask; + await connection.SendEnd(request); + await ReceiveBadRequestResponse(connection); } } } + + [Theory] + [InlineData(" ")] + [InlineData("GET ")] + [InlineData("GET / HTTP/1.2\r")] + [InlineData("GET / HTTP/1.0\rA")] + public async Task ServerClosesConnectionAsSoonAsBadRequestLineIsDetected(string request) + { + using (var server = new TestServer(context => { return Task.FromResult(0); })) + { + using (var connection = new TestConnection(server.Port)) + { + await connection.Send(request); + await ReceiveBadRequestResponse(connection); + } + } + } + + private async Task ReceiveBadRequestResponse(TestConnection connection) + { + await connection.Receive( + "HTTP/1.1 400 Bad Request", + ""); + await connection.Receive( + "Connection: close", + ""); + await connection.ReceiveStartsWith("Date: "); + await connection.ReceiveEnd( + "Content-Length: 0", + "Server: Kestrel", + "", + ""); + } } } \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/ChunkedRequestTests.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/ChunkedRequestTests.cs index 7acafa9883..217af83a6b 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/ChunkedRequestTests.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/ChunkedRequestTests.cs @@ -371,6 +371,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests await connection.Receive( "HTTP/1.1 400 Bad Request", + "Connection: close", ""); await connection.ReceiveStartsWith("Date:"); await connection.ReceiveForcedEnd( @@ -415,6 +416,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests await connection.Receive( "HTTP/1.1 400 Bad Request", + "Connection: close", ""); await connection.ReceiveStartsWith("Date:"); await connection.ReceiveForcedEnd( diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/EngineTests.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/EngineTests.cs index 3432a75de3..270a93d998 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/EngineTests.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/EngineTests.cs @@ -724,21 +724,6 @@ namespace Microsoft.AspNetCore.Server.KestrelTests { using (var server = new TestServer(AppChunked, testContext)) { - using (var connection = new TestConnection(server.Port)) - { - await connection.SendEnd( - "GET /"); - await connection.Receive( - "HTTP/1.1 400 Bad Request", - ""); - await connection.ReceiveStartsWith("Date:"); - await connection.ReceiveForcedEnd( - "Content-Length: 0", - "Server: Kestrel", - "", - ""); - } - using (var connection = new TestConnection(server.Port)) { await connection.SendEnd( @@ -750,6 +735,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests "Content-Length: 0", "", "HTTP/1.1 400 Bad Request", + "Connection: close", ""); await connection.ReceiveStartsWith("Date:"); await connection.ReceiveForcedEnd(