Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#768 closed defect (fixed)

String garbling when using nested regex locations

Reported by: launchpad.net/~blubberdiblub Owned by:
Priority: major Milestone:
Component: nginx-core Version: 1.8.x
Keywords: try_files alias Cc: niels.boehm@…
uname -a: Linux lwlvlin32 3.13.0-49-generic #83-Ubuntu SMP Fri Apr 10 20:11:33 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux
nginx -V: nginx version: nginx/1.8.0
built with OpenSSL 1.0.1f 6 Jan 2014
TLS SNI support enabled
configure arguments: --with-cc-opt='-g -O2 -fPIE -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D_FORTIFY_SOURCE=2' --with-ld-opt='-Wl,-Bsymbolic-functions -fPIE -pie -Wl,-z,relro -Wl,-z,now' --prefix=/usr/share/nginx --conf-path=/etc/nginx/nginx.conf --http-log-path=/var/log/nginx/access.log --error-log-path=/var/log/nginx/error.log --lock-path=/var/lock/nginx.lock --pid-path=/run/nginx.pid --http-client-body-temp-path=/var/lib/nginx/body --http-fastcgi-temp-path=/var/lib/nginx/fastcgi --http-proxy-temp-path=/var/lib/nginx/proxy --http-scgi-temp-path=/var/lib/nginx/scgi --http-uwsgi-temp-path=/var/lib/nginx/uwsgi --with-debug --with-pcre-jit --with-ipv6 --with-http_ssl_module --with-http_stub_status_module --with-http_realip_module --with-http_auth_request_module --with-http_gzip_static_module --without-http_browser_module --without-http_geo_module --without-http_limit_req_module --without-http_limit_zone_module --without-http_memcached_module --without-http_referer_module --without-http_scgi_module --without-http_split_clients_module --without-http_ssi_module --without-http_userid_module --without-http_uwsgi_module --add-module=/build/buildd/nginx-1.8.0/debian/modules/nginx-echo

Description

In certain circumstances with certain configurations (I'm not sure about the exact conditions), the string passed as SCRIPT_NAME to phm-fpm gets garbled (it may have been garbled earlier, tho, for instance during fastcgi_split_path_info or during assignment to the fastcgi_param).

We used to have this problem with an earlier version, too (1.4.1 or 1.6.3, I don't remember), in a different config file, which we rewrote (and which is now uglier).

Now it showed up in this config file which I present an excerpt of here when we upgraded from 1.6.3 to 1.8.0.

In the fastcgi hexdump note the variable SCRIPT_NAME, which gets a value of ^/pm/recommended/index.php assigned by nginx. However, the expected value is /recommended/index.php. It appears as if nginx mixed one of the regexes with the path to be parsed.

The request for which that happened was GET /pma/recommended/ HTTP/1.0.

Attachments (2)

nginx-config-excerpt.conf (601 bytes ) - added by launchpad.net/~blubberdiblub 9 years ago.
fastcgi-hexdump.txt (5.2 KB ) - added by launchpad.net/~blubberdiblub 9 years ago.
hexdump of what nginx sent to php-fpm

Download all attachments as: .zip

Change History (7)

by launchpad.net/~blubberdiblub, 9 years ago

Attachment: nginx-config-excerpt.conf added

by launchpad.net/~blubberdiblub, 9 years ago

Attachment: fastcgi-hexdump.txt added

hexdump of what nginx sent to php-fpm

comment:1 by Maxim Dounin, 9 years ago

Keywords: try_files alias added
Status: newaccepted

Simplified configuration to trigger this:

location /pma {
    alias /opt/dezem/var/www/sites/pma/htdocs;

    location ~ ^/pma/.*/$ {
        try_files /recommended/index.php =405;
    }
}

Similar configuration is already in ticket #97:

# bug: request "/foo/test.gif" will try "/tmp//foo/test.gif"
location /foo/ {
    alias /tmp/;
    location ~ gif {
        try_files $uri =405;
    }
}

It tests a wrong file in try_files (in your case, you compensate it by a regex in fastcgi_split_path_info). Additionally, if a file is nevertheless found - it uses wrong URI after try_files. Wrong URI seems to be a regression introduced by c985d90a8d1f (nginx 1.7.1).

I'll take a look how to fix this properly.

comment:2 by Maxim Dounin, 9 years ago

Here is a patch which fixes the problem (both problems, actually: with incorrect file tried and the regression with wrong URI used). Testing is appreciated.

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1438038209 -10800
#      Tue Jul 28 02:03:29 2015 +0300
# Node ID ab2f7ea48b3accd880163f7785185a9bd44e9a29
# Parent  701a1596905f5a07c6b487e46a4da4ace7656d5c
Fixed wrong URI after try_files in nested location (ticket #97).

The following configuration with alias, nested location and try_files
resulted in wrong file being used.  Request "/foo/test.gif" tried to
use "/tmp//foo/test.gif" instead of "/tmp/test.gif":

    location /foo/ {
        alias /tmp/;
        location ~ gif {
            try_files $uri =405;
        }
    }

Additionally, rev. c985d90a8d1f introduced a regression if
the "/tmp//foo/test.gif" file was found (ticket #768).  Resulting URI
was set to "gif?/foo/test.gif", as the code used clcf->name of current
location ("location ~ gif") instead of parent one ("location /foo/").

Fix is to use r->uri instead of clcf->name in all cases in the
ngx_http_core_try_files_phase() function.  It is expected to be
already matched and identical to the clcf->name of the right
location.

diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c
--- a/src/http/ngx_http_core_module.c
+++ b/src/http/ngx_http_core_module.c
@@ -1240,7 +1240,7 @@ ngx_http_core_try_files_phase(ngx_http_r
             *e.pos = '\0';
 
             if (alias && alias != NGX_MAX_SIZE_T_VALUE
-                && ngx_strncmp(name, clcf->name.data, alias) == 0)
+                && ngx_strncmp(name, r->uri.data, alias) == 0)
             {
                 ngx_memmove(name, name + alias, len - alias);
                 path.len -= alias;
@@ -1324,6 +1324,8 @@ ngx_http_core_try_files_phase(ngx_http_r
             }
 
         } else {
+            name = r->uri.data;
+
             r->uri.len = alias + path.len;
             r->uri.data = ngx_pnalloc(r->pool, r->uri.len);
             if (r->uri.data == NULL) {
@@ -1331,8 +1333,8 @@ ngx_http_core_try_files_phase(ngx_http_r
                 return NGX_OK;
             }
 
-            p = ngx_copy(r->uri.data, clcf->name.data, alias);
-            ngx_memcpy(p, name, path.len);
+            p = ngx_copy(r->uri.data, name, alias);
+            ngx_memcpy(p, path.data, path.len);
         }
 
         ngx_http_set_exten(r);

comment:3 by Maxim Dounin <mdounin@…>, 9 years ago

In bd55d75a1410502a9b4ac6fb44ec4528437b530a/nginx:

Fixed wrong URI after try_files in nested location (ticket #97).

The following configuration with alias, nested location and try_files
resulted in wrong file being used. Request "/foo/test.gif" tried to
use "/tmpfoo/test.gif" instead of "/tmp/test.gif":

location /foo/ {

alias /tmp/;
location ~ gif {

try_files $uri =405;

}

}

Additionally, rev. c985d90a8d1f introduced a regression if
the "/tmpfoo/test.gif" file was found (ticket #768). Resulting URI
was set to "gif?/foo/test.gif", as the code used clcf->name of current
location ("location ~ gif") instead of parent one ("location /foo/").

Fix is to use r->uri instead of clcf->name in all cases in the
ngx_http_core_try_files_phase() function. It is expected to be
already matched and identical to the clcf->name of the right
location.

comment:4 by Maxim Dounin, 9 years ago

Resolution: fixed
Status: acceptedclosed

Fix committed, thank you.

comment:5 by Maxim Dounin <mdounin@…>, 9 years ago

In 6341:2797b4347a2a/nginx:

Fixed wrong URI after try_files in nested location (ticket #97).

The following configuration with alias, nested location and try_files
resulted in wrong file being used. Request "/foo/test.gif" tried to
use "/tmpfoo/test.gif" instead of "/tmp/test.gif":

location /foo/ {

alias /tmp/;
location ~ gif {

try_files $uri =405;

}

}

Additionally, rev. c985d90a8d1f introduced a regression if
the "/tmpfoo/test.gif" file was found (ticket #768). Resulting URI
was set to "gif?/foo/test.gif", as the code used clcf->name of current
location ("location ~ gif") instead of parent one ("location /foo/").

Fix is to use r->uri instead of clcf->name in all cases in the
ngx_http_core_try_files_phase() function. It is expected to be
already matched and identical to the clcf->name of the right
location.

Note: See TracTickets for help on using tickets.