From 1a67af93acca669e130dbafab91fa8d253705476 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Fri, 7 Mar 2025 02:06:01 -0500 Subject: [PATCH 1/2] HTTP: Avoid out-of-bounds pointer arithmetic parsing URLs Creating a pointer before the start of an array is undefined behavior according to the C standard, so don't do that. This actually makes the code simpler too. --- src/http/ngx_http_parse.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index a45c04554..22afac00f 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -1295,7 +1295,7 @@ ngx_http_parse_complex_uri(ngx_http_request_t *r, ngx_uint_t merge_slashes) switch (ch) { #if (NGX_WIN32) case '\\': - if (u - 2 >= r->uri.data + if (u - r->uri.data >= 2 && *(u - 1) == '.' && *(u - 2) != '.') { u--; @@ -1319,7 +1319,7 @@ ngx_http_parse_complex_uri(ngx_http_request_t *r, ngx_uint_t merge_slashes) #endif case '/': #if (NGX_WIN32) - if (u - 2 >= r->uri.data + if (u - r->uri.data >= 2 && *(u - 1) == '.' && *(u - 2) != '.') { u--; @@ -1457,13 +1457,12 @@ ngx_http_parse_complex_uri(ngx_http_request_t *r, ngx_uint_t merge_slashes) case '/': case '?': case '#': - u -= 4; + u -= 3; for ( ;; ) { - if (u < r->uri.data) { + if (u <= r->uri.data) { return NGX_HTTP_PARSE_INVALID_REQUEST; } - if (*u == '/') { - u++; + if (u[-1] == '/') { break; } u--; @@ -1561,15 +1560,14 @@ ngx_http_parse_complex_uri(ngx_http_request_t *r, ngx_uint_t merge_slashes) u--; } else if (state == sw_dot_dot) { - u -= 4; + u -= 3; for ( ;; ) { - if (u < r->uri.data) { + if (u <= r->uri.data) { return NGX_HTTP_PARSE_INVALID_REQUEST; } - if (*u == '/') { - u++; + if (u[-1] == '/') { break; } From 0ae6ece357dc66481555c3b84e828b7fe341407a Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 10 Mar 2025 18:59:12 -0400 Subject: [PATCH 2/2] SECURITY FIX: Proxy: escape ';' in URI paths This fixes a security hole when used with many Java application servers, which treat ';' as a special character in paths. See [1] for more details. This changes how Nginx constructs requests for its backends. Something like: location /fake1/ { proxy_pass http://127.0.0.1:8000/fake2/; } will now escape the semicolon. This is ultimately a flaw in Nginx, not the backend servers: it is a client error to make an HTTP request with a URI that contains a path with a character that is not valid in paths, and as the client Nginx is responsible for only making valid requests. This is not a complete fix. A complete fix would require blocking %2F in paths (HAProxy and NGINX disagree on its meaning) and blocking unescaped semicolons in paths (NGINX and Java app servers disagree on their meaning). This does fix the case where proxy_pass is used with a URI that does not contain any variables. [1]: https://i.blackhat.com/us-18/Wed-August-8/us-18-Orange-Tsai-Breaking-Parser-Logic-Take-Your-Path-Normalization-Off-And-Pop-0days-Out-2.pdf --- src/core/ngx_string.c | 22 +++++++++++++++++++++- src/core/ngx_string.h | 1 + src/http/modules/ngx_http_proxy_module.c | 18 ++++++------------ 3 files changed, 28 insertions(+), 13 deletions(-) diff --git a/src/core/ngx_string.c b/src/core/ngx_string.c index f8f738472..019de4343 100644 --- a/src/core/ngx_string.c +++ b/src/core/ngx_string.c @@ -1565,6 +1565,26 @@ ngx_escape_uri(u_char *dst, u_char *src, size_t size, ngx_uint_t type) /* ~}| {zyx wvut srqp onml kjih gfed cba` */ 0xb8000001, /* 1011 1000 0000 0000 0000 0000 0000 0001 */ + 0xffffffff, /* 1111 1111 1111 1111 1111 1111 1111 1111 */ + 0xffffffff, /* 1111 1111 1111 1111 1111 1111 1111 1111 */ + 0xffffffff, /* 1111 1111 1111 1111 1111 1111 1111 1111 */ + 0xffffffff /* 1111 1111 1111 1111 1111 1111 1111 1111 */ + }; + + /* " ", "#", "%", "?", not allowed */ + + static uint32_t uri_path[] = { + 0xffffffff, /* 1111 1111 1111 1111 1111 1111 1111 1111 */ + + /* ?>=< ;:98 7654 3210 /.-, +*)( '&%$ #"! */ + 0xd800002d, /* 1101 1000 0000 0000 0000 0000 0010 1101 */ + + /* _^]\ [ZYX WVUT SRQP ONML KJIH GFED CBA@ */ + 0x50000000, /* 0101 0000 0000 0000 0000 0000 0000 0000 */ + + /* ~}| {zyx wvut srqp onml kjih gfed cba` */ + 0xb8000001, /* 1011 1000 0000 0000 0000 0000 0000 0001 */ + 0xffffffff, /* 1111 1111 1111 1111 1111 1111 1111 1111 */ 0xffffffff, /* 1111 1111 1111 1111 1111 1111 1111 1111 */ 0xffffffff, /* 1111 1111 1111 1111 1111 1111 1111 1111 */ @@ -1634,7 +1654,7 @@ ngx_escape_uri(u_char *dst, u_char *src, size_t size, ngx_uint_t type) /* mail_auth is the same as memcached */ static uint32_t *map[] = - { uri, args, uri_component, html, refresh, memcached, memcached }; + { uri, args, uri_component, html, refresh, memcached, memcached, uri_path }; escape = map[type]; diff --git a/src/core/ngx_string.h b/src/core/ngx_string.h index 713eb42a7..0183ed71a 100644 --- a/src/core/ngx_string.h +++ b/src/core/ngx_string.h @@ -203,6 +203,7 @@ u_char *ngx_utf8_cpystrn(u_char *dst, u_char *src, size_t n, size_t len); #define NGX_ESCAPE_REFRESH 4 #define NGX_ESCAPE_MEMCACHED 5 #define NGX_ESCAPE_MAIL_AUTH 6 +#define NGX_ESCAPE_URI_PATH 7 #define NGX_UNESCAPE_URI 1 #define NGX_UNESCAPE_REDIRECT 2 diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c index 27c34fef2..f5bc6a9c3 100644 --- a/src/http/modules/ngx_http_proxy_module.c +++ b/src/http/modules/ngx_http_proxy_module.c @@ -1208,12 +1208,8 @@ ngx_http_proxy_create_key(ngx_http_request_t *r) loc_len = (r->valid_location && ctx->vars.uri.len) ? plcf->location.len : 0; - if (r->quoted_uri || r->internal) { - escape = 2 * ngx_escape_uri(NULL, r->uri.data + loc_len, - r->uri.len - loc_len, NGX_ESCAPE_URI); - } else { - escape = 0; - } + escape = 2 * ngx_escape_uri(NULL, r->uri.data + loc_len, + r->uri.len - loc_len, NGX_ESCAPE_URI_PATH); len = ctx->vars.uri.len + r->uri.len - loc_len + escape + sizeof("?") - 1 + r->args.len; @@ -1231,7 +1227,7 @@ ngx_http_proxy_create_key(ngx_http_request_t *r) if (escape) { ngx_escape_uri(p, r->uri.data + loc_len, - r->uri.len - loc_len, NGX_ESCAPE_URI); + r->uri.len - loc_len, NGX_ESCAPE_URI_PATH); p += r->uri.len - loc_len + escape; } else { @@ -1321,10 +1317,8 @@ ngx_http_proxy_create_request(ngx_http_request_t *r) loc_len = (r->valid_location && ctx->vars.uri.len) ? plcf->location.len : 0; - if (r->quoted_uri || r->internal) { - escape = 2 * ngx_escape_uri(NULL, r->uri.data + loc_len, - r->uri.len - loc_len, NGX_ESCAPE_URI); - } + escape = 2 * ngx_escape_uri(NULL, r->uri.data + loc_len, + r->uri.len - loc_len, NGX_ESCAPE_URI_PATH); uri_len = ctx->vars.uri.len + r->uri.len - loc_len + escape + sizeof("?") - 1 + r->args.len; @@ -1448,7 +1442,7 @@ ngx_http_proxy_create_request(ngx_http_request_t *r) if (escape) { ngx_escape_uri(b->last, r->uri.data + loc_len, - r->uri.len - loc_len, NGX_ESCAPE_URI); + r->uri.len - loc_len, NGX_ESCAPE_URI_PATH); b->last += r->uri.len - loc_len + escape; } else {