From 35d35f22a36c9ba564889a355f09d95a8e5b78fe Mon Sep 17 00:00:00 2001 From: "Chris Ross (ASP.NET)" Date: Tue, 2 Oct 2018 08:59:20 -0700 Subject: [PATCH] Disallow any frames after a reset is received #2154 --- src/Kestrel.Core/CoreStrings.resx | 3 + .../Internal/Http2/Http2Connection.cs | 33 ++++++- .../Internal/Http2/Http2Stream.cs | 8 ++ .../Properties/CoreStrings.Designer.cs | 14 +++ .../Http2/Http2ConnectionTests.cs | 95 +++++++++++++++++++ .../Http2/H2SpecTests.cs | 2 +- 6 files changed, 153 insertions(+), 2 deletions(-) diff --git a/src/Kestrel.Core/CoreStrings.resx b/src/Kestrel.Core/CoreStrings.resx index 635d4e3e49..b17228eec4 100644 --- a/src/Kestrel.Core/CoreStrings.resx +++ b/src/Kestrel.Core/CoreStrings.resx @@ -587,4 +587,7 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l The client closed the connection. + + A frame of type {frameType} was received after stream {streamId} was reset or aborted. + \ No newline at end of file diff --git a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs index c8d465f9df..87c6c4ed3b 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs @@ -224,6 +224,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 catch (Http2StreamErrorException ex) { Log.Http2StreamError(ConnectionId, ex); + // The client doesn't know this error is coming, allow draining additional frames for now. AbortStream(_incomingFrame.StreamId, new IOException(ex.Message, ex)); await _frameWriter.WriteRstStreamAsync(ex.StreamId, ex.ErrorCode); } @@ -448,6 +449,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 if (_streams.TryGetValue(_incomingFrame.StreamId, out var stream)) { + if (stream.DoNotDrainRequest) + { + // Hard abort, do not allow any more frames on this stream. + throw new Http2ConnectionErrorException(CoreStrings.FormatHttp2ErrorStreamAborted(_incomingFrame.Type, stream.StreamId), Http2ErrorCode.STREAM_CLOSED); + } if (stream.EndStreamReceived) { // http://httpwg.org/specs/rfc7540.html#rfc.section.5.1 @@ -501,6 +507,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 if (_streams.TryGetValue(_incomingFrame.StreamId, out var stream)) { + if (stream.DoNotDrainRequest) + { + // Hard abort, do not allow any more frames on this stream. + throw new Http2ConnectionErrorException(CoreStrings.FormatHttp2ErrorStreamAborted(_incomingFrame.Type, stream.StreamId), Http2ErrorCode.STREAM_CLOSED); + } + // http://httpwg.org/specs/rfc7540.html#rfc.section.5.1 // // ...an endpoint that receives any frames after receiving a frame with the @@ -609,7 +621,20 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 } ThrowIfIncomingFrameSentToIdleStream(); - AbortStream(_incomingFrame.StreamId, new IOException(CoreStrings.Http2StreamResetByClient)); + + if (_streams.TryGetValue(_incomingFrame.StreamId, out var stream)) + { + // Second reset + if (stream.DoNotDrainRequest) + { + // Hard abort, do not allow any more frames on this stream. + throw new Http2ConnectionErrorException(CoreStrings.FormatHttp2ErrorStreamAborted(_incomingFrame.Type, stream.StreamId), Http2ErrorCode.STREAM_CLOSED); + } + + // No additional inbound header or data frames are allowed for this stream after receiving a reset. + stream.DisallowAdditionalRequestFrames(); + stream.Abort(new IOException(CoreStrings.Http2StreamResetByClient)); + } return Task.CompletedTask; } @@ -771,6 +796,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 } else if (_streams.TryGetValue(_incomingFrame.StreamId, out var stream)) { + if (stream.DoNotDrainRequest) + { + // Hard abort, do not allow any more frames on this stream. + throw new Http2ConnectionErrorException(CoreStrings.FormatHttp2ErrorStreamAborted(_incomingFrame.Type, stream.StreamId), Http2ErrorCode.STREAM_CLOSED); + } + if (!stream.TryUpdateOutputWindow(_incomingFrame.WindowUpdateSizeIncrement)) { throw new Http2StreamErrorException(_incomingFrame.StreamId, CoreStrings.Http2ErrorWindowUpdateSizeInvalid, Http2ErrorCode.FLOW_CONTROL_ERROR); diff --git a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs index 2f5566084d..be7c9b9d09 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs @@ -53,6 +53,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 public bool RequestBodyStarted { get; private set; } public bool EndStreamReceived => (_completionState & StreamCompletionFlags.EndStreamReceived) == StreamCompletionFlags.EndStreamReceived; private bool IsAborted => (_completionState & StreamCompletionFlags.Aborted) == StreamCompletionFlags.Aborted; + internal bool DoNotDrainRequest => (_completionState & StreamCompletionFlags.DoNotDrainRequest) == StreamCompletionFlags.DoNotDrainRequest; public override bool IsUpgradableRequest => false; @@ -381,6 +382,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 return _context.FrameWriter.TryUpdateStreamWindow(_outputFlowControl, bytes); } + public void DisallowAdditionalRequestFrames() + { + ApplyCompletionFlag(StreamCompletionFlags.DoNotDrainRequest); + } + public void Abort(IOException abortReason) { var states = ApplyCompletionFlag(StreamCompletionFlags.Aborted); @@ -415,6 +421,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 private void ResetAndAbort(ConnectionAbortedException abortReason, Http2ErrorCode error) { + // Future incoming frames will drain for a default grace period to avoid destabilizing the connection. var states = ApplyCompletionFlag(StreamCompletionFlags.Aborted); if (states.OldState == states.NewState) @@ -507,6 +514,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 RequestProcessingEnded = 1, EndStreamReceived = 2, Aborted = 4, + DoNotDrainRequest = 8, } } } diff --git a/src/Kestrel.Core/Properties/CoreStrings.Designer.cs b/src/Kestrel.Core/Properties/CoreStrings.Designer.cs index d6a4c620d9..7690976935 100644 --- a/src/Kestrel.Core/Properties/CoreStrings.Designer.cs +++ b/src/Kestrel.Core/Properties/CoreStrings.Designer.cs @@ -2198,6 +2198,20 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core internal static string FormatConnectionAbortedByClient() => GetString("ConnectionAbortedByClient"); + /// + /// A frame of type {frameType} was received after stream {streamId} was reset or aborted. + /// + internal static string Http2ErrorStreamAborted + { + get => GetString("Http2ErrorStreamAborted"); + } + + /// + /// A frame of type {frameType} was received after stream {streamId} was reset or aborted. + /// + internal static string FormatHttp2ErrorStreamAborted(object frameType, object streamId) + => string.Format(CultureInfo.CurrentCulture, GetString("Http2ErrorStreamAborted", "frameType", "streamId"), frameType, streamId); + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/test/Kestrel.InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs b/test/Kestrel.InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs index 600def2cd3..bbe3471951 100644 --- a/test/Kestrel.InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs +++ b/test/Kestrel.InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs @@ -2195,6 +2195,101 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests expectedErrorMessage: CoreStrings.FormatHttp2ErrorHeadersInterleaved(Http2FrameType.RST_STREAM, streamId: 1, headersStreamId: 1)); } + // Compare to h2spec http2/5.1/8 + [Fact] + public async Task RST_STREAM_IncompleteRequest_AdditionalDataFrames_ConnectionAborted() + { + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "POST"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + }; + await InitializeConnectionAsync(context => tcs.Task); + + await StartStreamAsync(1, headers, endStream: false); + await SendDataAsync(1, new byte[1], endStream: false); + await SendDataAsync(1, new byte[2], endStream: false); + await SendRstStreamAsync(1); + await SendDataAsync(1, new byte[10], endStream: false); + tcs.TrySetResult(0); + + await WaitForConnectionErrorAsync(ignoreNonGoAwayFrames: false, expectedLastStreamId: 1, + Http2ErrorCode.STREAM_CLOSED, CoreStrings.FormatHttp2ErrorStreamAborted(Http2FrameType.DATA, 1)); + } + + [Fact] + public async Task RST_STREAM_IncompleteRequest_AdditionalTrailerFrames_ConnectionAborted() + { + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "POST"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + }; + await InitializeConnectionAsync(context => tcs.Task); + + await StartStreamAsync(1, headers, endStream: false); + await SendDataAsync(1, new byte[1], endStream: false); + await SendDataAsync(1, new byte[2], endStream: false); + await SendRstStreamAsync(1); + await SendHeadersAsync(1, Http2HeadersFrameFlags.END_HEADERS | Http2HeadersFrameFlags.END_STREAM, _requestTrailers); + tcs.TrySetResult(0); + + await WaitForConnectionErrorAsync(ignoreNonGoAwayFrames: false, expectedLastStreamId: 1, + Http2ErrorCode.STREAM_CLOSED, CoreStrings.FormatHttp2ErrorStreamAborted(Http2FrameType.HEADERS, 1)); + } + + [Fact] + public async Task RST_STREAM_IncompleteRequest_AdditionalResetFrame_ConnectionAborted() + { + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "POST"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + }; + await InitializeConnectionAsync(context => tcs.Task); + + await StartStreamAsync(1, headers, endStream: false); + await SendDataAsync(1, new byte[1], endStream: false); + await SendRstStreamAsync(1); + await SendRstStreamAsync(1); + tcs.TrySetResult(0); + + await WaitForConnectionErrorAsync(ignoreNonGoAwayFrames: false, expectedLastStreamId: 1, + Http2ErrorCode.STREAM_CLOSED, CoreStrings.FormatHttp2ErrorStreamAborted(Http2FrameType.RST_STREAM, 1)); + } + + [Fact] + public async Task RST_STREAM_IncompleteRequest_AdditionalWindowUpdateFrame_ConnectionAborted() + { + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "POST"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + }; + await InitializeConnectionAsync(context => tcs.Task); + + await StartStreamAsync(1, headers, endStream: false); + await SendDataAsync(1, new byte[1], endStream: false); + await SendRstStreamAsync(1); + await SendWindowUpdateAsync(1, 1024); + tcs.TrySetResult(0); + + await WaitForConnectionErrorAsync(ignoreNonGoAwayFrames: false, expectedLastStreamId: 1, + Http2ErrorCode.STREAM_CLOSED, CoreStrings.FormatHttp2ErrorStreamAborted(Http2FrameType.WINDOW_UPDATE, 1)); + } + [Fact] public async Task SETTINGS_KestrelDefaults_Sent() { diff --git a/test/Kestrel.Transport.FunctionalTests/Http2/H2SpecTests.cs b/test/Kestrel.Transport.FunctionalTests/Http2/H2SpecTests.cs index 3bdfbd9929..4d9822cae5 100644 --- a/test/Kestrel.Transport.FunctionalTests/Http2/H2SpecTests.cs +++ b/test/Kestrel.Transport.FunctionalTests/Http2/H2SpecTests.cs @@ -55,7 +55,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests.Http2 get { var dataset = new TheoryData(); - var toSkip = new[] { "http2/5.1/8" }; + var toSkip = new string[] { /*"http2/5.1/8"*/ }; foreach (var testcase in H2SpecCommands.EnumerateTestCases()) {