Bound gRPC frame sizes#220
Conversation
Reject oversized server-streaming gRPC messages as soon as their frame header is available. Also reject compressed stream frames consistently and avoid unchecked frame length arithmetic before slicing buffers.
Read unary client responses incrementally and reject bodies above the configured limit before protobuf decoding. Preallocate only after a valid Content-Length has been checked.
|
👋 Thanks for assigning @tnull as a reviewer! |
|
|
||
| // Applies to complete unary gRPC responses. The server applies the same cap to unary request | ||
| // bodies before protobuf decoding. | ||
| const MAX_GRPC_UNARY_RESPONSE_LEN: usize = 10 * 1024 * 1024; |
There was a problem hiding this comment.
wdyt about having the unit explicit in the const name? _BYTES instead of _LEN?
There was a problem hiding this comment.
i think that is implied
| Ok(payload) | ||
| } | ||
|
|
||
| fn check_grpc_unary_response_len(len: u64) -> Result<(), LdkServerError> { |
There was a problem hiding this comment.
if you drop the content_size path, you can change this to receive usize and avoid the as castings both on L535 and L542
There was a problem hiding this comment.
if we do that then we need to either cast the content_length before validating it which is unsafe, or do a try_into which ends up being more ugly than these casts.
| content_length as usize | ||
| } else { | ||
| 0 | ||
| }; |
There was a problem hiding this comment.
I think the Some path is essentially useless as ldk-server doesn't set the content length header. It actually could't even if it wanted to. In gRPC the body streams and you can't compute a length.
Also, maybe worth noting that having Vec::with_capacity(0) hardcoded it's kinda bad because it will always have to allocate. Given the context where this is running it seems like not that big of a deal. You could initialize for something different than zero, or maybe use some BytesMut trickery to alloc exactly what you need.
There was a problem hiding this comment.
Vec::with_capacity(0) is the same as Vec::new() so this is still an improvement from the pervious version but yeah, I should make sure we return the content length from the server
|
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
We hand-rolled gRPC to reduce exposure to supply chain risk, but I think this PR demonstrates that it is a trade-off because we might introduce vulnerabilities ourselves. Hopefully we can keep the gRPC feature set small enough that the balance does not shift unfavourably. |
|
added 2 commits where we make sure we always send the content length on both client and server to make the handling cleaner |
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
I'm not sure I'm the right person to review this, don't have a lot of context on gRPC. Claude seems to think this is fine.
5 participants
Reject oversized server-streaming gRPC messages as soon as their frame
header is available. Also reject compressed stream frames consistently
and avoid unchecked frame length arithmetic before slicing buffers.
Read unary client responses incrementally and reject bodies above the
configured limit before protobuf decoding. Preallocate only after a
valid Content-Length has been checked.
tested with all the graph clis and they still fit within the limit