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.
# HG changeset patch
# User Ruslan Ermilov <r...@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 <r...@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 <r...@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 <r...@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;
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
