Hello!

On Thu, Feb 02, 2012 at 03:27:27AM +0400, vitalif@yourcmc.ru wrote:

> A new version of patch, unescaping in ngx_http_parse_unsafe_uri()...
> Note that it required removing if (ch == ?) blocks that come AFTER
> decoding the character (I don't understand their purpose, they seem
> incorrect, as escaped '?' does not mean the beginning of query
> string).
> And I still don't understand what NGX_UNESCAPE_REDIRECT is... :-)

> This is a patch for nginx 1.1.12 handling of X-Accel-Redirect URLs:
> allow to handle percent-encoded URLs and complex filenames in X-Accel-Redirect
> (for example, filenames which contain '?').
>
> --- a/src/core/ngx_string.c 2011-11-25 20:36:02.000000000 +0400
> +++ b/src/core/ngx_string.c 2012-02-02 03:21:46.153704057 +0400
> @@ -1590,21 +1590,11 @@ ngx_unescape_uri(u_char **dst, u_char **
> ch = (u_char) ((decoded << 4) + c - 'a' + 10);
>
> if (type & NGX_UNESCAPE_URI) {
> - if (ch == '?') {
> - *d++ = ch;
> - goto done;
> - }
> -

This looks like a correct change.

> *d++ = ch;
> break;
> }
>
> if (type & NGX_UNESCAPE_REDIRECT) {
> - if (ch == '?') {
> - *d++ = ch;
> - goto done;
> - }
> -

This is not correct, as this will break current behaviour of
redirect unescaping. Unless you are willing to work on it in
general, please don't touch it. In any case this should be a
separate patch.

> if (ch > '%' && ch < 0x7f) {
> *d++ = ch;
> break;
> --- a/src/http/ngx_http_parse.c 2011-11-28 13:15:33.000000000 +0400
> +++ b/src/http/ngx_http_parse.c 2012-02-02 03:18:42.561709398 +0400
> @@ -1591,7 +1591,7 @@ 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;
> + u_char ch, *p, *src, *dst;
> size_t len;
>
> len = uri->len;
> @@ -1605,6 +1605,23 @@ ngx_http_parse_unsafe_uri(ngx_http_reque
> goto unsafe;
> }
>
> + /* unescape URI */
> +
> + if (ngx_strchr(p, '%')) {

This isn't a good idea to assume uri is null-terminated.

> +
> + dst = uri->data;
> + src = uri->data;
> +
> + ngx_unescape_uri(&dst, &src, uri->len, NGX_UNESCAPE_URI);

And it may not be good idea to assume uri->data is writable.

> +
> + len = (uri->data + uri->len) - src;
> + if (len) {
> + dst = ngx_copy(dst, src, len);
> + }
> +
> + uri->len = dst - uri->data;
> + }
> +
> for ( /* void */ ; len; len--) {
>
> ch = *p++;

This also needs changes at least in ssi module, which currently
does unescaping itself. The dav module needs some attention too,
as this change will affect it and it should be made clear it
doesn't break things.

Maxim Dounin

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel