[PATCH] perl module: fix SSI parameter termination bug

[Maxim Dounin]

Hello!

On Tue, Mar 13, 2012 at 08:34:33PM +1300, Matthew Daley wrote:

> Hi,
> 
> There is a small issue in the handling of the SSI command provided by
> the Perl module.
> 
> It NULL-terminates the 'sub' parameter value before passing it to
> ngx_http_perl_eval_anon_sub, but this has the problem that if the
> parameter's length is the maximum allowable for SSI commands, it will
> write the NULL byte just past the end of the allocated buffer:
> 
> ==14669== Invalid write of size 1
> ==14669==    at 0x80A79E3: ngx_http_perl_ssi (ngx_http_perl_module.c:384)
> ==14669==    by 0x80904CF: ngx_http_ssi_body_filter (ngx_http_ssi_filter_module.c:794)

[...]

> I have attached a patch which attempts to fix the problem by creating
> an appropriately-sized buffer and NULL-terminating a copy of the
> string to eval.

Nice catch, thanks.  The patch is wrong though, see below.

[...]

> @@ -381,7 +382,6 @@
>      ctx->ssi = ssi_ctx;
>  
>      handler = params[NGX_HTTP_PERL_SSI_SUB];
> -    handler->data[handler->len] = '\0';
>  
>      {
>  
> @@ -392,7 +392,7 @@
>  
>      /* the code is disabled to force the precompiled perl code using only */
>  
> -    ngx_http_perl_eval_anon_sub(aTHX_ handler, &sv);
> +    ngx_http_perl_eval_anon_sub(aTHX_ r->pool, handler, &sv);

This code is indeed disabled, as stated in the comment above, it's 
under #if 0.

The null-terminated string in handler is required for 

    sv = newSVpvn((char *) handler->data, handler->len);

below though, and your patch will break it.

Correct solution would be to reserve space for NUL character in 
ssi module instead.  Quick and dirty patch which should work:

--- a/src/http/modules/ngx_http_ssi_filter_module.c
+++ b/src/http/modules/ngx_http_ssi_filter_module.c
@@ -1204,7 +1204,7 @@ ngx_http_ssi_parse(ngx_http_request_t *r
 
                 if (ctx->value_buf == NULL) {
                     ctx->param->value.data = ngx_pnalloc(r->pool,
-                                                         ctx->value_len);
+                                                         ctx->value_len + 1);
                     if (ctx->param->value.data == NULL) {
                         return NGX_ERROR;
                     }

Maxim Dounin