[PATCH] perl module: fix SSI parameter termination bug

[Matthew Daley]

On Tue, Mar 13, 2012 at 9:28 PM, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 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.

Yeah, just wanted to make sure that it'd still work if re-enabled.

>
> The null-terminated string in handler is required for
>
>    sv = newSVpvn((char *) handler->data, handler->len);
>
> below though, and your patch will break it.

Hmm, I was going by this info in the perlapi documentation, which made
me think that newSVpvn didn't need it:
"Notice that you can choose to specify the length of the string to be
assigned by using sv_setpvn , newSVpvn , or newSVpv , or you may allow
Perl to calculate the length by using sv_setpv or by specifying 0 as
the second argument to newSVpv . Be warned, though, that Perl will
determine the string's length by using strlen , which depends on the
string terminating with a NUL character."

However I see below that it says:
"All SVs that contain strings should be terminated with a NUL
character. If it is not NUL-terminated there is a risk of core dumps
and corruptions from code which passes the string to C functions or
system calls which expect a NUL-terminated string. Perl's own
functions typically add a trailing NUL for this reason."

So you are right.

>
> 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;
>                     }
>

I wasn't sure if changing the SSI allocation code was appropriate in
this case, but since newSVpvn really does need the NULL termination,
this is indeed a better idea.