On Tue, Jun 17, 2008 at 7:40 AM, Santiago Vila <[EMAIL PROTECTED]> wrote:
> Denver: Please tell me if you consider this patch acceptable to fix
> the insecure use of tmpnam, as I plan to apply it to the Debian
> version of wdiff:
>
> diff -ru wdiff-0.5.original/wdiff.c wdiff-0.5/wdiff.c
> --- wdiff-0.5.original/wdiff.c 2008-06-17 11:30:26.000000000 +0000
> +++ wdiff-0.5/wdiff.c 2008-06-17 11:31:21.159349530 +0000
> @@ -560,6 +560,7 @@
> split_file_into_words (SIDE *side)
> {
> struct stat stat_buffer; /* for checking if file is directory */
> + int fd; /* for file descriptors returned by mkstemp */
>
> /* Open files. */
>
> @@ -571,8 +572,9 @@
> this temporary local file. Once done, prepare it for reading.
> We do not need the file name itself anymore. */
>
> - tmpnam (side->temp_name);
> - side->file = fopen (side->temp_name, "w+");
> + strcpy(side->temp_name, "/tmp/wdiff.XXXXXX");
> + fd = mkstemp(side->temp_name);
> + side->file = fdopen(fd, "w+");
> if (side->file == NULL)
> error (EXIT_OTHER_REASON, errno, side->temp_name);
> if (unlink (side->temp_name) != 0)
> @@ -598,8 +600,9 @@
> side->character = getc (side->file);
> side->position = 0;
>
> - tmpnam (side->temp_name);
> - side->temp_file = fopen (side->temp_name, "w");
> + strcpy(side->temp_name, "/tmp/wdiff.XXXXXX");
> + fd = mkstemp(side->temp_name);
> + side->temp_file = fdopen(fd, "w");
> if (side->temp_file == NULL)
> error (EXIT_OTHER_REASON, errno, side->temp_name);
This patch is acceptable. I do have some cautionary comments, though.
First of all, using /tmp/wdiff.XXXXXX as a template does not seem very
portable. Perhaps you want to try P_tmpdir from <stdio.h> first as
glibc's tmpfile does. However, if you can guarantee that a Debian
system will always have a /tmp directory that is writable by any user,
then it is ok.
Secondly, the manpage for mkstemp has the following note: "Don't use
this function, use tmpfile(3) instead.". However, switching to
tmpfile is not practical for wdiff 0.5 because the filename string
must be passed to diff later on. I think we will have to stick with
mkstemp until wdiff starts using a diff library, which will probably
be in version 0.7. So mkstemp is ok for now.
Thanks for your diligence in fixing this problem.
Denver
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
