Made some tests. Looks fine.
Thanks!

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