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

[Sergey Kandaurov]

> On 18 Mar 2021, at 03:55, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 
> Hello!
> 
> On Wed, Mar 17, 2021 at 06:27:17PM +0300, geniuss99 wrote:
> 
> [...]
> 
>>> 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?
>> 
>> I use gRPC module to pass normal http requests to h2 upstreams, not gRPC ones.
>> Works like a charm.
>> 
>> So can't give you any gRPC servers for testing. You can observe the bug by
>> connecting to developers.google.com h2 server for example. They use padding
>> in DATA streams.
> 
> Thanks for the example.  Interesting, it only use padding when 
> returning compressed responses, likely as an attempt to mitigate 
> BREACH attacks (https://en.wikipedia.org/wiki/BREACH).
> 
> The following patch seem to fix this properly.  Review and testing 
> appreciated.
> 
> # HG changeset patch
> # User Maxim Dounin <mdounin at mdounin.ru>
> # Date 1616027280 -10800
> #      Thu Mar 18 03:28:00 2021 +0300
> # Node ID f8dbaa4ae09de125420e5a325b2ccac4a5636494
> # Parent  0215ec9aaa8af6036c62e1db676c9b0cc1d5fca4
> gRPC: fixed handling of padding on DATA frames.
> 
> The response size check introduced in 39501ce97e29 did not take into
> account possible padding on DATA frames, resulting in incorrect
> "upstream sent response body larger than indicated content length" errors
> if upstream server used padding in responses with known length.
> 
> Fix is to check the actual size of response buffers produced by the code,
> similarly to how it is done in other protocols, instead of checking
> the size of DATA frames.
> 
> Reported at:
> http://mailman.nginx.org/pipermail/nginx-devel/2021-March/013907.html
> 
> diff --git a/src/http/modules/ngx_http_grpc_module.c b/src/http/modules/ngx_http_grpc_module.c
> --- a/src/http/modules/ngx_http_grpc_module.c
> +++ b/src/http/modules/ngx_http_grpc_module.c
> @@ -2074,17 +2074,6 @@ ngx_http_grpc_filter(void *data, ssize_t
>                     return NGX_ERROR;
>                 }
> 
> -                if (ctx->length != -1) {
> -                    if ((off_t) ctx->rest > ctx->length) {
> -                        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) {
>                     ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
>                                   "upstream violated stream flow control, "
> @@ -2450,6 +2439,18 @@ ngx_http_grpc_filter(void *data, ssize_t
>             b->pos = b->last;
>             buf->last = b->pos;
> 
> +            if (ctx->length != -1) {
> +
> +                if (buf->last - buf->pos > ctx->length) {
> +                    ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
> +                                  "upstream sent response body larger "
> +                                  "than indicated content length");
> +                    return NGX_ERROR;
> +                }
> +
> +                ctx->length -= buf->last - buf->pos;
> +            }
> +
>             return NGX_AGAIN;
>         }
> 
> @@ -2457,6 +2458,18 @@ ngx_http_grpc_filter(void *data, ssize_t
>         buf->last = b->pos;
>         ctx->rest = ctx->padding;
> 
> +        if (ctx->length != -1) {
> +
> +            if (buf->last - buf->pos > ctx->length) {
> +                ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
> +                              "upstream sent response body larger "
> +                              "than indicated content length");
> +                return NGX_ERROR;
> +            }
> +
> +            ctx->length -= buf->last - buf->pos;
> +        }
> +
>     done:
> 
>         if (ctx->padding) {
> 

Looks good.

-- 
Sergey Kandaurov