Reject control chars in request headers#4565
Conversation
Validate the path and operation name of incoming requests at decode time so peer-controlled control characters cannot reach loggers and other downstream text-based sinks. Sanitize the offending value embedded in the FormatException message to stop the same payload from leaking into log output via the wrapped exception.
The path/operation only fails this check on bogus or malicious requests, and echoing the value back in the exception message risks leaking the peer-controlled string into logs that render the wrapped exception. Removing it is simpler than sanitizing.
There was a problem hiding this comment.
Pull request overview
This PR hardens request header decoding by validating incoming request paths and operation names so peer-controlled control characters (and other non-printable/non-ASCII characters) can’t flow into loggers or other text-based sinks, addressing #4432.
Changes:
- Add
ServiceAddress.CheckOperationto validate operation names as printable ASCII (or empty). - Validate decoded
Path/Operationat decode time for IceRpc, and decodedOperationfor Ice, mapping failures toInvalidDataException. - Add regression tests ensuring invalid path/operation headers are rejected/abort as expected.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/IceRpc.Tests/IceRpcProtocolConnectionTests.cs | Adds IceRpc tests that invalid path/operation values are refused at decode time. |
| tests/IceRpc.Tests/IceProtocolConnectionTests.cs | Adds Ice test ensuring invalid operation aborts the server connection and dispatcher is not invoked. |
| src/IceRpc/ServiceAddress.cs | Removes path echo from CheckPath error; adds CheckOperation validation helper for operation names. |
| src/IceRpc/Internal/IceRpcProtocolConnection.cs | Validates decoded IceRpc request Path and Operation and rejects invalid headers early. |
| src/IceRpc/Internal/IceProtocolConnection.cs | Validates decoded Ice request Operation and converts invalid values into decode-time failures. |
| // prevents control characters (notably CR/LF/NUL) from flowing into loggers and other downstream sinks. | ||
| try | ||
| { | ||
| ServiceAddress.CheckPath(header.Path); |
There was a problem hiding this comment.
I'm not sure that IceRPC has the same restriction as Ice here. i.e. ascii only characters.
There was a problem hiding this comment.
For Path we already have this check in ServiceAddress, so it makes sense we don't accept paths that are not valid according to the ServiceAddress rules.
For Operation I guess we didn't really specify what a valid operation names is in the "icerpc" protocol, at the protocol level is just a string. And this should probably be handled in the integration layer if at all, for example Slice operations are derived from Slice identifiers and have the same limitations that the identifier has. Same for Protobuf, they are derived from the Protobuf IDL and have its own rules.
I think is probably wrong to check the operation name here, as the protocol layer doesn't specify what the operation name is, checking the path seems correct because ServiceAddress format is specified in the protocol.
There was a problem hiding this comment.
Ok, I couldn't find anywhere in the docs were we way we only accept ASCII chars (but maybe its there?).
Is this check really required here? ServiceAddress' Path init function already does this check, no?
There was a problem hiding this comment.
Is this check really required here? ServiceAddress' Path init function already does this check, no?
ServiceAddress is not used here, we decode the Path as a string, using IceRpcRequestHeader
icerpc-csharp/slice/IceRpc/Internal/IceRpcDefinitions.slice
Lines 21 to 26 in 0909b4b
Then the path string is assigned to IncomingRequest.Path, another string without further validation.
Maybe is clear to move the validation to IncomingRequest.Path init
There was a problem hiding this comment.
Right. Yeah it would make sense to also have it there.
Reminds me of https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate
InsertCreativityHere
left a comment
InsertCreativityHere
left a comment
There was a problem hiding this comment.
Code changes look good to me!
But I think I will blog about what comprises a valid operation name in Slice.
I also don't think checking it at the IceRpc level is quite right, but it fine for now.
I'd prefer operation names to just be "strings comprised of alphanumeric characters".
And not limit them to purely printable ASCII characters. The fact Slice identifiers are ASCII only is more a reflection of my inability to write a super cool parser : v) not of the Slice spec.
| { | ||
| 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 '#'."); |
There was a problem hiding this comment.
I think printing the 'invalid path' (like we did before) would be a valuable piece of information for debugging.
I recognize there's a tradeoff where: if there's control characters, the message might look weird. but I think the benefit of seeing "oh, 𒀗𒀝/𒀱 was the bad path" outweighs the very unlikely situation where you've gotten control or formatting characters in here.
Validate the path and operation name of incoming requests at decode time so peer-controlled control characters cannot reach loggers and other downstream text-based sinks.
Fix #4432