From 6728e756b77afb54a29e06cdba3e6f194984ec40 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Fri, 23 Feb 2018 17:24:20 +0000 Subject: [PATCH] Sanitize and centralize exception throws (#2293) * Sanitize and centralize exception throws --- src/Kestrel.Core/BadHttpRequestException.cs | 24 +++++++++++- .../Internal/Http/Http1Connection.cs | 28 +++++++------- .../Internal/Http/Http1MessageBody.cs | 20 +++++----- .../Internal/Http/HttpHeaders.Generated.cs | 2 +- src/Kestrel.Core/Internal/Http/HttpParser.cs | 10 +++-- .../Internal/Http/HttpProtocol.cs | 38 +++++++++++++------ .../Internal/Http/HttpRequestHeaders.cs | 14 +------ .../StackTraceHiddenAttribute.cs | 17 +++++++++ tools/CodeGenerator/KnownHeaders.cs | 2 +- 9 files changed, 101 insertions(+), 54 deletions(-) create mode 100644 src/Kestrel.Core/Internal/Infrastructure/StackTraceHiddenAttribute.cs diff --git a/src/Kestrel.Core/BadHttpRequestException.cs b/src/Kestrel.Core/BadHttpRequestException.cs index e778f76641..1414753d9f 100644 --- a/src/Kestrel.Core/BadHttpRequestException.cs +++ b/src/Kestrel.Core/BadHttpRequestException.cs @@ -1,7 +1,9 @@ // 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.Diagnostics; using System.IO; +using System.Runtime.CompilerServices; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; @@ -30,6 +32,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core internal StringValues AllowedHeader { get; } + [StackTraceHidden] + internal static void Throw(RequestRejectionReason reason) + { + throw GetException(reason); + } + + [MethodImpl(MethodImplOptions.NoInlining)] internal static BadHttpRequestException GetException(RequestRejectionReason reason) { BadHttpRequestException ex; @@ -102,6 +111,19 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core return ex; } + [StackTraceHidden] + internal static void Throw(RequestRejectionReason reason, string detail) + { + throw GetException(reason, detail); + } + + [StackTraceHidden] + internal static void Throw(RequestRejectionReason reason, in StringValues detail) + { + throw GetException(reason, detail.ToString()); + } + + [MethodImpl(MethodImplOptions.NoInlining)] internal static BadHttpRequestException GetException(RequestRejectionReason reason, string detail) { BadHttpRequestException ex; @@ -141,4 +163,4 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core return ex; } } -} +} \ No newline at end of file diff --git a/src/Kestrel.Core/Internal/Http/Http1Connection.cs b/src/Kestrel.Core/Internal/Http/Http1Connection.cs index a66179fa4a..4b24908f53 100644 --- a/src/Kestrel.Core/Internal/Http/Http1Connection.cs +++ b/src/Kestrel.Core/Internal/Http/Http1Connection.cs @@ -119,7 +119,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http var result = _parser.ParseRequestLine(new Http1ParsingHandler(this), buffer, out consumed, out examined); if (!result && overLength) { - ThrowRequestRejected(RequestRejectionReason.RequestLineTooLong); + BadHttpRequestException.Throw(RequestRejectionReason.RequestLineTooLong); } return result; @@ -143,7 +143,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http if (!result && overLength) { - ThrowRequestRejected(RequestRejectionReason.HeadersExceedMaxTotalSize); + BadHttpRequestException.Throw(RequestRejectionReason.HeadersExceedMaxTotalSize); } if (result) { @@ -278,7 +278,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http // requests (https://tools.ietf.org/html/rfc7231#section-4.3.6). if (method != HttpMethod.Connect) { - ThrowRequestRejected(RequestRejectionReason.ConnectMethodRequired); + BadHttpRequestException.Throw(RequestRejectionReason.ConnectMethodRequired); } // When making a CONNECT request to establish a tunnel through one or @@ -303,7 +303,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http // OPTIONS request (https://tools.ietf.org/html/rfc7231#section-4.3.7). if (method != HttpMethod.Options) { - ThrowRequestRejected(RequestRejectionReason.OptionsMethodRequired); + BadHttpRequestException.Throw(RequestRejectionReason.OptionsMethodRequired); } RawTarget = Asterisk; @@ -367,17 +367,17 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http var host = HttpRequestHeaders.HeaderHost; if (host.Count <= 0) { - ThrowRequestRejected(RequestRejectionReason.MissingHostHeader); + BadHttpRequestException.Throw(RequestRejectionReason.MissingHostHeader); } else if (host.Count > 1) { - ThrowRequestRejected(RequestRejectionReason.MultipleHostHeaders); + BadHttpRequestException.Throw(RequestRejectionReason.MultipleHostHeaders); } else if (_requestTargetForm == HttpRequestTarget.AuthorityForm) { if (!host.Equals(RawTarget)) { - ThrowRequestRejected(RequestRejectionReason.InvalidHostHeader, host.ToString()); + BadHttpRequestException.Throw(RequestRejectionReason.InvalidHostHeader, in host); } } else if (_requestTargetForm == HttpRequestTarget.AbsoluteForm) @@ -393,7 +393,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http if ((host != _absoluteRequestTarget.Authority || !_absoluteRequestTarget.IsDefaultPort) && host != authorityAndPort) { - ThrowRequestRejected(RequestRejectionReason.InvalidHostHeader, host.ToString()); + BadHttpRequestException.Throw(RequestRejectionReason.InvalidHostHeader, in host); } } } @@ -446,7 +446,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { if (_requestProcessingStatus == RequestProcessingStatus.ParsingHeaders) { - throw BadHttpRequestException.GetException(RequestRejectionReason + BadHttpRequestException.Throw(RequestRejectionReason .MalformedRequestInvalidHeaders); } throw; @@ -464,11 +464,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http endConnection = true; return true; case RequestProcessingStatus.ParsingRequestLine: - throw BadHttpRequestException.GetException( - RequestRejectionReason.InvalidRequestLine); + BadHttpRequestException.Throw(RequestRejectionReason.InvalidRequestLine); + break; case RequestProcessingStatus.ParsingHeaders: - throw BadHttpRequestException.GetException( - RequestRejectionReason.MalformedRequestInvalidHeaders); + BadHttpRequestException.Throw(RequestRejectionReason.MalformedRequestInvalidHeaders); + break; } } else if (!_keepAlive && _requestProcessingStatus == RequestProcessingStatus.RequestPending) @@ -482,7 +482,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { // In this case, there is an ongoing request but the start line/header parsing has timed out, so send // a 408 response. - throw BadHttpRequestException.GetException(RequestRejectionReason.RequestTimeout); + BadHttpRequestException.Throw(RequestRejectionReason.RequestTimeout); } endConnection = false; diff --git a/src/Kestrel.Core/Internal/Http/Http1MessageBody.cs b/src/Kestrel.Core/Internal/Http/Http1MessageBody.cs index 4e08e419fb..427bbacfcf 100644 --- a/src/Kestrel.Core/Internal/Http/Http1MessageBody.cs +++ b/src/Kestrel.Core/Internal/Http/Http1MessageBody.cs @@ -46,7 +46,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http if (_context.RequestTimedOut) { - _context.ThrowRequestRejected(RequestRejectionReason.RequestTimeout); + BadHttpRequestException.Throw(RequestRejectionReason.RequestTimeout); } var readableBuffer = result.Buffer; @@ -97,7 +97,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http } else if (result.IsCompleted) { - _context.ThrowRequestRejected(RequestRejectionReason.UnexpectedEndOfRequestContent); + BadHttpRequestException.Throw(RequestRejectionReason.UnexpectedEndOfRequestContent); } awaitable = _context.Input.ReadAsync(); @@ -233,7 +233,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { if (headers.HeaderTransferEncoding.Count > 0 || (headers.ContentLength.HasValue && headers.ContentLength.Value != 0)) { - context.ThrowRequestRejected(RequestRejectionReason.UpgradeRequestCannotHavePayload); + BadHttpRequestException.Throw(RequestRejectionReason.UpgradeRequestCannotHavePayload); } return new ForUpgrade(context); @@ -252,7 +252,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http // status code and then close the connection. if (transferCoding != TransferCoding.Chunked) { - context.ThrowRequestRejected(RequestRejectionReason.FinalTransferCodingNotChunked, transferEncoding.ToString()); + BadHttpRequestException.Throw(RequestRejectionReason.FinalTransferCodingNotChunked, in transferEncoding); } return new ForChunkedEncoding(keepAlive, context); @@ -278,7 +278,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http if (HttpMethods.IsPost(context.Method) || HttpMethods.IsPut(context.Method)) { var requestRejectionReason = httpVersion == HttpVersion.Http11 ? RequestRejectionReason.LengthRequired : RequestRejectionReason.LengthRequiredHttp10; - context.ThrowRequestRejected(requestRejectionReason, context.Method); + BadHttpRequestException.Throw(requestRejectionReason, context.Method); } } @@ -339,7 +339,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { if (_contentLength > _context.MaxRequestBodySize) { - _context.ThrowRequestRejected(RequestRejectionReason.RequestBodyTooLarge); + BadHttpRequestException.Throw(RequestRejectionReason.RequestBodyTooLarge); } } } @@ -452,7 +452,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http if (_consumedBytes > _context.MaxRequestBodySize) { - _context.ThrowRequestRejected(RequestRejectionReason.RequestBodyTooLarge); + BadHttpRequestException.Throw(RequestRejectionReason.RequestBodyTooLarge); } } @@ -509,7 +509,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http } // At this point, 10 bytes have been consumed which is enough to parse the max value "7FFFFFFF\r\n". - _context.ThrowRequestRejected(RequestRejectionReason.BadChunkSizeData); + BadHttpRequestException.Throw(RequestRejectionReason.BadChunkSizeData); } private void ParseExtension(ReadOnlyBuffer buffer, out SequencePosition consumed, out SequencePosition examined) @@ -604,7 +604,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http } else { - _context.ThrowRequestRejected(RequestRejectionReason.BadChunkSuffix); + BadHttpRequestException.Throw(RequestRejectionReason.BadChunkSuffix); } } @@ -660,7 +660,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http throw new IOException(CoreStrings.BadRequest_BadChunkSizeData, ex); } - _context.ThrowRequestRejected(RequestRejectionReason.BadChunkSizeData); + BadHttpRequestException.Throw(RequestRejectionReason.BadChunkSizeData); return -1; // can't happen, but compiler complains } diff --git a/src/Kestrel.Core/Internal/Http/HttpHeaders.Generated.cs b/src/Kestrel.Core/Internal/Http/HttpHeaders.Generated.cs index 80bf03c4fd..704d2216a8 100644 --- a/src/Kestrel.Core/Internal/Http/HttpHeaders.Generated.cs +++ b/src/Kestrel.Core/Internal/Http/HttpHeaders.Generated.cs @@ -4049,7 +4049,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { if (_contentLength.HasValue) { - ThrowMultipleContentLengthsException(); + BadHttpRequestException.Throw(RequestRejectionReason.MultipleContentLengths); } else { diff --git a/src/Kestrel.Core/Internal/Http/HttpParser.cs b/src/Kestrel.Core/Internal/Http/HttpParser.cs index 36c4ffecfb..2ca5cdddbf 100644 --- a/src/Kestrel.Core/Internal/Http/HttpParser.cs +++ b/src/Kestrel.Core/Internal/Http/HttpParser.cs @@ -4,6 +4,7 @@ using System; using System.Buffers; using System.Collections; +using System.Diagnostics; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; @@ -254,7 +255,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http } // Headers don't end in CRLF line. - RejectRequest(RequestRejectionReason.InvalidRequestHeadersNoCRLF); + BadHttpRequestException.Throw(RequestRejectionReason.InvalidRequestHeadersNoCRLF); } // We moved the reader so look ahead 2 bytes so reset both the reader @@ -483,18 +484,19 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http c == '~'; } - private void RejectRequest(RequestRejectionReason reason) - => throw BadHttpRequestException.GetException(reason); - + [StackTraceHidden] private unsafe void RejectRequestLine(byte* requestLine, int length) => throw GetInvalidRequestException(RequestRejectionReason.InvalidRequestLine, requestLine, length); + [StackTraceHidden] private unsafe void RejectRequestHeader(byte* headerLine, int length) => throw GetInvalidRequestException(RequestRejectionReason.InvalidRequestHeader, headerLine, length); + [StackTraceHidden] private unsafe void RejectUnknownVersion(byte* version, int length) => throw GetInvalidRequestException(RequestRejectionReason.UnrecognizedHTTPVersion, version, length); + [MethodImpl(MethodImplOptions.NoInlining)] private unsafe BadHttpRequestException GetInvalidRequestException(RequestRejectionReason reason, byte* detail, int length) => BadHttpRequestException.GetException( reason, diff --git a/src/Kestrel.Core/Internal/Http/HttpProtocol.cs b/src/Kestrel.Core/Internal/Http/HttpProtocol.cs index c5cef990ee..80d073f23e 100644 --- a/src/Kestrel.Core/Internal/Http/HttpProtocol.cs +++ b/src/Kestrel.Core/Internal/Http/HttpProtocol.cs @@ -429,7 +429,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http _requestHeadersParsed++; if (_requestHeadersParsed > ServerOptions.Limits.MaxRequestHeaderCount) { - ThrowRequestRejected(RequestRejectionReason.TooManyHeaders); + BadHttpRequestException.Throw(RequestRejectionReason.TooManyHeaders); } var valueString = value.GetAsciiStringNonNullCharacters(); @@ -863,13 +863,26 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http _responseBytesWritten + count > responseHeaders.ContentLength.Value) { _keepAlive = false; - throw new InvalidOperationException( - CoreStrings.FormatTooManyBytesWritten(_responseBytesWritten + count, responseHeaders.ContentLength.Value)); + ThrowTooManyBytesWritten(count); } _responseBytesWritten += count; } + [StackTraceHidden] + private void ThrowTooManyBytesWritten(int count) + { + throw GetTooManyBytesWrittenException(count); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private InvalidOperationException GetTooManyBytesWrittenException(int count) + { + var responseHeaders = HttpResponseHeaders; + return new InvalidOperationException( + CoreStrings.FormatTooManyBytesWritten(_responseBytesWritten + count, responseHeaders.ContentLength.Value)); + } + private void CheckLastWrite() { var responseHeaders = HttpResponseHeaders; @@ -1250,25 +1263,28 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http // Writes to HEAD response are ignored and logged at the end of the request if (Method != "HEAD") { - // Throw Exception for 204, 205, 304 responses. - throw new InvalidOperationException(CoreStrings.FormatWritingToResponseBodyNotSupported(StatusCode)); + ThrowWritingToResponseBodyNotSupported(); } } + [StackTraceHidden] + private void ThrowWritingToResponseBodyNotSupported() + { + // Throw Exception for 204, 205, 304 responses. + throw new InvalidOperationException(CoreStrings.FormatWritingToResponseBodyNotSupported(StatusCode)); + } + + [StackTraceHidden] private void ThrowResponseAbortedException() { throw new ObjectDisposedException(CoreStrings.UnhandledApplicationException, _applicationException); } - public void ThrowRequestRejected(RequestRejectionReason reason) - => throw BadHttpRequestException.GetException(reason); - - public void ThrowRequestRejected(RequestRejectionReason reason, string detail) - => throw BadHttpRequestException.GetException(reason, detail); - + [StackTraceHidden] public void ThrowRequestTargetRejected(Span target) => throw GetInvalidRequestTargetException(target); + [MethodImpl(MethodImplOptions.NoInlining)] private BadHttpRequestException GetInvalidRequestTargetException(Span target) => BadHttpRequestException.GetException( RequestRejectionReason.InvalidRequestTarget, diff --git a/src/Kestrel.Core/Internal/Http/HttpRequestHeaders.cs b/src/Kestrel.Core/Internal/Http/HttpRequestHeaders.cs index f512df8fcf..c441ae5ed6 100644 --- a/src/Kestrel.Core/Internal/Http/HttpRequestHeaders.cs +++ b/src/Kestrel.Core/Internal/Http/HttpRequestHeaders.cs @@ -19,7 +19,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http long parsed; if (!HeaderUtilities.TryParseNonNegativeInt64(value, out parsed)) { - ThrowInvalidContentLengthException(value); + BadHttpRequestException.Throw(RequestRejectionReason.InvalidContentLength, value); } return parsed; @@ -47,7 +47,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { if (!StringUtilities.TryGetAsciiString(pKeyBytes, keyBuffer, keyLength)) { - throw BadHttpRequestException.GetException(RequestRejectionReason.InvalidCharactersInHeaderName); + BadHttpRequestException.Throw(RequestRejectionReason.InvalidCharactersInHeaderName); } } @@ -56,16 +56,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http Unknown[key] = AppendValue(existing, value); } - private static void ThrowInvalidContentLengthException(string value) - { - throw BadHttpRequestException.GetException(RequestRejectionReason.InvalidContentLength, value); - } - - private static void ThrowMultipleContentLengthsException() - { - throw BadHttpRequestException.GetException(RequestRejectionReason.MultipleContentLengths); - } - public Enumerator GetEnumerator() { return new Enumerator(this); diff --git a/src/Kestrel.Core/Internal/Infrastructure/StackTraceHiddenAttribute.cs b/src/Kestrel.Core/Internal/Infrastructure/StackTraceHiddenAttribute.cs new file mode 100644 index 0000000000..cf6e608d7b --- /dev/null +++ b/src/Kestrel.Core/Internal/Infrastructure/StackTraceHiddenAttribute.cs @@ -0,0 +1,17 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +namespace System.Diagnostics +{ + /// + /// Attribute to add to non-returning throw only methods, + /// to restore the stack trace back to what it would be if the throw was in-place + /// + [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Struct, Inherited = false)] + internal sealed class StackTraceHiddenAttribute : Attribute + { + // https://github.com/dotnet/coreclr/blob/eb54e48b13fdfb7233b7bcd32b93792ba3e89f0c/src/mscorlib/shared/System/Diagnostics/StackTraceHiddenAttribute.cs + public StackTraceHiddenAttribute() { } + } +} \ No newline at end of file diff --git a/tools/CodeGenerator/KnownHeaders.cs b/tools/CodeGenerator/KnownHeaders.cs index 8571143b3f..7578113ba3 100644 --- a/tools/CodeGenerator/KnownHeaders.cs +++ b/tools/CodeGenerator/KnownHeaders.cs @@ -36,7 +36,7 @@ namespace CodeGenerator {{{(header.Identifier == "ContentLength" ? $@" if (_contentLength.HasValue) {{ - ThrowMultipleContentLengthsException(); + BadHttpRequestException.Throw(RequestRejectionReason.MultipleContentLengths); }} else {{