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

geniuss99 geniuss.dev at gmail.com
Fri Mar 19 16:32:31 UTC 2021


Made some tests. Looks fine.
Thanks!

On Thu, Mar 18, 2021 at 3:56 AM 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) {
>
> --
> Maxim Dounin
> http://mdounin.ru/
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel


More information about the nginx-devel mailing list