Welcome! Log In Create A New Profile

Advanced

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

Maxim Dounin
March 17, 2021 08:56PM
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
Subject Author Views Posted

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

geniuss99 165 March 16, 2021 03:06PM

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

Maxim Dounin 19 March 16, 2021 07:46PM

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

geniuss99 15 March 17, 2021 11:28AM

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

Maxim Dounin 18 March 17, 2021 08:56PM

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

geniuss99 16 March 19, 2021 12:34PM

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

Maxim Dounin 35 March 23, 2021 11:14AM

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

Sergey Kandaurov 16 March 23, 2021 08:26AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 55
Record Number of Users: 6 on February 13, 2018
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready