[Groff] Re: groff 1.17: pre-grohtml's unsafe temporary file handling & other improvements

[solar]

On Tue, Jun 05, 2001 at 11:39:46AM +0100, Gaius Mulley wrote:

Hi Gaius,

Thanks for CC'ing me on this.

> here are some patches which include:
> 
>   * improved security w.r.t grohtml, thanks from the people at openwall.
>   * altering the syntax of \On to \O[2] and \O[5 filename] as Werner
>     suggested.
>   * new option to grohtml -D, invoked via groff -P-Ddirpng which places all
>     images in directory dirpng.
>   * another grohtml option -I, groff -P-Ifoobar which defines the image stem:
>     such as: foobar-%d.png

I no longer see security problems with this code, but I think it could
be made cleaner and more reliable.

> +static char *image_dir      = 0;                // user specified image 
> directory

Why do you dislike NULL in initializers?  A bug in some broken libc?

> -  sprintf(buffer, "grohtml-%d", (int)getpid());
> +  if (image_dir == NULL) {
> +    image_dir = (char *)malloc(1);
> +    image_dir[0] = (char)0;

Why not just do:

        image_dir = "";

None of the uses of image_dir in your patch require that it's malloc'ed
or can be written to.

> +  } else if ((strlen(image_dir)>1) && (image_dir[strlen(image_dir)-1] != 
> '/')) {

I think the check should be ">= 1", not "> 1".

> +    sprintf(buffer, "%s/", image_dir);
> +    image_dir = strdup(buffer);

This isn't a security problem as no privilege boundaries are involved,
but I think the code needs to be changed to avoid arbitrary limits
like this or at least to not contain buffer overflow possibilities
(could check user-provided pathnames against PATH_MAX and use buffers
which are sufficiently larger than PATH_MAX).  There're many places,
but I think it's worth doing for code which is meant to be widely used
(and groff is).

For portable code, I would write this with xmalloc and a temporary
variable to save the pointer in.  (If you could ensure that image_dir
is malloc'ed, then xrealloc would work here as well.)

For non-portable code, there's asprintf(3).

> +  if ((image_dir != NULL) && (strcmp(image_dir, "") != 0))

Maybe it's just me, but I don't think the strcmp() is really clearer
than image_dir[0] != '\0' or even *image_dir.  It takes me longer to
make sure that a strcmp() check is correct.

-- 
/sd