Re: CVE id request: mktemp

["Todd C. Miller" <Todd.Miller () courtesan com>]

In message <20080815115550.GD31878 () ngolde de>
        so spake Nico Golde (oss-security+ml):

> Hi,
> mktemp (not the coreutils one) from
> ftp://ftp.mktemp.org/pub/mktemp/ is not generating fully
> random names. Steve, can you assign a CVE id to this?
>
> This is=20
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=3D495193
> I wrote an explanation on why this happens, available on:
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=3D495193#30

The private version of mktemp(3) used by mktemp(1) is based on the
BSD version which uses the pid as part of the file name.  If there
is a small number X's, only the pid will be used when constructing
the file name.

It's probably fine to just remove the pid from the equation entirely.
I'll look at putting out a new version of mktemp(1) in the next few
days with this fix and some other changes.

In the meantime, the following diff should suffice.

 - todd

Index: priv_mktemp.c
===================================================================
RCS file: /home/cvs/courtesan/mktemp/priv_mktemp.c,v
retrieving revision 1.8
diff -u -p -u -r1.8 priv_mktemp.c
--- priv_mktemp.c       26 Apr 2004 18:31:35 -0000      1.8
+++ priv_mktemp.c       15 Aug 2008 14:31:32 -0000
@@ -84,22 +84,23 @@ _gettemp(path, doopen, domkdir)
                return(0);
        }
 
-       pid = getpid();
+       if (*path == '\0') {
+               errno = EINVAL;
+               return(0);
+       }
+
        for (trv = path; *trv; ++trv)
                ;
-       --trv;
-       while (trv >= path && *trv == 'X' && pid != 0) {
-               *trv-- = (pid % 10) + '0';
-               pid /= 10;
-       }
-       while (trv >= path && *trv == 'X') {
-               char c;
+       do {
+               --trv;
+       } while (trv >= path && *trv == 'X');
+       start = trv + 1;
 
+again:
+       for (trv = start; *trv; ++trv) {
                pid = (get_random() & 0xffff) % (26+26);
-               c = alphabet[pid];
-               *trv-- = c;
+               *trv = alphabet[pid];
        }
-       start = trv + 1;
 
        /*
         * check the target directory; if you have six X's and it
@@ -124,58 +125,20 @@ _gettemp(path, doopen, domkdir)
                }
        }
 
-       for (;;) {
-               if (doopen) {
-                       if ((*doopen =
-                           open(path, O_CREAT|O_EXCL|O_RDWR, 0600)) >= 0)
-                               return(1);
-                       if (errno != EEXIST)
-                               return(0);
-               } else if (domkdir) {
-                       if (mkdir(path, 0700) == 0)
-                               return(1);
-                       if (errno != EEXIST)
-                               return(0);
-               } else if (lstat(path, &sbuf))
-                       return(errno == ENOENT ? 1 : 0);
+       if (doopen) {
+               if ((*doopen =
+                   open(path, O_CREAT|O_EXCL|O_RDWR, 0600)) >= 0)
+                       return(1);
+               if (errno != EEXIST)
+                       return(0);
+       } else if (domkdir) {
+               if (mkdir(path, 0700) == 0)
+                       return(1);
+               if (errno != EEXIST)
+                       return(0);
+       } else if (lstat(path, &sbuf))
+               return(errno == ENOENT ? 1 : 0);
 
-               /* tricky little algorithm for backward compatibility */
-               for (trv = start;;) {
-                       if (!*trv)
-                               return (0);
-                       if (*trv == 'Z')
-                               *trv++ = 'a';
-                       else {
-                               if (isdigit((unsigned char)(*trv)))
-                                       *trv = 'a';
-                               else if (*trv == 'z')   /* wrap from z to A */
-                                       *trv = 'A';
-                               else {
-#ifdef HAVE_EBCDIC
-                                       switch(*trv) {
-                                       case 'i':
-                                               *trv = 'j';
-                                               break;
-                                       case 'r':
-                                               *trv = 's';
-                                               break;
-                                       case 'I':
-                                               *trv = 'J';
-                                               break;
-                                       case 'R':
-                                               *trv = 'S';
-                                               break;
-                                       default:
-                                               ++*trv;
-                                               break;
-                                       }
-#else
-                                       ++*trv;
-#endif
-                               }
-                               break;
-                       }
-               }
-       }
+       goto again;
        /*NOTREACHED*/
 }