Config max request message size to fend off potential malicious attacks

[Ajkcki]

Feature Request

If a message size is maliciously large, blindly accept it may not be a good idea.

A quick test shows that with the current tonic server (0.8.1), a simple hello-world unary service was able to take a 1GB size request message and return a response; even though the service handler always responds "hello world" without processing the request at all, just accepting such a message significantly slow down the server. It would be more reasonable to drop the connection instead of blindly consuming system resources to decompress/parse such a large message.

This is the same request as in #264.

[LucioFranco]

I know this isn't the same setting you're looking for but tonic 0.8.2 which was released last week now has a default body limit size thanks to axum https://docs.rs/axum/latest/axum/extract/struct.DefaultBodyLimit.html which limits bodies to 2mb. I believe this should work to mitigate this sort of attack? Would be good to run your test against that.

[davidpdrsn]

https://docs.rs/tower-http/latest/tower_http/limit/index.html should work with tonic though it wont give gRPC specific responses.

[LucioFranco]

@davidpdrsn I wonder if we can make it smart and detect grpc and reply in a grpc fashion if so? Though we could also just implement this into tonic.

[davidpdrsn]

Yeah that would be nice. Already have some gRPC stuff in tower-http so makes sense to support for limited bodies as well.

[Ajkcki]

Just pulled tonic 0.8.2 and re-run the test, the same behavior, the server can still accept large request messages . @LucioFranco can you please elaborate how to set up this body limit? Say, in the following helloworld example, how to limit the size of message HelloRequest, which is generated from a protobuf definition

message HelloRequest {
  string name = 1;
}

The client can send over an arbitrarily long string as string name.

#[derive(Default)]
pub struct MyGreeter {}

#[tonic::async_trait]
impl Greeter for MyGreeter {
    async fn say_hello(
        &self,
        request: Request<HelloRequest>,
    ) -> Result<Response<HelloReply>, Status> {
        println!("Got a request from {:?}", request.remote_addr());

        let reply = hello_world::HelloReply {
            message: format!("Hello {}!", request.into_inner().name),
        };
        Ok(Response::new(reply))
    }
}

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
    let addr = "[::1]:50051".parse().unwrap();
    let greeter = MyGreeter::default();

    println!("GreeterServer listening on {}", addr);

    Server::builder()
        .add_service(GreeterServer::new(greeter))
        .serve(addr)
        .await?;

    Ok(())
}

[davidpdrsn]

@Ajkcki you need to use https://docs.rs/tower-http/latest/tower_http/limit/index.html

[Ajkcki]

@davidpdrsn I tried, but tower_http didn't seem to work. Any idea how to fix it?

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
    let addr = "[::1]:50051".parse().unwrap();
    let greeter = MyGreeter::default();

    println!("GreeterServer listening on {}", addr);

    Server::builder()
        .layer(tower::timeout::TimeoutLayer::new(Duration::from_secs(30)))       //<======= this works
        .layer(tower_http::limit::RequestBodyLimitLayer::new(4096))           //<=======  error 
        .add_service(GreeterServer::new(greeter))
        .serve(addr)
        .await?;

    Ok(())
}

Here is the error message

error[E0277]: the trait bound `Routes: Service<tonic::codegen::http::Request<http_body::limited::Limited<tonic::transport::Body>>>` is not satisfied
   --> src/main.rs:169:10
    |
169 |         .serve(addr)
    |          ^^^^^ the trait `Service<tonic::codegen::http::Request<http_body::limited::Limited<tonic::transport::Body>>>` is not implemented for `Routes`
    |
    = help: the trait `Service<tonic::codegen::http::Request<tonic::transport::Body>>` is implemented for `Routes`
    = note: required because of the requirements on the impl of `Service<tonic::codegen::http::Request<tonic::transport::Body>>` for `RequestBodyLimit<Routes>`

[LucioFranco]

Yeah, so I think we will need to implement our own length limit but I don't think its trivial since it needs to cross a couple config bounds. We will need to pass a length limit into the decode/encode part, I think we can follow a similar path to what compression did. OR rethink how we abstract config between the transport and grpc protocol.

[aoudiamoncef]

Hi @LucioFranco,

Is there any updates about this issue, I see that there is related issues which was merged
#529

Thanks

[LucioFranco]

My comment above is still accurate, I have not had time to work on this so if someone is interested I can help mentor.

[aoudiamoncef]

Hi @LucioFranco,

I'm interested to contribute, I did a quick look at the codebase:

tonic/tonic/src/codec/decode.rs

Line 154 in 8277071

let len = self.buf.get_u32() as usize;

Adding a control on len with a max_message_size with a default value of 4MB could do the trick.

For example

if len > self.max_message_size {
   return Err(Status::new(Code::OutOfRange, format!("Error, message too large: found {} bytes, maximum authorised {} bytes", len, self.max_message_size)));
}

Same for the encoder

If you have a better idea, you're welcome, I'll be glad to implement It.

[LucioFranco]

Yeah, I think that should work, I think the interesting part will be with how we configure it.

[aoudiamoncef]

I have some questions before implementing it:

• We'll have the same config to control the maximum size of message in both encoder/decoder ? Or max_encoding_message_size/max_decoder_message_size

• Intuitively, adding the config in server builder seems to be the easiest way ?

[LucioFranco]

We'll have the same config to control the maximum size of message in both encoder/decoder ? Or max_encoding_message_size/max_decoder_message_size

Yeah, I think having two configs should be fine.

Intuitively, adding the config in server builder seems to be the easiest way ?

Probably should be done similar to how these are setup https://github.com/hyperium/tonic/blob/master/tonic-build/src/server.rs#L71