[H/3] Fix code passed into QuicConnection.CloseAsync and QuicStream.Abort (#55282)

ManickaP · amcasey · web-flow · commit b37eafded11e · 2024-05-03T00:06:45.000Z
* Fix code passed into QuicConnection.CloseAsync and QuicStream.Abort

* Validate that configured error code is used

---------

Co-authored-by: Andrew Casey &lt;andrew.casey@microsoft.com&gt;
diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs
@@ -166,6 +166,9 @@ private void AbortCore(Exception exception, Http3ErrorCode errorCode)
                 abortReason = new ConnectionAbortedException(exception.Message, exception);
             }
 
+            // This has the side-effect of validating the error code, so do it before we consume the error code
+            _errorCodeFeature.Error = (long)errorCode;
+
             _context.WebTransportSession?.Abort(abortReason, errorCode);
 
             Log.Http3StreamAbort(TraceIdentifier, errorCode, abortReason);
@@ -181,7 +184,6 @@ private void AbortCore(Exception exception, Http3ErrorCode errorCode)
             RequestBodyPipe.Writer.Complete(exception);
 
             // Abort framewriter and underlying transport after stopping output.
-            _errorCodeFeature.Error = (long)errorCode;
             _frameWriter.Abort(abortReason);
         }
     }
diff --git a/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionContext.FeatureCollection.cs b/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionContext.FeatureCollection.cs
@@ -16,7 +16,11 @@ internal sealed partial class QuicConnectionContext : IProtocolErrorCodeFeature,
     public long Error
     {
         get => _error ?? -1;
-        set => _error = value;
+        set
+        {
+            QuicTransportOptions.ValidateErrorCode(value);
+            _error = value;
+        }
     }
 
     public X509Certificate2? ClientCertificate
diff --git a/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionContext.cs b/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionContext.cs
@@ -56,6 +56,7 @@ public override async ValueTask DisposeAsync()
         {
             lock (_shutdownLock)
             {
+                // The DefaultCloseErrorCode setter validates that the error code is within the valid range
                 _closeTask ??= _connection.CloseAsync(errorCode: _context.Options.DefaultCloseErrorCode).AsTask();
             }
 
@@ -81,7 +82,7 @@ public override void Abort(ConnectionAbortedException abortReason)
                 return;
             }
 
-            var resolvedErrorCode = _error ?? 0;
+            var resolvedErrorCode = _error ?? 0; // Only valid error codes are assigned to _error
             _abortReason = ExceptionDispatchInfo.Capture(abortReason);
             QuicLog.ConnectionAbort(_log, this, resolvedErrorCode, abortReason.Message);
             _closeTask = _connection.CloseAsync(errorCode: resolvedErrorCode).AsTask();
@@ -130,7 +131,7 @@ public override void Abort(ConnectionAbortedException abortReason)
         catch (QuicException ex) when (ex.QuicError == QuicError.ConnectionAborted)
         {
             // Shutdown initiated by peer, abortive.
-            _error = ex.ApplicationErrorCode;
+            _error = ex.ApplicationErrorCode; // Trust Quic to provide us a valid error code
             QuicLog.ConnectionAborted(_log, this, ex.ApplicationErrorCode.GetValueOrDefault(), ex);
 
             ThreadPool.UnsafeQueueUserWorkItem(state =>
diff --git a/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.FeatureCollection.cs b/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.FeatureCollection.cs
@@ -38,7 +38,11 @@ public OnCloseRegistration(Action<object?> callback, object? state)
     public long Error
     {
         get => _error ?? -1;
-        set => _error = value;
+        set
+        {
+            QuicTransportOptions.ValidateErrorCode(value);
+            _error = value;
+        }
     }
 
     public long StreamId { get; private set; }
@@ -54,6 +58,8 @@ public long Error
 
     public void AbortRead(long errorCode, ConnectionAbortedException abortReason)
     {
+        QuicTransportOptions.ValidateErrorCode(errorCode);
+
         lock (_shutdownLock)
         {
             if (_stream != null)
@@ -74,6 +80,8 @@ public void AbortRead(long errorCode, ConnectionAbortedException abortReason)
 
     public void AbortWrite(long errorCode, ConnectionAbortedException abortReason)
     {
+        QuicTransportOptions.ValidateErrorCode(errorCode);
+
         lock (_shutdownLock)
         {
             if (_stream != null)
diff --git a/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs b/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs
@@ -273,7 +273,7 @@ private async ValueTask DoReceiveAsync()
         catch (QuicException ex) when (ex.QuicError is QuicError.StreamAborted or QuicError.ConnectionAborted)
         {
             // Abort from peer.
-            _error = ex.ApplicationErrorCode;
+            _error = ex.ApplicationErrorCode; // Trust Quic to provide us a valid error code
             QuicLog.StreamAbortedRead(_log, this, ex.ApplicationErrorCode.GetValueOrDefault());
 
             // This could be ignored if _shutdownReason is already set.
@@ -434,7 +434,7 @@ private async ValueTask DoSendAsync()
         catch (QuicException ex) when (ex.QuicError is QuicError.StreamAborted or QuicError.ConnectionAborted)
         {
             // Abort from peer.
-            _error = ex.ApplicationErrorCode;
+            _error = ex.ApplicationErrorCode; // Trust Quic to provide us a valid error code
             QuicLog.StreamAbortedWrite(_log, this, ex.ApplicationErrorCode.GetValueOrDefault());
 
             // This could be ignored if _shutdownReason is already set.
@@ -501,7 +501,7 @@ public override void Abort(ConnectionAbortedException abortReason)
             _shutdownReason = abortReason;
         }
 
-        var resolvedErrorCode = _error ?? 0;
+        var resolvedErrorCode = _error ?? 0; // _error is validated on assignment
         QuicLog.StreamAbort(_log, this, resolvedErrorCode, abortReason.Message);
 
         if (stream.CanRead)
diff --git a/src/Servers/Kestrel/Transport.Quic/src/QuicTransportOptions.cs b/src/Servers/Kestrel/Transport.Quic/src/QuicTransportOptions.cs
@@ -68,14 +68,15 @@ public long DefaultCloseErrorCode
         }
     }
 
-    private static void ValidateErrorCode(long errorCode)
+    internal static void ValidateErrorCode(long errorCode)
     {
         const long MinErrorCode = 0;
         const long MaxErrorCode = (1L << 62) - 1;
 
         if (errorCode < MinErrorCode || errorCode > MaxErrorCode)
         {
-            throw new ArgumentOutOfRangeException(nameof(errorCode), errorCode, $"A value between {MinErrorCode} and {MaxErrorCode} is required.");
+            // Print the values in hex since the max is unintelligible in decimal
+            throw new ArgumentOutOfRangeException(nameof(errorCode), errorCode, $"A value between 0x{MinErrorCode:x} and 0x{MaxErrorCode:x} is required.");
         }
     }
 
diff --git a/src/Servers/Kestrel/Transport.Quic/test/QuicConnectionContextTests.cs b/src/Servers/Kestrel/Transport.Quic/test/QuicConnectionContextTests.cs
@@ -706,6 +706,33 @@ public async Task PersistentState_StreamsReused_StatePersisted()
         Assert.Equal(true, state);
     }
 
+    [ConditionalTheory]
+    [MsQuicSupported]
+    [InlineData(-1L)] // Too small
+    [InlineData(1L << 62)] // Too big
+    public async Task IProtocolErrorFeature_InvalidErrorCode(long errorCode)
+    {
+        // Arrange
+        await using var connectionListener = await QuicTestHelpers.CreateConnectionListenerFactory(LoggerFactory);
+
+        var options = QuicTestHelpers.CreateClientConnectionOptions(connectionListener.EndPoint);
+        await using var clientConnection = await QuicConnection.ConnectAsync(options);
+
+        await using var serverConnection = await connectionListener.AcceptAndAddFeatureAsync().DefaultTimeout();
+
+        // Act
+        var clientStream = await clientConnection.OpenOutboundStreamAsync(QuicStreamType.Bidirectional);
+        await clientStream.WriteAsync(TestData).DefaultTimeout();
+
+        var serverStream = await serverConnection.AcceptAsync().DefaultTimeout();
+
+        var protocolErrorCodeFeature = serverConnection.Features.Get<IProtocolErrorCodeFeature>();
+
+        // Assert
+        Assert.IsType<QuicConnectionContext>(protocolErrorCodeFeature);
+        Assert.Throws<ArgumentOutOfRangeException>(() => protocolErrorCodeFeature.Error = errorCode);
+    }
+
     private record RequestState(
         QuicConnection QuicConnection,
         MultiplexedConnectionContext ServerConnection,
diff --git a/src/Servers/Kestrel/Transport.Quic/test/QuicStreamContextTests.cs b/src/Servers/Kestrel/Transport.Quic/test/QuicStreamContextTests.cs
@@ -526,4 +526,59 @@ public async Task StreamAbortFeature_AbortWrite_ClientReceivesAbort()
         var serverEx = await Assert.ThrowsAsync<ConnectionAbortedException>(() => serverReadTask).DefaultTimeout();
         Assert.Equal("Test reason", serverEx.Message);
     }
+
+    [ConditionalTheory]
+    [MsQuicSupported]
+    [InlineData(-1L)] // Too small
+    [InlineData(1L << 62)] // Too big
+    public async Task IProtocolErrorFeature_InvalidErrorCode(long errorCode)
+    {
+        // Arrange
+        await using var connectionListener = await QuicTestHelpers.CreateConnectionListenerFactory(LoggerFactory);
+
+        var options = QuicTestHelpers.CreateClientConnectionOptions(connectionListener.EndPoint);
+        await using var clientConnection = await QuicConnection.ConnectAsync(options);
+
+        await using var serverConnection = await connectionListener.AcceptAndAddFeatureAsync().DefaultTimeout();
+
+        // Act
+        var clientStream = await clientConnection.OpenOutboundStreamAsync(QuicStreamType.Bidirectional);
+        await clientStream.WriteAsync(TestData).DefaultTimeout();
+
+        var serverStream = await serverConnection.AcceptAsync().DefaultTimeout();
+
+        var protocolErrorCodeFeature = serverStream.Features.Get<IProtocolErrorCodeFeature>();
+
+        // Assert
+        Assert.IsType<QuicStreamContext>(protocolErrorCodeFeature);
+        Assert.Throws<ArgumentOutOfRangeException>(() => protocolErrorCodeFeature.Error = errorCode);
+    }
+
+    [ConditionalTheory]
+    [MsQuicSupported]
+    [InlineData(-1L)] // Too small
+    [InlineData(1L << 62)] // Too big
+    public async Task IStreamAbortFeature_InvalidErrorCode(long errorCode)
+    {
+        // Arrange
+        await using var connectionListener = await QuicTestHelpers.CreateConnectionListenerFactory(LoggerFactory);
+
+        var options = QuicTestHelpers.CreateClientConnectionOptions(connectionListener.EndPoint);
+        await using var clientConnection = await QuicConnection.ConnectAsync(options);
+
+        await using var serverConnection = await connectionListener.AcceptAndAddFeatureAsync().DefaultTimeout();
+
+        // Act
+        var clientStream = await clientConnection.OpenOutboundStreamAsync(QuicStreamType.Bidirectional);
+        await clientStream.WriteAsync(TestData).DefaultTimeout();
+
+        var serverStream = await serverConnection.AcceptAsync().DefaultTimeout();
+
+        var protocolErrorCodeFeature = serverStream.Features.Get<IStreamAbortFeature>();
+
+        // Assert
+        Assert.IsType<QuicStreamContext>(protocolErrorCodeFeature);
+        Assert.Throws<ArgumentOutOfRangeException>(() => protocolErrorCodeFeature.AbortRead(errorCode, new ConnectionAbortedException()));
+        Assert.Throws<ArgumentOutOfRangeException>(() => protocolErrorCodeFeature.AbortWrite(errorCode, new ConnectionAbortedException()));
+    }
 }
diff --git a/src/Servers/Kestrel/test/Interop.FunctionalTests/Http3/Http3RequestTests.cs b/src/Servers/Kestrel/test/Interop.FunctionalTests/Http3/Http3RequestTests.cs
@@ -13,17 +13,16 @@
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Internal;
+using Microsoft.AspNetCore.InternalTesting;
 using Microsoft.AspNetCore.Server.Kestrel.Core;
 using Microsoft.AspNetCore.Server.Kestrel.Https;
-using Microsoft.AspNetCore.InternalTesting;
+using Microsoft.AspNetCore.Server.Kestrel.Transport.Quic;
 using Microsoft.Extensions.DependencyInjection;
-using Microsoft.Extensions.Diagnostics.Metrics;
 using Microsoft.Extensions.Diagnostics.Metrics.Testing;
 using Microsoft.Extensions.Hosting;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Testing;
 using Microsoft.Extensions.Primitives;
-using Xunit;
 
 namespace Interop.FunctionalTests.Http3;
 
@@ -2031,6 +2030,34 @@ public async Task GET_GracefulServerShutdown_RequestCompleteSuccessfullyInsideHo
         }
     }
 
+    [ConditionalFact]
+    [MsQuicSupported]
+    public async Task ServerReset_InvalidErrorCode()
+    {
+        var ranHandler = false;
+        var hostBuilder = CreateHostBuilder(context =>
+        {
+            ranHandler = true;
+            // Can't test a too-large value since it's bigger than int
+            //Assert.Throws<ArgumentOutOfRangeException>(() => context.Features.Get<IHttpResetFeature>().Reset(-1)); // Invalid negative value
+            context.Features.Get<IHttpResetFeature>().Reset(-1);
+            return Task.CompletedTask;
+        });
+
+        using var host = await hostBuilder.StartAsync().DefaultTimeout();
+        using var client = HttpHelpers.CreateClient();
+
+        var request = new HttpRequestMessage(HttpMethod.Get, $"https://127.0.0.1:{host.GetPort()}/");
+        request.Version = GetProtocol(HttpProtocols.Http3);
+        request.VersionPolicy = HttpVersionPolicy.RequestVersionExact;
+
+        var response = await client.SendAsync(request, CancellationToken.None).DefaultTimeout();
+        await host.StopAsync().DefaultTimeout();
+
+        Assert.True(ranHandler);
+        Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode);
+    }
+
     private IHostBuilder CreateHostBuilder(RequestDelegate requestDelegate, HttpProtocols? protocol = null, Action<KestrelServerOptions> configureKestrel = null)
     {
         return HttpHelpers.CreateHostBuilder(AddTestLogging, requestDelegate, protocol, configureKestrel);

Original file line number	Diff line number	Diff line change
`@@ -166,6 +166,9 @@ private void AbortCore(Exception exception, Http3ErrorCode errorCode)`
`166`	`166`	`abortReason = new ConnectionAbortedException(exception.Message, exception);`
`167`	`167`	`}`
`168`	`168`
	`169`	`+ // This has the side-effect of validating the error code, so do it before we consume the error code`
	`170`	`+ _errorCodeFeature.Error = (long)errorCode;`
	`171`	`+`
`169`	`172`	`_context.WebTransportSession?.Abort(abortReason, errorCode);`
`170`	`173`
`171`	`174`	`Log.Http3StreamAbort(TraceIdentifier, errorCode, abortReason);`
`@@ -181,7 +184,6 @@ private void AbortCore(Exception exception, Http3ErrorCode errorCode)`
`181`	`184`	`RequestBodyPipe.Writer.Complete(exception);`
`182`	`185`
`183`	`186`	`// Abort framewriter and underlying transport after stopping output.`
`184`		`- _errorCodeFeature.Error = (long)errorCode;`
`185`	`187`	`_frameWriter.Abort(abortReason);`
`186`	`188`	`}`
`187`	`189`	`}`
Original file line number	Diff line number	Diff line change
`@@ -16,7 +16,11 @@ internal sealed partial class QuicConnectionContext : IProtocolErrorCodeFeature,`
`16`	`16`	`public long Error`
`17`	`17`	`{`
`18`	`18`	`get => _error ?? -1;`
`19`		`- set => _error = value;`
	`19`	`+ set`
	`20`	`+ {`
	`21`	`+ QuicTransportOptions.ValidateErrorCode(value);`
	`22`	`+ _error = value;`
	`23`	`+ }`
`20`	`24`	`}`
`21`	`25`
`22`	`26`	`public X509Certificate2? ClientCertificate`
Original file line number	Diff line number	Diff line change
`@@ -56,6 +56,7 @@ public override async ValueTask DisposeAsync()`
`56`	`56`	`{`
`57`	`57`	`lock (_shutdownLock)`
`58`	`58`	`{`
	`59`	`+ // The DefaultCloseErrorCode setter validates that the error code is within the valid range`
`59`	`60`	`_closeTask ??= _connection.CloseAsync(errorCode: _context.Options.DefaultCloseErrorCode).AsTask();`
`60`	`61`	`}`
`61`	`62`
`@@ -81,7 +82,7 @@ public override void Abort(ConnectionAbortedException abortReason)`
`81`	`82`	`return;`
`82`	`83`	`}`
`83`	`84`
`84`		`- var resolvedErrorCode = _error ?? 0;`
	`85`	`+ var resolvedErrorCode = _error ?? 0; // Only valid error codes are assigned to _error`
`85`	`86`	`_abortReason = ExceptionDispatchInfo.Capture(abortReason);`
`86`	`87`	`QuicLog.ConnectionAbort(_log, this, resolvedErrorCode, abortReason.Message);`
`87`	`88`	`_closeTask = _connection.CloseAsync(errorCode: resolvedErrorCode).AsTask();`
`@@ -130,7 +131,7 @@ public override void Abort(ConnectionAbortedException abortReason)`
`130`	`131`	`catch (QuicException ex) when (ex.QuicError == QuicError.ConnectionAborted)`
`131`	`132`	`{`
`132`	`133`	`// Shutdown initiated by peer, abortive.`
`133`		`- _error = ex.ApplicationErrorCode;`
	`134`	`+ _error = ex.ApplicationErrorCode; // Trust Quic to provide us a valid error code`
`134`	`135`	`QuicLog.ConnectionAborted(_log, this, ex.ApplicationErrorCode.GetValueOrDefault(), ex);`
`135`	`136`
`136`	`137`	`ThreadPool.UnsafeQueueUserWorkItem(state =>`
Original file line number	Diff line number	Diff line change
`@@ -38,7 +38,11 @@ public OnCloseRegistration(Action<object?> callback, object? state)`
`38`	`38`	`public long Error`
`39`	`39`	`{`
`40`	`40`	`get => _error ?? -1;`
`41`		`- set => _error = value;`
	`41`	`+ set`
	`42`	`+ {`
	`43`	`+ QuicTransportOptions.ValidateErrorCode(value);`
	`44`	`+ _error = value;`
	`45`	`+ }`
`42`	`46`	`}`
`43`	`47`
`44`	`48`	`public long StreamId { get; private set; }`
`@@ -54,6 +58,8 @@ public long Error`
`54`	`58`
`55`	`59`	`public void AbortRead(long errorCode, ConnectionAbortedException abortReason)`
`56`	`60`	`{`
	`61`	`+ QuicTransportOptions.ValidateErrorCode(errorCode);`
	`62`	`+`
`57`	`63`	`lock (_shutdownLock)`
`58`	`64`	`{`
`59`	`65`	`if (_stream != null)`
`@@ -74,6 +80,8 @@ public void AbortRead(long errorCode, ConnectionAbortedException abortReason)`
`74`	`80`
`75`	`81`	`public void AbortWrite(long errorCode, ConnectionAbortedException abortReason)`
`76`	`82`	`{`
	`83`	`+ QuicTransportOptions.ValidateErrorCode(errorCode);`
	`84`	`+`
`77`	`85`	`lock (_shutdownLock)`
`78`	`86`	`{`
`79`	`87`	`if (_stream != null)`
Original file line number	Diff line number	Diff line change
`@@ -68,14 +68,15 @@ public long DefaultCloseErrorCode`
`68`	`68`	`}`
`69`	`69`	`}`
`70`	`70`
`71`		`- private static void ValidateErrorCode(long errorCode)`
	`71`	`+ internal static void ValidateErrorCode(long errorCode)`
`72`	`72`	`{`
`73`	`73`	`const long MinErrorCode = 0;`
`74`	`74`	`const long MaxErrorCode = (1L << 62) - 1;`
`75`	`75`
`76`	`76`	`if (errorCode < MinErrorCode \|\| errorCode > MaxErrorCode)`
`77`	`77`	`{`
`78`		`- throw new ArgumentOutOfRangeException(nameof(errorCode), errorCode, $"A value between {MinErrorCode} and {MaxErrorCode} is required.");`
	`78`	`+ // Print the values in hex since the max is unintelligible in decimal`
	`79`	`+ throw new ArgumentOutOfRangeException(nameof(errorCode), errorCode, $"A value between 0x{MinErrorCode:x} and 0x{MaxErrorCode:x} is required.");`
`79`	`80`	`}`
`80`	`81`	`}`
`81`	`82`