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

[Maxim Dounin]

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/