[PATCH v2] Make ngx_http_parse_unsafe_uri() to be able to unescape uri

Ruslan Ermilov ru at nginx.com
Fri Dec 20 16:26:59 UTC 2013


On Wed, Dec 18, 2013 at 07:26:26PM +0400, Maxim Dounin wrote:
> On Wed, Dec 18, 2013 at 06:09:44AM +0300, Raman Shishniou wrote:
> > Is something wrong with this patch?
> 
> Previous statement that it looks overcomplicated still applies.
> 
> [...]

We've started internal testing of the attached patch series.
You can help by testing and optionally reviewing it.

Note it doesn't attempt to address the "../blabla" issue, and
other similar issues.  It's unclear at this point what should
be the right fix.  It's clear to me that the "/blabla/.." case
should be considered unsafe.  But as Maxim noted privately, we
should probably consider all URIs not starting from "/" to be
unsafe.
-------------- next part --------------
# HG changeset patch
# User Ruslan Ermilov <ru at nginx.com>
# Date 1387556000 -14400
#      Fri Dec 20 20:13:20 2013 +0400
# Node ID ef3ae992a4e34cbf725a570529d7e5c69d42afe1
# Parent  d448241a2658ba3613719844f25f9666c50ffca5
Upstream: keep $upstream_http_x_accel_redirect intact.

When processing the X-Accel-Redirect header, the value of the
$upstream_http_x_accel_redirect variable was also overwritten.

diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c
+++ b/src/http/ngx_http_upstream.c
@@ -1994,7 +1994,7 @@ ngx_http_upstream_test_connect(ngx_conne
 static ngx_int_t
 ngx_http_upstream_process_headers(ngx_http_request_t *r, ngx_http_upstream_t *u)
 {
-    ngx_str_t                      *uri, args;
+    ngx_str_t                       uri, args;
     ngx_uint_t                      i, flags;
     ngx_list_part_t                *part;
     ngx_table_elt_t                *h;
@@ -2035,11 +2035,11 @@ ngx_http_upstream_process_headers(ngx_ht
             }
         }
 
-        uri = &u->headers_in.x_accel_redirect->value;
+        uri = u->headers_in.x_accel_redirect->value;
         ngx_str_null(&args);
         flags = NGX_HTTP_LOG_UNSAFE;
 
-        if (ngx_http_parse_unsafe_uri(r, uri, &args, &flags) != NGX_OK) {
+        if (ngx_http_parse_unsafe_uri(r, &uri, &args, &flags) != NGX_OK) {
             ngx_http_finalize_request(r, NGX_HTTP_NOT_FOUND);
             return NGX_DONE;
         }
@@ -2048,7 +2048,7 @@ ngx_http_upstream_process_headers(ngx_ht
             r->method = NGX_HTTP_GET;
         }
 
-        ngx_http_internal_redirect(r, uri, &args);
+        ngx_http_internal_redirect(r, &uri, &args);
         ngx_http_finalize_request(r, NGX_DONE);
         return NGX_DONE;
     }
# HG changeset patch
# User Ruslan Ermilov <ru at nginx.com>
# Date 1387556039 -14400
#      Fri Dec 20 20:13:59 2013 +0400
# Node ID d177ae8bbe6fc1164544f5cdf9a02d68611144fa
# Parent  ef3ae992a4e34cbf725a570529d7e5c69d42afe1
Fixed an off-by-one bug in ngx_http_parse_unsafe_uri().

diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c
--- a/src/http/ngx_http_parse.c
+++ b/src/http/ngx_http_parse.c
@@ -1814,7 +1814,7 @@ ngx_http_parse_unsafe_uri(ngx_http_reque
             goto unsafe;
         }
 
-        if (ngx_path_separator(ch) && len > 2) {
+        if (ngx_path_separator(ch) && len >= 4) {
 
             /* detect "/../" */
 
# HG changeset patch
# User Ruslan Ermilov <ru at nginx.com>
# Date 1387556050 -14400
#      Fri Dec 20 20:14:10 2013 +0400
# Node ID e4908c30a324934e7409c4a8e2285974937f2a4f
# Parent  d177ae8bbe6fc1164544f5cdf9a02d68611144fa
Teach ngx_http_parse_unsafe_uri() how to unescape URIs.

This fixes handling of escaped URIs in X-Accel-Redirect (ticket #316),
SSI (ticket #240), and DAV.

diff --git a/src/http/modules/ngx_http_ssi_filter_module.c b/src/http/modules/ngx_http_ssi_filter_module.c
--- a/src/http/modules/ngx_http_ssi_filter_module.c
+++ b/src/http/modules/ngx_http_ssi_filter_module.c
@@ -1982,8 +1982,6 @@ static ngx_int_t
 ngx_http_ssi_include(ngx_http_request_t *r, ngx_http_ssi_ctx_t *ctx,
     ngx_str_t **params)
 {
-    u_char                      *dst, *src;
-    size_t                       len;
     ngx_int_t                    rc, key;
     ngx_str_t                   *uri, *file, *wait, *set, *stub, args;
     ngx_buf_t                   *b;
@@ -2054,18 +2052,6 @@ ngx_http_ssi_include(ngx_http_request_t 
         return rc;
     }
 
-    dst = uri->data;
-    src = uri->data;
-
-    ngx_unescape_uri(&dst, &src, uri->len, NGX_UNESCAPE_URI);
-
-    len = (uri->data + uri->len) - src;
-    if (len) {
-        dst = ngx_movemem(dst, src, len);
-    }
-
-    uri->len = dst - uri->data;
-
     ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
                    "ssi include: \"%V\"", uri);
 
diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c
--- a/src/http/ngx_http_parse.c
+++ b/src/http/ngx_http_parse.c
@@ -1780,11 +1780,13 @@ ngx_int_t
 ngx_http_parse_unsafe_uri(ngx_http_request_t *r, ngx_str_t *uri,
     ngx_str_t *args, ngx_uint_t *flags)
 {
-    u_char  ch, *p;
-    size_t  len;
+    u_char      ch, *p, *src, *dst;
+    size_t      len;
+    ngx_uint_t  escaped;
 
     len = uri->len;
     p = uri->data;
+    escaped = 0;
 
     if (len == 0 || p[0] == '?') {
         goto unsafe;
@@ -1798,6 +1800,11 @@ ngx_http_parse_unsafe_uri(ngx_http_reque
 
         ch = *p++;
 
+        if (ch == '%') {
+            escaped = 1;
+            continue;
+        }
+
         if (usual[ch >> 5] & (1 << (ch & 0x1f))) {
             continue;
         }
@@ -1807,7 +1814,7 @@ ngx_http_parse_unsafe_uri(ngx_http_reque
             args->data = p;
             uri->len -= len;
 
-            return NGX_OK;
+            break;
         }
 
         if (ch == '\0') {
@@ -1824,6 +1831,53 @@ ngx_http_parse_unsafe_uri(ngx_http_reque
         }
     }
 
+    if (escaped) {
+        ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
+                       "escaped URI: \"%V\"", uri);
+
+        src = uri->data;
+
+        dst = ngx_pnalloc(r->pool, uri->len);
+        if (dst == NULL) {
+            return NGX_ERROR;
+	}
+
+        uri->data = dst;
+
+        ngx_unescape_uri(&dst, &src, uri->len, 0);
+
+        uri->len = dst - uri->data;
+
+        ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
+                       "unescaped URI: \"%V\"", uri);
+
+        len = uri->len;
+        p = uri->data;
+
+        if (p[0] == '.' && len == 3 && p[1] == '.' && ngx_path_separator(p[2]))
+        {
+            goto unsafe;
+        }
+
+        for ( /* void */ ; len; len--) {
+
+            ch = *p++;
+
+            if (ch == '\0') {
+                goto unsafe;
+            }
+
+            if (ngx_path_separator(ch) && len >= 4) {
+
+                /* detect "/../" */
+
+                if (p[0] == '.' && p[1] == '.' && ngx_path_separator(p[2])) {
+                    goto unsafe;
+                }
+            }
+        }
+    }
+
     return NGX_OK;
 
 unsafe:
# HG changeset patch
# User Ruslan Ermilov <ru at nginx.com>
# Date 1387556093 -14400
#      Fri Dec 20 20:14:53 2013 +0400
# Node ID 0cf4215c164f429c9893014fc3dfbccde1120bbb
# Parent  e4908c30a324934e7409c4a8e2285974937f2a4f
Dav: emit a warning about unsafe URI.

diff --git a/src/http/modules/ngx_http_dav_module.c b/src/http/modules/ngx_http_dav_module.c
--- a/src/http/modules/ngx_http_dav_module.c
+++ b/src/http/modules/ngx_http_dav_module.c
@@ -604,7 +604,7 @@ destination_done:
 
     duri.len = last - p;
     duri.data = p;
-    flags = 0;
+    flags = NGX_HTTP_LOG_UNSAFE;
 
     if (ngx_http_parse_unsafe_uri(r, &duri, &args, &flags) != NGX_OK) {
         goto invalid_destination;


More information about the nginx-devel mailing list