[Bug] X-Accel-Redirect

[apeman]

Hi everyone,

I got a server which serves Data to a client ( in my case MP3s ). The
MP3s are saved in a storage directory in the form Artist - Title.mp3

I'm using X-Accel-Redirect to serve the files from the storage dir to
the user. This workes fine for most cases but fails for special chars
like the question mark. I already tried encoding it - but the char
always gets stripped like in Query strings. But since it's not an url
I'm providing no stripping should take place. Here are my results:


[b]Example Filename is:  /ext/53665/Katy Perry - Who Am I Living
For?.mp3[/b]

[b]Plain:[/b]
open() "/ext/53665/Katy Perry - Who Am I Living For" failed (2: No such
file or directory)

[b]urlencoded:[/b]
open()
"/usr/local/nginx/html%2Fdl%2F53665%2FKaty+Perry+-+Who+Am+I+Living+For%253F.mp3"
failed (2: No such file or directory)

[b]Rawurlencoded:[/b]
 open()
"/usr/local/nginx/html%2Fdl%2F53665%2FKaty%20Perry%20-%20Who%20Am%20I%20Living%20For%3F.mp3"
failed (2: No such file or directory)

[b]Replaced ? with \?[/b]
open() "/ext/53665/Katy Perry - Who Am I Living For\" failed (2: No such
file or directory)

[b]Escaped ? to %3F[/b]
open() "/ext/53665/Katy Perry - Who Am I Living For%3F.mp3" failed (2:
No such file or directory)

It seems like X-Accel-Redirect treats the path as url instead of a
realpath - or even doesn't apply any decoding on the subrequest.

best regards,

Volker

Posted at Nginx Forum: http://forum.nginx.org/read.php?2,128346,128346#msg-128346


_______________________________________________
nginx mailing list
[hidden email]
http://nginx.org/mailman/listinfo/nginx

[Maxim Dounin]

Hello!

On Wed, Sep 08, 2010 at 09:53:14AM -0400, rovervr wrote:

> Hi everyone,
>
> I got a server which serves Data to a client ( in my case MP3s ). The
> MP3s are saved in a storage directory in the form Artist - Title.mp3
>
> I'm using X-Accel-Redirect to serve the files from the storage dir to
> the user. This workes fine for most cases but fails for special chars
> like the question mark. I already tried encoding it - but the char
> always gets stripped like in Query strings. But since it's not an url
> I'm providing no stripping should take place. Here are my results:

X-Accel-Redirects expects URI, but unescaped one.  This was
last discussed yesterday:

http://nginx.org/pipermail/nginx/2010-September/022384.html

Quote:

  X-Accel-Redirect expected to contain non-encoded URI.  This isn't
  really right, but it's how it currently works.

In particular this makes impossible to (normally) serve resources
with '?' in name as you see in your tests, as anything after '?'
is treated as query string.

Correct fix would be to change X-Accel-Redirect to accept escaped
URI instead.  I believe patches are welcome.

Maxim Dounin

_______________________________________________
nginx mailing list
[hidden email]
http://nginx.org/mailman/listinfo/nginx

[apeman]

Hi,
I created a small patch for that issue which works for me. But it needs
to be reviewed by Igor or someone who knows C better than me.
It checks the static request from X-Accel-Redirect for '%' and escapes
them if found.


[code]
--- nginx-0.8.50orig/src/http/modules/ngx_http_static_module.c
2010-05-24 14:35:10.000000000 +0200
+++ nginx-0.8.50/src/http/modules/ngx_http_static_module.c    
2010-09-09 13:49:49.000000000 +0200
@@ -47,7 +47,7 @@
 static ngx_int_t
 ngx_http_static_handler(ngx_http_request_t *r)
 {
-    u_char                    *last, *location;
+    u_char                    *last, *location, *src, *dst;
     size_t                     root, len;
     ngx_str_t                  path;
     ngx_int_t                  rc;
@@ -83,6 +83,28 @@
     ngx_log_debug1(NGX_LOG_DEBUG_HTTP, log, 0,
                    "http filename: \"%s\"", path.data);

+    /*
+     * X-Accel-Redirect Patch
+     * If the path contains a % it probably must be decoded
+     */
+    if( strstr( path.data, "%" ) != NULL )
+    {
+        ngx_str_t *uri = &path;
+
+        dst = uri->data;
+        src = uri->data;
+
+        ngx_unescape_uri( &dst, &src, uri->len, NGX_UNESCAPE_URI );
+
+        len = uri->len - ( src - dst ) + 1;
+        if ( len )
+        {
+            dst = ngx_copy( dst, src, len);
+        }
+        uri->len = dst - uri->data;
+    }
+
+
     clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);

     ngx_memzero(&of, sizeof(ngx_open_file_info_t));

[/code]

It would be nice to get a little feedback if this is ok.

best regards,

Volker Richter

Posted at Nginx Forum: http://forum.nginx.org/read.php?2,128346,128745#msg-128745


_______________________________________________
nginx mailing list
[hidden email]
http://nginx.org/mailman/listinfo/nginx

[Dennis Jacobfeuerborn]

Hi,
you cannot use the presence of a character as an indicator to determine if
a string is encoded or not. After all "%" is a perfectly legitimate
character that can be used for a filename or directory.

The question is whether path.data represents the raw encoded version of the
path in which case it must *always* be decoded or if this variable already
represents the decoded version of the path in which case it should *never*
be decoded a second time.

Regards,
   Dennis

On 09/09/2010 02:03 PM, rovervr wrote:

> Hi,
> I created a small patch for that issue which works for me. But it needs
> to be reviewed by Igor or someone who knows C better than me.
> It checks the static request from X-Accel-Redirect for '%' and escapes
> them if found.
>
>
> [code]
> --- nginx-0.8.50orig/src/http/modules/ngx_http_static_module.c
> 2010-05-24 14:35:10.000000000 +0200
> +++ nginx-0.8.50/src/http/modules/ngx_http_static_module.c
> 2010-09-09 13:49:49.000000000 +0200
> @@ -47,7 +47,7 @@
>   static ngx_int_t
>   ngx_http_static_handler(ngx_http_request_t *r)
>   {
> -    u_char                    *last, *location;
> +    u_char                    *last, *location, *src, *dst;
>       size_t                     root, len;
>       ngx_str_t                  path;
>       ngx_int_t                  rc;
> @@ -83,6 +83,28 @@
>       ngx_log_debug1(NGX_LOG_DEBUG_HTTP, log, 0,
>                      "http filename: \"%s\"", path.data);
>
> +    /*
> +     * X-Accel-Redirect Patch
> +     * If the path contains a % it probably must be decoded
> +     */
> +    if( strstr( path.data, "%" ) != NULL )
> +    {
> +        ngx_str_t *uri =&path;
> +
> +        dst = uri->data;
> +        src = uri->data;
> +
> +        ngx_unescape_uri(&dst,&src, uri->len, NGX_UNESCAPE_URI );
> +
> +        len = uri->len - ( src - dst ) + 1;
> +        if ( len )
> +        {
> +            dst = ngx_copy( dst, src, len);
> +        }
> +        uri->len = dst - uri->data;
> +    }
> +
> +
>       clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
>
>       ngx_memzero(&of, sizeof(ngx_open_file_info_t));
>
> [/code]
>
> It would be nice to get a little feedback if this is ok.
>
> best regards,
>
> Volker Richter
>
> Posted at Nginx Forum: http://forum.nginx.org/read.php?2,128346,128745#msg-128745
>
>
> _______________________________________________
> nginx mailing list
> [hidden email]
> http://nginx.org/mailman/listinfo/nginx

_______________________________________________
nginx mailing list
[hidden email]
http://nginx.org/mailman/listinfo/nginx

[apeman]

If you'd use an url with a filename that contains an '%' the client
should always encode it to '%25' which would get decoded correctly in
this case.

In RFC1738 the '%' is an "Unsafe Character" and should always be encoded
in URIs

The check could be extended to something like %([a-f0-9]{2}) but
directories or files like %ears or %b1blabla would fail that check, too.

Posted at Nginx Forum: http://forum.nginx.org/read.php?2,128346,128760#msg-128760


_______________________________________________
nginx mailing list
[hidden email]
http://nginx.org/mailman/listinfo/nginx

[Maxim Dounin]

Hello!

On Thu, Sep 09, 2010 at 08:03:07AM -0400, rovervr wrote:

> I created a small patch for that issue which works for me. But it needs
> to be reviewed by Igor or someone who knows C better than me.
> It checks the static request from X-Accel-Redirect for '%' and escapes
> them if found.

This patch is wrong, it breaks access to normal files with '%'.  
Additionally, it doesn't change X-Accel-Redirect behaviour for
non-static files.

Instead X-Accel-Redirect value should be unescaped when it got
from upstream, somewhere before ngx_http_internal_redirect() call.
I personally believe ngx_http_parse_unsafe_uri() should be changed
to unescape uri (note that it will also affect ssi and dav
modules).  Though I haven't investigated this carefully enough.

Maxim Dounin

_______________________________________________
nginx mailing list
[hidden email]
http://nginx.org/mailman/listinfo/nginx

[apeman]

Hi Maxim,


[quote="Maxim Dounin"]
Additionally, it doesn't change X-Accel-Redirect behaviour for
non-static files.
[/quote]

Don't get me wrong .. but isn't X-Accel-Redirect for serving static
files without the overhead of php-fpm/fastcgi? Can you provide an
example where X-Accel-Redirect is used for dynamic files?


The patch works for me because I just use nginx to serve download files,
but I'd like to provide a patch that fixes the wrong behaviour of
X-Accel-Redirect for everyone.

Posted at Nginx Forum: http://forum.nginx.org/read.php?2,128346,128873#msg-128873


_______________________________________________
nginx mailing list
[hidden email]
http://nginx.org/mailman/listinfo/nginx

[apeman]

[quote="Maxim Dounin"]
This patch is wrong, it breaks access to normal files with '%'.
[/quote]

This code fixes the double encoding issue while serving static files
without X-Accel-Redirect as stated by Maxim


[code]
--- /tmp/nginx-0.8.50orig/src/http/modules/ngx_http_static_module.c    
2010-05-24 14:35:10.000000000 +0200
+++ ngx_http_static_module.c    2010-09-09 20:23:29.000000000 +0200
@@ -47,7 +47,7 @@
 static ngx_int_t
 ngx_http_static_handler(ngx_http_request_t *r)
 {
-    u_char                    *last, *location;
+    u_char                    *last, *location, *src, *dst;
     size_t                     root, len;
     ngx_str_t                  path;
     ngx_int_t                  rc;
@@ -83,6 +83,30 @@
     ngx_log_debug1(NGX_LOG_DEBUG_HTTP, log, 0,
                    "http filename: \"%s\"", path.data);

+    /*
+     * X-Accel-Redirect Patch
+     * If the path contains a % it probably must be decoded
+     */
+
+    if( strstr( path.data, "%" ) != NULL  &&
+       r->quoted_uri == 0)
+    {
+        ngx_str_t *uri = &path;
+
+        dst = uri->data;
+        src = uri->data;
+
+        ngx_unescape_uri( &dst, &src, uri->len, NGX_UNESCAPE_URI );
+
+        len = uri->len - ( src - dst ) + 1;
+        if ( len )
+        {
+            dst = ngx_copy( dst, src, len);
+        }
+        uri->len = dst - uri->data;
+    }
+
+
     clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);

     ngx_memzero(&of, sizeof(ngx_open_file_info_t));

[/code]

Posted at Nginx Forum: http://forum.nginx.org/read.php?2,128346,128876#msg-128876


_______________________________________________
nginx mailing list
[hidden email]
http://nginx.org/mailman/listinfo/nginx

[Maxim Dounin]

Hello!

On Thu, Sep 09, 2010 at 02:19:55PM -0400, rovervr wrote:

> Hi Maxim,
>
>
> [quote="Maxim Dounin"]
> Additionally, it doesn't change X-Accel-Redirect behaviour for
> non-static files.
> [/quote]
>
> Don't get me wrong .. but isn't X-Accel-Redirect for serving static
> files without the overhead of php-fpm/fastcgi? Can you provide an
> example where X-Accel-Redirect is used for dynamic files?

With X-Accel-Redirect you may redirect request to anything you
want, e.g. to appropriate backend.

Maxim Dounin

_______________________________________________
nginx mailing list
[hidden email]
http://nginx.org/mailman/listinfo/nginx

[Maxim Dounin]

Hello!

On Thu, Sep 09, 2010 at 02:29:04PM -0400, rovervr wrote:

> [quote="Maxim Dounin"]
> This patch is wrong, it breaks access to normal files with '%'.
> [/quote]
>
> This code fixes the double encoding issue while serving static files
> without X-Accel-Redirect as stated by Maxim

This one is wrong as well.  It won't solve the problem even for
static files when original request happened to contain escaped
chars.

And, as I already said, there is no sense to guess anything in
static module.  The problem should be solved elsewere, preserving
invariant of r->uri being unescaped uri without args.

Maxim Dounin

_______________________________________________
nginx mailing list
[hidden email]
http://nginx.org/mailman/listinfo/nginx

[apeman]

This is the last version of the patch for version 0.8.52 which is now
live on our production servers for several days without any flaws.

http://www.coderain.de/nginx/nginx-0.8.52-xred.patch

The escaping takes place at ngx_http_parse_unsafe_uri() as Maxim
suggested.

best regards

volker

Posted at Nginx Forum: http://forum.nginx.org/read.php?2,128346,136548#msg-136548


_______________________________________________
nginx mailing list
[hidden email]
http://nginx.org/mailman/listinfo/nginx

[Maxim Dounin]

Hello!

[sorry for long delay, I had no time to review the patch]

On Sun, Oct 03, 2010 at 10:11:58AM -0400, rovervr wrote:

> This is the last version of the patch for version 0.8.52 which is now
> live on our production servers for several days without any flaws.
>
> http://www.coderain.de/nginx/nginx-0.8.52-xred.patch
>
> The escaping takes place at ngx_http_parse_unsafe_uri() as Maxim
> suggested.

s/escaping/unescaping/

This patch is wrong.  It will unescape query string as well, which
is expected to remain escaped.  Additionaly, at least "../" unsafe
check should be reconsidered after unescaping.

Maxim Dounin

_______________________________________________
nginx mailing list
[hidden email]
http://nginx.org/mailman/listinfo/nginx