Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#162 closed defect (fixed)

buffer overflow under a particular rewrite configuration

Reported by: fan liu Owned by: somebody
Priority: major Milestone:
Component: nginx-core Version: 1.2.x
Keywords: rewrite coredump Cc:
uname -a: Linux liufan 2.6.35.6-45.fc14.i686 #1 SMP Mon Oct 18 23:56:17 UTC 2010 i686 i686 i386 GNU/Linux
nginx -V: nginx version: nginx/1.2.0
built by gcc 4.5.1 20100924 (Red Hat 4.5.1-4) (GCC)
configure arguments:

Description

nginx conf:
location / {

rewrite (/.*)$ $1?ip=$remote_addr&fetch=baidu? last;

}

Surely, this config leads to url handling loop. I config in this way just in order to make the coredump appear easier.

The request url is:
/r/www/cache/g%E6%A0%A1%E8%BF%90%E5%8A%A8%E5%9C%BA%E4%B8%8A%E6%95%A3%E6%AD%A5%E3%80%82%E4%BB%96%E4%BB%8E%E6%A5%BC%E4%B8%8A%E8%B5%B0%E4%BA%86%E4%B8%8B%E6%9D%A5%EF%BC%8C%E7%9C%8B%E5%88%B0%E6%88%91%E7%A9%BF%E7%9A%84...www8090kkwww8090kk%E3%80%90%3Cem%3Ewww8090kk%3C/em%3E%E3%80%91_%3Cem%3Ewww8090kk%3C/em%3E%E2%80%BB%E9%AB%98%E6%B8%85%E2%80%BBwww8090kkwww8090kk%20=======...%3Cbr%3E%3Cspan%20class=

As you see, there are many '%', that's what leads to the coredump.

The coredump happens in this way:

When nginx handles rewrite declaration, it first estimates buffer length the destination needs. As to
this statement, it calls:
ngx_http_script_copy_capture_len_code
ngx_http_script_mark_args_code
ngx_http_script_copy_len_code
...

Then, nginx starts to translate the url, it calls:
ngx_http_script_copy_capture_code
ngx_http_script_copy_code
...

In function ngx_http_script_mark_args_code, e->is_args is set to 1.
As a result, when estimate dest buffer in function ngx_http_script_copy_capture_len_code,
ngx_escape_uri is not called, but when translate the url in function ngx_http_script_copy_capture_code, ngx_escape_uri is called.

That's the problem.

ngx_escape_uri makes '%' be THREE characters, while we only allocates ONE byte buffer space. So, coredump.

Change History (4)

comment:1 by Maxim Dounin, 9 years ago

Thank you for report. The following patch should fix this:

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1336042504 -14400
# Node ID cdf28c793849c77b35e384a66bb8ce2755497401
# Parent  41c1eac9cc2468753f5cb1f1f0608ab09b11bb56
Rewrite: fixed escaping and possible segfault (ticket #162).

The following code resulted in incorrect escaping of uri and possible
segfault:

    location / {
        rewrite ^(.*) $1?c=$1;
        return 200 "$uri";
    }

If there were arguments in a rewrite's replacement string, and length was
actually calculated (due to duplicate captures as in the example above,
or variables present), the is_args flag was set and incorrectly copied
after length calculation.  This resulted in escaping applied to the uri part
of the replacement, resulting in incorrect escaping.  Additionally, buffer
was allocated without escaping expected, thus this also resulted in buffer
overrun and possible segfault.

diff --git a/src/http/ngx_http_script.c b/src/http/ngx_http_script.c
--- a/src/http/ngx_http_script.c
+++ b/src/http/ngx_http_script.c
@@ -1043,7 +1043,6 @@ ngx_http_script_regex_start_code(ngx_htt
         }
 
         e->buf.len = len;
-        e->is_args = le.is_args;
     }
 
     if (code->add_args && r->args.len) {

Tests added here: http://mdounin.ru/hg/nginx-tests/rev/7f5095965c88

comment:2 by Maxim Dounin, 9 years ago

In [4618/nginx]:

(The changeset message doesn't reference this ticket)

comment:3 by Maxim Dounin, 9 years ago

Resolution: fixed
Status: newclosed

Fix committed.

comment:4 by Maxim Dounin, 9 years ago

In [4665/nginx]:

(The changeset message doesn't reference this ticket)

Note: See TracTickets for help on using tickets.