From 9ab38e8ee35fc08a57636b1b6190dca70b0076fa Mon Sep 17 00:00:00 2001 From: Thibault Charbonnier Date: Mon, 23 Mar 2020 19:40:47 -0700 Origin: https://github.com/openresty/lua-nginx-module/commit/9ab38e8ee35fc08a57636b1b6190dca70b0076fa Subject: [PATCH] bugfix: prevented request smuggling in the ngx.location.capture API. Signed-off-by: Yichun Zhang (agentzh) --- src/ngx_http_lua_subrequest.c | 196 +++++-------- t/020-subrequest.t | 520 +++++++++++++++++++++++++++++++++- 2 files changed, 585 insertions(+), 131 deletions(-) Index: http-lua/src/ngx_http_lua_subrequest.c =================================================================== --- http-lua.orig/src/ngx_http_lua_subrequest.c 2020-07-14 09:50:35.830928117 +0200 +++ http-lua/src/ngx_http_lua_subrequest.c 2020-07-14 09:50:35.826928232 +0200 @@ -56,8 +56,6 @@ ngx_string("Content-Length"); -static ngx_int_t ngx_http_lua_set_content_length_header(ngx_http_request_t *r, - off_t len); static ngx_int_t ngx_http_lua_adjust_subrequest(ngx_http_request_t *sr, ngx_uint_t method, int forward_body, ngx_http_request_body_t *body, unsigned vars_action, @@ -78,7 +76,7 @@ static ngx_int_t ngx_http_post_request_to_head(ngx_http_request_t *r); static ngx_int_t ngx_http_lua_copy_in_file_request_body(ngx_http_request_t *r); static ngx_int_t ngx_http_lua_copy_request_headers(ngx_http_request_t *sr, - ngx_http_request_t *r); + ngx_http_request_t *pr, int pr_not_chunked); /* ngx.location.capture is just a thin wrapper around @@ -628,8 +626,8 @@ unsigned vars_action, ngx_array_t *extra_vars) { ngx_http_request_t *r; - ngx_int_t rc; ngx_http_core_main_conf_t *cmcf; + int pr_not_chunked = 0; size_t size; r = sr->parent; @@ -639,46 +637,32 @@ if (body) { sr->request_body = body; - rc = ngx_http_lua_set_content_length_header(sr, - body->buf - ? ngx_buf_size(body->buf) - : 0); - - if (rc != NGX_OK) { - return NGX_ERROR; - } - } else if (!always_forward_body && method != NGX_HTTP_PUT && method != NGX_HTTP_POST && r->headers_in.content_length_n > 0) { - rc = ngx_http_lua_set_content_length_header(sr, 0); - if (rc != NGX_OK) { - return NGX_ERROR; - } - -#if 1 sr->request_body = NULL; -#endif } else { - if (ngx_http_lua_copy_request_headers(sr, r) != NGX_OK) { - return NGX_ERROR; + if (!r->headers_in.chunked) { + pr_not_chunked = 1; } - if (sr->request_body) { + if (sr->request_body && sr->request_body->temp_file) { /* deep-copy the request body */ - if (sr->request_body->temp_file) { - if (ngx_http_lua_copy_in_file_request_body(sr) != NGX_OK) { - return NGX_ERROR; - } + if (ngx_http_lua_copy_in_file_request_body(sr) != NGX_OK) { + return NGX_ERROR; } } } + if (ngx_http_lua_copy_request_headers(sr, r, pr_not_chunked) != NGX_OK) { + return NGX_ERROR; + } + sr->method = method; switch (method) { @@ -1124,100 +1108,6 @@ } -static ngx_int_t -ngx_http_lua_set_content_length_header(ngx_http_request_t *r, off_t len) -{ - ngx_table_elt_t *h, *header; - u_char *p; - ngx_list_part_t *part; - ngx_http_request_t *pr; - ngx_uint_t i; - - r->headers_in.content_length_n = len; - - if (ngx_list_init(&r->headers_in.headers, r->pool, 20, - sizeof(ngx_table_elt_t)) != NGX_OK) - { - return NGX_ERROR; - } - - h = ngx_list_push(&r->headers_in.headers); - if (h == NULL) { - return NGX_ERROR; - } - - h->key = ngx_http_lua_content_length_header_key; - h->lowcase_key = ngx_pnalloc(r->pool, h->key.len); - if (h->lowcase_key == NULL) { - return NGX_ERROR; - } - - ngx_strlow(h->lowcase_key, h->key.data, h->key.len); - - r->headers_in.content_length = h; - - p = ngx_palloc(r->pool, NGX_OFF_T_LEN); - if (p == NULL) { - return NGX_ERROR; - } - - h->value.data = p; - - h->value.len = ngx_sprintf(h->value.data, "%O", len) - h->value.data; - - h->hash = ngx_http_lua_content_length_hash; - -#if 0 - dd("content length hash: %lu == %lu", (unsigned long) h->hash, - ngx_hash_key_lc((u_char *) "Content-Length", - sizeof("Content-Length") - 1)); -#endif - - dd("r content length: %.*s", - (int) r->headers_in.content_length->value.len, - r->headers_in.content_length->value.data); - - pr = r->parent; - - if (pr == NULL) { - return NGX_OK; - } - - /* forward the parent request's all other request headers */ - - part = &pr->headers_in.headers.part; - header = part->elts; - - for (i = 0; /* void */; i++) { - - if (i >= part->nelts) { - if (part->next == NULL) { - break; - } - - part = part->next; - header = part->elts; - i = 0; - } - - if (header[i].key.len == sizeof("Content-Length") - 1 - && ngx_strncasecmp(header[i].key.data, (u_char *) "Content-Length", - sizeof("Content-Length") - 1) == 0) - { - continue; - } - - if (ngx_http_lua_set_input_header(r, header[i].key, - header[i].value, 0) == NGX_ERROR) - { - return NGX_ERROR; - } - } - - return NGX_OK; -} - - static void ngx_http_lua_handle_subreq_responses(ngx_http_request_t *r, ngx_http_lua_ctx_t *ctx) @@ -1732,11 +1622,17 @@ static ngx_int_t -ngx_http_lua_copy_request_headers(ngx_http_request_t *sr, ngx_http_request_t *r) +ngx_http_lua_copy_request_headers(ngx_http_request_t *sr, + ngx_http_request_t *pr, int pr_not_chunked) { - ngx_table_elt_t *header; + ngx_table_elt_t *clh, *header; ngx_list_part_t *part; ngx_uint_t i; + u_char *p; + off_t len; + + dd("before: parent req headers count: %d", + (int) pr->headers_in.headers.part.nelts); if (ngx_list_init(&sr->headers_in.headers, sr->pool, 20, sizeof(ngx_table_elt_t)) != NGX_OK) @@ -1744,10 +1640,46 @@ return NGX_ERROR; } - dd("before: parent req headers count: %d", - (int) r->headers_in.headers.part.nelts); + if (sr->request_body && !pr_not_chunked) { + + /* craft our own Content-Length */ + + len = sr->request_body->buf ? ngx_buf_size(sr->request_body->buf) : 0; + + clh = ngx_list_push(&sr->headers_in.headers); + if (clh == NULL) { + return NGX_ERROR; + } - part = &r->headers_in.headers.part; + clh->hash = ngx_http_lua_content_length_hash; + clh->key = ngx_http_lua_content_length_header_key; + clh->lowcase_key = ngx_pnalloc(sr->pool, clh->key.len); + if (clh->lowcase_key == NULL) { + return NGX_ERROR; + } + + ngx_strlow(clh->lowcase_key, clh->key.data, clh->key.len); + + p = ngx_palloc(sr->pool, NGX_OFF_T_LEN); + if (p == NULL) { + return NGX_ERROR; + } + + clh->value.data = p; + clh->value.len = ngx_sprintf(clh->value.data, "%O", len) + - clh->value.data; + + sr->headers_in.content_length = clh; + sr->headers_in.content_length_n = len; + + dd("sr crafted content-length: %.*s", + (int) sr->headers_in.content_length->value.len, + sr->headers_in.content_length->value.data); + } + + /* copy the parent request's headers */ + + part = &pr->headers_in.headers.part; header = part->elts; for (i = 0; /* void */; i++) { @@ -1762,7 +1694,14 @@ i = 0; } - dd("setting request header %.*s: %.*s", (int) header[i].key.len, + if (!pr_not_chunked && header[i].key.len == sizeof("Content-Length") - 1 + && ngx_strncasecmp(header[i].key.data, (u_char *) "Content-Length", + sizeof("Content-Length") - 1) == 0) + { + continue; + } + + dd("sr copied req header %.*s: %.*s", (int) header[i].key.len, header[i].key.data, (int) header[i].value.len, header[i].value.data); @@ -1774,9 +1713,10 @@ } dd("after: parent req headers count: %d", - (int) r->headers_in.headers.part.nelts); + (int) pr->headers_in.headers.part.nelts); return NGX_OK; } + /* vi:set ft=c ts=4 sw=4 et fdm=marker: */ Index: http-lua/t/020-subrequest.t =================================================================== --- http-lua.orig/t/020-subrequest.t 2020-07-14 09:50:35.830928117 +0200 +++ http-lua/t/020-subrequest.t 2020-07-14 09:50:35.826928232 +0200 @@ -14,6 +14,7 @@ plan tests => repeat_each() * (blocks() * 3 + 23); $ENV{TEST_NGINX_MEMCACHED_PORT} ||= 11211; +$ENV{TEST_NGINX_HTML_DIR} ||= html_dir(); #no_diff(); no_long_string(); @@ -210,7 +211,7 @@ -=== TEST 8: PUT (nobody, proxy method) +=== TEST 8: PUT (with body, proxy method) --- config location /other { default_type 'foo/bar'; @@ -242,7 +243,7 @@ -=== TEST 9: PUT (nobody, no proxy method) +=== TEST 9: PUT (with body, no proxy method) --- config location /other { default_type 'foo/bar'; @@ -271,7 +272,7 @@ -=== TEST 10: PUT (nobody, no proxy method) +=== TEST 10: PUT (no body, no proxy method) --- config location /other { default_type 'foo/bar'; @@ -2877,3 +2878,516 @@ --- no_error_log [error] --- error_code: 200 + + + +=== TEST 77: avoid request smuggling 1/4 (default capture + smuggle in header) +--- http_config + upstream backend { + server unix:$TEST_NGINX_HTML_DIR/nginx.sock; + keepalive 32; + } + + server { + listen unix:$TEST_NGINX_HTML_DIR/nginx.sock; + + location / { + content_by_lua_block { + ngx.say("method: ", ngx.var.request_method, + ", uri: ", ngx.var.uri, + ", X: ", ngx.var.http_x) + } + } + } +--- config + location /proxy { + proxy_http_version 1.1; + proxy_set_header Connection ""; + proxy_pass http://backend/foo; + } + + location /capture { + server_tokens off; + more_clear_headers Date; + + content_by_lua_block { + local res = ngx.location.capture("/proxy") + ngx.print(res.body) + } + } + + location /t { + content_by_lua_block { + local req = [[ +GET /capture HTTP/1.1 +Host: test.com +Content-Length: 37 +Transfer-Encoding: chunked + +0 + +GET /capture HTTP/1.1 +Host: test.com +X: GET /bar HTTP/1.0 + +]] + + local sock = ngx.socket.tcp() + sock:settimeout(1000) + + local ok, err = sock:connect("127.0.0.1", $TEST_NGINX_SERVER_PORT) + if not ok then + ngx.say("failed to connect: ", err) + return + end + + local bytes, err = sock:send(req) + if not bytes then + ngx.say("failed to send req: ", err) + return + end + + ngx.say("req bytes: ", bytes) + + local n_resp = 0 + + local reader = sock:receiveuntil("\r\n") + while true do + local line, err = reader() + if line then + ngx.say(line) + if line == "0" then + n_resp = n_resp + 1 + end + + if n_resp >= 2 then + break + end + + else + ngx.say("err: ", err) + break + end + end + + sock:close() + } + } +--- request +GET /t +--- response_body +req bytes: 146 +HTTP/1.1 200 OK +Server: nginx +Content-Type: text/plain +Transfer-Encoding: chunked +Connection: keep-alive + +1f +method: GET, uri: /foo, X: nil + +0 + +HTTP/1.1 200 OK +Server: nginx +Content-Type: text/plain +Transfer-Encoding: chunked +Connection: keep-alive + +2d +method: GET, uri: /foo, X: GET /bar HTTP/1.0 + +0 +--- no_error_log +[error] + + + +=== TEST 78: avoid request smuggling 2/4 (POST capture + smuggle in body) +--- http_config + upstream backend { + server unix:$TEST_NGINX_HTML_DIR/nginx.sock; + keepalive 32; + } + + server { + listen unix:$TEST_NGINX_HTML_DIR/nginx.sock; + + location / { + content_by_lua_block { + ngx.say("method: ", ngx.var.request_method, + ", uri: ", ngx.var.uri) + } + } + } +--- config + location /proxy { + proxy_http_version 1.1; + proxy_set_header Connection ""; + proxy_pass http://backend/foo; + } + + location /capture { + server_tokens off; + more_clear_headers Date; + + content_by_lua_block { + ngx.req.read_body() + local res = ngx.location.capture("/proxy", { method = ngx.HTTP_POST }) + ngx.print(res.body) + } + } + + location /t { + content_by_lua_block { + local req = [[ +GET /capture HTTP/1.1 +Host: test.com +Content-Length: 57 +Transfer-Encoding: chunked + +0 + +POST /capture HTTP/1.1 +Host: test.com +Content-Length: 60 + +POST /bar HTTP/1.1 +Host: test.com +Content-Length: 5 + +hello + +]] + + local sock = ngx.socket.tcp() + sock:settimeout(1000) + + local ok, err = sock:connect("127.0.0.1", $TEST_NGINX_SERVER_PORT) + if not ok then + ngx.say("failed to connect: ", err) + return + end + + local bytes, err = sock:send(req) + if not bytes then + ngx.say("failed to send req: ", err) + return + end + + ngx.say("req bytes: ", bytes) + + local n_resp = 0 + + local reader = sock:receiveuntil("\r\n") + while true do + local line, err = reader() + if line then + ngx.say(line) + if line == "0" then + n_resp = n_resp + 1 + end + + if n_resp >= 2 then + break + end + + else + ngx.say("err: ", err) + break + end + end + + sock:close() + } + } +--- request +GET /t +--- response_body +req bytes: 205 +HTTP/1.1 200 OK +Server: nginx +Content-Type: text/plain +Transfer-Encoding: chunked +Connection: keep-alive + +18 +method: POST, uri: /foo + +0 + +HTTP/1.1 200 OK +Server: nginx +Content-Type: text/plain +Transfer-Encoding: chunked +Connection: keep-alive + +18 +method: POST, uri: /foo + +0 +--- no_error_log +[error] + + + +=== TEST 79: avoid request smuggling 3/4 (POST capture w/ always_forward_body + smuggle in body) +--- http_config + upstream backend { + server unix:$TEST_NGINX_HTML_DIR/nginx.sock; + keepalive 32; + } + + server { + listen unix:$TEST_NGINX_HTML_DIR/nginx.sock; + + location / { + content_by_lua_block { + ngx.say("method: ", ngx.var.request_method, + ", uri: ", ngx.var.uri) + } + } + } +--- config + location /proxy { + proxy_http_version 1.1; + proxy_set_header Connection ""; + proxy_pass http://backend/foo; + } + + location /capture { + server_tokens off; + more_clear_headers Date; + + content_by_lua_block { + ngx.req.read_body() + local res = ngx.location.capture("/proxy", { + method = ngx.HTTP_POST, + always_forward_body = true + }) + ngx.print(res.body) + } + } + + location /t { + content_by_lua_block { + local req = [[ +GET /capture HTTP/1.1 +Host: test.com +Content-Length: 57 +Transfer-Encoding: chunked + +0 + +POST /capture HTTP/1.1 +Host: test.com +Content-Length: 60 + +POST /bar HTTP/1.1 +Host: test.com +Content-Length: 5 + +hello + +]] + + local sock = ngx.socket.tcp() + sock:settimeout(1000) + + local ok, err = sock:connect("127.0.0.1", $TEST_NGINX_SERVER_PORT) + if not ok then + ngx.say("failed to connect: ", err) + return + end + + local bytes, err = sock:send(req) + if not bytes then + ngx.say("failed to send req: ", err) + return + end + + ngx.say("req bytes: ", bytes) + + local n_resp = 0 + + local reader = sock:receiveuntil("\r\n") + while true do + local line, err = reader() + if line then + ngx.say(line) + if line == "0" then + n_resp = n_resp + 1 + end + + if n_resp >= 2 then + break + end + + else + ngx.say("err: ", err) + break + end + end + + sock:close() + } + } +--- request +GET /t +--- response_body +req bytes: 205 +HTTP/1.1 200 OK +Server: nginx +Content-Type: text/plain +Transfer-Encoding: chunked +Connection: keep-alive + +18 +method: POST, uri: /foo + +0 + +HTTP/1.1 200 OK +Server: nginx +Content-Type: text/plain +Transfer-Encoding: chunked +Connection: keep-alive + +18 +method: POST, uri: /foo + +0 +--- no_error_log +[error] + + + +=== TEST 80: avoid request smuggling 4/4 (POST capture w/ body + smuggle in body) +--- http_config + upstream backend { + server unix:$TEST_NGINX_HTML_DIR/nginx.sock; + keepalive 32; + } + + server { + listen unix:$TEST_NGINX_HTML_DIR/nginx.sock; + + location / { + content_by_lua_block { + ngx.say("method: ", ngx.var.request_method, + ", uri: ", ngx.var.uri) + } + } + } +--- config + location /proxy { + proxy_http_version 1.1; + proxy_set_header Connection ""; + proxy_pass http://backend/foo; + } + + location /capture { + server_tokens off; + more_clear_headers Date; + + content_by_lua_block { + ngx.req.read_body() + local res = ngx.location.capture("/proxy", { + method = ngx.HTTP_POST, + always_forward_body = true, + body = ngx.req.get_body_data() + }) + ngx.print(res.body) + } + } + + location /t { + content_by_lua_block { + local req = [[ +GET /capture HTTP/1.1 +Host: test.com +Content-Length: 57 +Transfer-Encoding: chunked + +0 + +POST /capture HTTP/1.1 +Host: test.com +Content-Length: 60 + +POST /bar HTTP/1.1 +Host: test.com +Content-Length: 5 + +hello + +]] + + local sock = ngx.socket.tcp() + sock:settimeout(1000) + + local ok, err = sock:connect("127.0.0.1", $TEST_NGINX_SERVER_PORT) + if not ok then + ngx.say("failed to connect: ", err) + return + end + + local bytes, err = sock:send(req) + if not bytes then + ngx.say("failed to send req: ", err) + return + end + + ngx.say("req bytes: ", bytes) + + local n_resp = 0 + + local reader = sock:receiveuntil("\r\n") + while true do + local line, err = reader() + if line then + ngx.say(line) + if line == "0" then + n_resp = n_resp + 1 + end + + if n_resp >= 2 then + break + end + + else + ngx.say("err: ", err) + break + end + end + + sock:close() + } + } +--- request +GET /t +--- response_body +req bytes: 205 +HTTP/1.1 200 OK +Server: nginx +Content-Type: text/plain +Transfer-Encoding: chunked +Connection: keep-alive + +18 +method: POST, uri: /foo + +0 + +HTTP/1.1 200 OK +Server: nginx +Content-Type: text/plain +Transfer-Encoding: chunked +Connection: keep-alive + +18 +method: POST, uri: /foo + +0 +--- no_error_log +[error]