[PATCH] gRPC: fixed bug when padding is used in DATA frame

[Maxim Dounin]

Hello!

On Tue, Mar 16, 2021 at 10:04:27PM +0300, geniuss99 wrote:

>  src/http/modules/ngx_http_grpc_module.c |  16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)
> 
> 
> # HG changeset patch
> # User geniuss99 <geniuss.dev at gmail.com>
> # Date 1615921026 -10800
> #      Tue Mar 16 21:57:06 2021 +0300
> # Node ID 13552d5b785104f9d137d956a6cbef25ec09b345
> # Parent  ed1348e8e25381b3b1a2540289effcf7ccec6fd6
> gRPC: fixed bug when padding is used in DATA frame.
> 
> As per RFC 7540 DATA frame MAY contain padding if the PADDED flag is set
> in "Type" field (see clause 6.1).
> 
> When such frame is sent from an upstream which previously defined payload
> length via "Content-Length" header nginx fails with an error: "upstream sent
> response body larger than indicated content length".
> 
> This happens because padding is not taken into account while comparing expected
> and received payload length.

Thanks for reporting this, looks like a bug introduced by me in 
7680:39501ce97e29 (nginx 1.19.1).

Do you have any particular servers/gRPC libraries in mind which 
actually use padding on DATA frames, and so nginx cannot talk with 
them after introduction of the check in question?

> 
> diff -r ed1348e8e253 -r 13552d5b7851 src/http/modules/ngx_http_grpc_module.c
> --- a/src/http/modules/ngx_http_grpc_module.c	Thu Mar 11 20:49:39 2021 +0300
> +++ b/src/http/modules/ngx_http_grpc_module.c	Tue Mar 16 21:57:06 2021 +0300
> @@ -2075,14 +2075,24 @@
>                  }
>  
>                  if (ctx->length != -1) {
> -                    if ((off_t) ctx->rest > ctx->length) {
> +                    if (ctx->flags & NGX_HTTP_V2_PADDED_FLAG) {
> +                        if (b->pos < b->last) {
> +                            u_char pad_length = *b->pos;
> +                            size_t payload_data_length = ctx->rest - pad_length - 1; // frame_payload_size_bytes - pad_length_bytes - pad_length_field_1_byte
> +
> +                            ctx->length -= payload_data_length;
> +                        }
> +
> +                    } else {
> +                        ctx->length -= ctx->rest;
> +                    }
> +
> +                    if (ctx->length < 0) {
>                          ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
>                                        "upstream sent response body larger "
>                                        "than indicated content length");
>                          return NGX_ERROR;
>                      }
> -
> -                    ctx->length -= ctx->rest;
>                  }
>  
>                  if (ctx->rest > ctx->recv_window) {

The fix looks suboptimal.  Rather, the check should be moved to 
the end of the function, where padding is already handled.  I'll 
take a look how to fix this properly.

(Note well that as per nginx style, local variables are defined at 
the start of the function, "//" comments are not used, and maximum 
line length is 80 characters.  The "Contributing Changes" 
article[1] have additional hints and links to appropriate part of 
the development guide with the details.)

[1] http://nginx.org/en/docs/contributing_changes.html

-- 
Maxim Dounin
http://mdounin.ru/