diff --git a/src/IceRpc/Internal/IceProtocolConnection.cs b/src/IceRpc/Internal/IceProtocolConnection.cs index 4201a3fd1d..92fc964a83 100644 --- a/src/IceRpc/Internal/IceProtocolConnection.cs +++ b/src/IceRpc/Internal/IceProtocolConnection.cs @@ -689,6 +689,18 @@ private static (int RequestId, IceRequestHeader Header, PipeReader? ContextReade var requestHeader = new IceRequestHeader(ref decoder); requestHeader.Facet.CheckFacetCount(); + // Reject operation names that contain characters outside the printable ASCII range. The path is built from + // requestHeader.Identity via IceIdentity.ToPath, which percent-escapes any unsafe characters, so it does not + // need an additional check here. + try + { + ServiceAddress.CheckOperation(requestHeader.Operation); + } + catch (FormatException exception) + { + throw new InvalidDataException(exception.Message, exception); + } + Pipe? contextPipe = null; long pos = decoder.Consumed; int count = decoder.DecodeSize(); diff --git a/src/IceRpc/Internal/IceRpcProtocolConnection.cs b/src/IceRpc/Internal/IceRpcProtocolConnection.cs index 168f01d3c3..7d910382b9 100644 --- a/src/IceRpc/Internal/IceRpcProtocolConnection.cs +++ b/src/IceRpc/Internal/IceRpcProtocolConnection.cs @@ -1227,6 +1227,20 @@ async Task PerformDispatchRequestAsync( { var decoder = new SliceDecoder(buffer); var header = new IceRpcRequestHeader(ref decoder); + + // Reject paths and operation names that contain characters outside the printable ASCII range. This + // matches the constraints already enforced by ServiceAddress and by the Slice identifier grammar, and + // prevents control characters (notably CR/LF/NUL) from flowing into loggers and other downstream sinks. + try + { + ServiceAddress.CheckPath(header.Path); + ServiceAddress.CheckOperation(header.Operation); + } + catch (FormatException exception) + { + throw new InvalidDataException(exception.Message, exception); + } + (IDictionary> fields, PipeReader? pipeReader) = DecodeFieldDictionary( ref decoder, diff --git a/src/IceRpc/ServiceAddress.cs b/src/IceRpc/ServiceAddress.cs index 2e6fcac1db..23a09c2a31 100644 --- a/src/IceRpc/ServiceAddress.cs +++ b/src/IceRpc/ServiceAddress.cs @@ -490,7 +490,7 @@ internal static void CheckPath(string path) if (path.Length == 0 || path[0] != '/' || !IsValid(path, _notValidInPath)) { throw new FormatException( - $"Invalid path '{path}'; a valid path starts with '/' and contains only unreserved characters, '%', and reserved characters other than '?' and '#'."); + "Invalid path; a valid path starts with '/' and contains only unreserved characters, '%', and reserved characters other than '?' and '#'."); } } @@ -501,6 +501,23 @@ internal static void CheckPath(string path) /// . internal static bool IsValidParamValue(string value) => IsValid(value, _notValidInParamValue); + /// Checks if is a valid operation name. A valid operation name contains + /// only printable ASCII characters (in the inclusive range 0x21 to 0x7E) and may be empty. + /// + /// The operation name to check. + /// Thrown if the operation name is not valid. + /// This range covers every legal Slice identifier and rejects all C0 control characters as well as DEL + /// and any non-ASCII content. Empty operation names are accepted because + /// defaults to the empty string. + internal static void CheckOperation(string operation) + { + if (operation.AsSpan().IndexOfAnyExceptInRange(FirstValidChar, LastValidChar) != -1) + { + throw new FormatException( + "Invalid operation name; an operation name contains only printable ASCII characters."); + } + } + /// "unchecked" constructor used by the Ice decoder when decoding a service address. /// internal ServiceAddress( diff --git a/tests/IceRpc.Tests/IceProtocolConnectionTests.cs b/tests/IceRpc.Tests/IceProtocolConnectionTests.cs index b70cefad6e..a8b34b1d8c 100644 --- a/tests/IceRpc.Tests/IceProtocolConnectionTests.cs +++ b/tests/IceRpc.Tests/IceProtocolConnectionTests.cs @@ -175,6 +175,49 @@ public async Task Thrown_dispatcher_exception_is_classified(string exceptionKind Assert.That(response.StatusCode, Is.EqualTo(expectedStatusCode)); } + /// Verifies that a request whose operation name contains characters outside the printable ASCII range + /// is rejected at decode time so peer-controlled control characters cannot reach loggers and other downstream + /// sinks. The dispatcher must not be invoked, and because ice cannot recover from a decode failure mid-stream, + /// the connection aborts and the pending invocation fails with + /// . + [TestCase("op\r\nINJECTED")] // CR/LF in operation + [TestCase("op\0")] // NUL in operation + public async Task Request_with_invalid_operation_aborts_server_connection(string operation) + { + // Arrange + var dispatcher = new InlineDispatcher((request, cancellationToken) => + { + Assert.Fail("The dispatcher must not be invoked for a request with an invalid operation name."); + return new(new OutgoingResponse(request)); + }); + + await using ServiceProvider provider = new ServiceCollection() + .AddProtocolTest(Protocol.Ice, dispatcher) + .BuildServiceProvider(validateScopes: true); + + ClientServerProtocolConnection sut = provider.GetRequiredService(); + (_, Task serverShutdownRequested) = await sut.ConnectAsync(); + + using var request = new OutgoingRequest(new ServiceAddress(Protocol.Ice) { Path = "/foo" }) + { + Operation = operation + }; + Task invokeTask = sut.Client.InvokeAsync(request); + + // Wait until the server's read loop has rejected the invalid header and signaled shutdown, so the + // disposal below cannot race with request decoding. + await serverShutdownRequested; + + // Act: closing the server surfaces the abort to the client's pending invocation. ReadFailed signals + // shutdown but doesn't close the duplex connection on its own. + await sut.Server.DisposeAsync(); + + // Assert + Assert.That( + async () => await invokeTask, + Throws.InstanceOf().With.Property("IceRpcError").EqualTo(IceRpcError.ConnectionAborted)); + } + /// Verifies that a StatusCode dispatched by the server is encoded as a ReplyStatus and decoded back to /// the expected StatusCode by the client. [Test, TestCaseSource(nameof(StatusCodeRoundTripSource))] diff --git a/tests/IceRpc.Tests/IceRpcProtocolConnectionTests.cs b/tests/IceRpc.Tests/IceRpcProtocolConnectionTests.cs index f05522d484..e9ddc2ee64 100644 --- a/tests/IceRpc.Tests/IceRpcProtocolConnectionTests.cs +++ b/tests/IceRpc.Tests/IceRpcProtocolConnectionTests.cs @@ -446,6 +446,54 @@ public async Task Invalid_request_refused(byte[] invalidRequestBytes, bool succe Throws.Nothing); } + /// Verifies that a request whose path or operation name contains characters outside the printable + /// ASCII range is rejected at decode time, so peer-controlled control characters cannot reach loggers and + /// other downstream sinks. + [TestCase("/foo\r\nINJECTED", "op")] // CR/LF in path + [TestCase("/foo", "op\r\nINJECTED")] // CR/LF in operation + [TestCase("/foo\0", "op")] // NUL in path + [TestCase("/foo", "op\0")] // NUL in operation + [TestCase("foo", "op")] // path without leading '/' + public async Task Request_with_invalid_path_or_operation_is_rejected(string path, string operation) + { + // Arrange + var taskExceptionObserver = new TestTaskExceptionObserver(); + + await using ServiceProvider provider = new ServiceCollection() + .AddProtocolTest(Protocol.IceRpc) + .AddTestMultiplexedTransportDecorator() + .AddSingleton(taskExceptionObserver) + .BuildServiceProvider(validateScopes: true); + + ClientServerProtocolConnection sut = provider.GetRequiredService(); + await sut.ConnectAsync(); + + var clientTransport = provider.GetRequiredService(); + IMultiplexedConnection clientTransportConnection = clientTransport.LastCreatedConnection; + IMultiplexedStream stream = await clientTransportConnection.CreateStreamAsync(bidirectional: true, default); + + var writer = new MemoryBufferWriter(new byte[256]); + var encoder = new SliceEncoder(writer); + Span sizePlaceholder = encoder.GetPlaceholderSpan(4); + int startPos = encoder.EncodedByteCount; + encoder.EncodeString(path); + encoder.EncodeString(operation); + encoder.EncodeVarUInt62(0); // empty field dictionary + SliceEncoder.EncodeVarUInt62((ulong)(encoder.EncodedByteCount - startPos), sizePlaceholder); + + // Act + stream.Output.Write(writer.WrittenMemory.Span); + await stream.Output.FlushAsync(); + stream.Output.CompleteOutput(success: true); + + // Assert + Assert.That( + async () => await taskExceptionObserver.DispatchRefusedException, + Is.InstanceOf() + .With.Property("IceRpcError").EqualTo(IceRpcError.IceRpcError) + .And.InnerException.InstanceOf()); + } + /// Verifies that a request with a field count large enough that count * 2 would overflow int arithmetic /// is correctly rejected. [Test]