Date: Tue, 11 May 2004 08:52:24 -0700 (PDT) From: Jim Luther <luther.j@apple.com> To: freebsd-gnats-submit@FreeBSD.org Subject: standards/66531: _gettemp uses a far smaller set of filenames than documented and doesn't range check inputs Message-ID: <200405111552.i4BFqO9P033698@www.freebsd.org> Resent-Message-ID: <200405111600.i4BG0fPk070591@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 66531
>Category: standards
>Synopsis: _gettemp uses a far smaller set of filenames than documented and doesn't range check inputs
>Confidential: no
>Severity: non-critical
>Priority: medium
>Responsible: freebsd-standards
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Tue May 11 09:00:40 PDT 2004
>Closed-Date:
>Last-Modified:
>Originator: Jim Luther
>Release: Mac OS X 10.3.3
>Organization:
Apple Computer, Inc.
>Environment:
Darwin luthji.apple.com 7.3.0 Darwin Kernel Version 7.3.0: Fri Mar 5 14:22:55 PST 2004; root:xnu/xnu-517.3.15.obj~4/RELEASE_PPC Power Macintosh powerpc
>Description:
_gettemp is the common subroutine used by mktemp(), mkstemp(), mkstemps(), and mkdtemp() to get temporary file names, create temporary files, and create temporary directories. I found a couple of bugs in it:
1 - The suffix length (slen) parameter passed into it is not range changed -- it could be longer than the basename of the path, or it could be negative. This could cause mkstemps() to possibly create a temporary file in the wrong directory if path[strlen(path)] - slen is an 'X" character and the random character chosen happens to match another directory. For example:
char template[] = "/X/template";
suffixlen = 9;
fd = mkstemps(template, suffixlen);
In the current code, the 'X' in the template would be replaced with a random character and if there were a directory in "/" with that character name, a file named "template" would be created in that directory.
If slen is negative, it could corrupt memory outside of the path buffer.
2 - If there's a filename collision, it only cycles through a subset of the possible filenames -- it does not try all permutations. The current code increments the characters until it hits the end of the padchar array -- it does not wrap around to the original random character. So, if by chance, this loop in _getttemp():
/* Fill space with random characters */
while (trv >= path && *trv == 'X') {
rand = arc4random() % (sizeof(padchar) - 1);
*trv-- = padchar[rand];
}
happened to get random numbers that replaced all of the 'X' characters with 'z' (the last character in padchar array), the collision code would exit without trying any other filenames.
>How-To-Repeat:
I used this test program to detect the problem and ensure it is fixed:
#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <string.h>
#include <sys/syslimits.h>
#define XX_MAX 3844 /* maximum on UFS volume (62 * 62) */
//#define XX_MAX 1296 /* maximum on HFS case-insensitive volume (36 * 36) */
int mkstemp_test(void)
{
char mkstemp_template[] = "/gettemp_test_dir/file_XX";
char template[NAME_MAX];
int i;
int fd;
i = 0;
while ( 1 )
{
strcpy(template, mkstemp_template);
fd = mkstemp(template);
if ( fd >= 0 )
{
close(fd);
if ( i >= XX_MAX )
{
fprintf(stdout, "mkstemp_test failed: created too many files?!?\n");
return ( EXIT_FAILURE );
}
}
else
{
if ( i == XX_MAX )
{
fprintf(stdout, "mkstemp_test passed\n");
return ( EXIT_SUCCESS );
}
else
{
fprintf(stdout, "mkstemp_test failed when creating file #%d\n", i + 1);
return ( EXIT_FAILURE );
}
}
++i;
}
}
int mkstemps_test(void)
{
char mkstemps_template[] = "/gettemp_test_dir/file_XX.test";
char template[NAME_MAX];
int i;
int fd;
strcpy(template, mkstemps_template);
fd = mkstemps(template, 13);
if ( fd >= 0 )
{
fprintf(stdout, "mkstemps_test failed: should have detected too long suffixlen parameter\n");
return ( EXIT_FAILURE );
}
strcpy(template, mkstemps_template);
fd = mkstemps(template, -1);
if ( fd >= 0 )
{
fprintf(stdout, "mkstemps_test failed: should have detected negative suffixlen parameter\n");
return ( EXIT_FAILURE );
}
i = 0;
while ( 1 )
{
strcpy(template, mkstemps_template);
fd = mkstemps(template, 5);
if ( fd >= 0 )
{
close(fd);
if ( i >= XX_MAX )
{
fprintf(stdout, "mkstemps_test failed: created too many files?!?\n");
return ( EXIT_FAILURE );
}
}
else
{
if ( i == XX_MAX )
{
fprintf(stdout, "mkstemps_test passed\n");
return ( EXIT_SUCCESS );
}
else
{
fprintf(stdout, "mkstemps_test failed when creating file #%d\n", i + 1);
return ( EXIT_FAILURE );
}
}
++i;
}
}
int mkdtemp_test(void)
{
char mkdtemp_template[] = "/gettemp_test_dir/dir_XX";
char template[NAME_MAX];
int i;
char *result;
i = 0;
while ( 1 )
{
strcpy(template, mkdtemp_template);
result = mkdtemp(template);
if ( result != NULL )
{
if ( i >= XX_MAX )
{
fprintf(stdout, "mkdtemp_test failed: created too many directories?!?\n");
return ( EXIT_FAILURE );
}
}
else
{
if ( i == XX_MAX )
{
fprintf(stdout, "mkdtemp_test passed\n");
return ( EXIT_SUCCESS );
}
else
{
fprintf(stdout, "mkdtemp_test failed when creating directory #%d\n", i + 1);
return ( EXIT_FAILURE );
}
}
++i;
}
}
int main (int argc, const char * argv[])
{
int err;
err = mkdir("/gettemp_test_dir", 0700);
if ( err == 0 )
{
err = mkstemp_test();
if ( !err )
{
err = mkstemps_test();
if ( !err )
{
err = mkdtemp_test();
if ( !err )
{
exit(EXIT_SUCCESS);
}
}
}
}
else
{
fprintf(stderr, "mkdir could not create '/gettemp_test_dir': delete it and try again\n");
}
exit(EXIT_FAILURE);
}
>Fix:
Here's the diffs from the patch used to fix this in Mac OS X. The file patched was (using the FreeBSD path) src/lib/libc/stdio/mktemp.c, v 1.28 so the diffs should work for FreeBSD, too.
Index: stdio/FreeBSD/mktemp.c
===================================================================
RCS file: /cvs/root/Libc/stdio/FreeBSD/mktemp.c,v
retrieving revision 1.2
retrieving revision 1.2.44.2
diff -u -d -b -r1.2 -r1.2.44.2
--- mktemp.c 2003/05/20 22:22:42 1.2
+++ mktemp.c 2004/03/16 23:05:15 1.2.44.2
@@ -106,13 +106,14 @@
int domkdir;
int slen;
{
- char *start, *trv, *suffp;
+ char *start, *trv, *suffp, *carryp;
char *pad;
struct stat sbuf;
int rval;
uint32_t rand;
+ char carrybuf[NAME_MAX];
- if (doopen != NULL && domkdir) {
+ if ((doopen != NULL && domkdir) || slen < 0) {
errno = EINVAL;
return (0);
}
@@ -122,7 +123,7 @@
trv -= slen;
suffp = trv;
--trv;
- if (trv < path) {
+ if (trv < path || NULL != strchr(suffp, '/')) {
errno = EINVAL;
return (0);
}
@@ -134,6 +135,9 @@
}
start = trv + 1;
+ /* save first combination of random characters */
+ memcpy(carrybuf, start, suffp - start);
+
/*
* check the target directory.
*/
@@ -170,14 +174,25 @@
return (errno == ENOENT);
/* If we have a collision, cycle through the space of filenames */
- for (trv = start;;) {
- if (*trv == '\0' || trv == suffp)
- return (0);
+ for (trv = start, carryp = carrybuf;;) {
+ /* have we tried all possible permutations? */
+ if (trv == suffp)
+ return (0); /* yes - exit with EEXIST */
pad = strchr(padchar, *trv);
- if (pad == NULL || *++pad == '\0')
- *trv++ = padchar[0];
- else {
- *trv++ = *pad;
+ if (pad == NULL) {
+ /* this should never happen */
+ errno = EIO;
+ return (0);
+ }
+ /* increment character */
+ *trv = (*++pad == '\0') ? padchar[0] : *pad;
+ /* carry to next position? */
+ if (*trv == *carryp) {
+ /* increment position and loop */
+ ++trv;
+ ++carryp;
+ } else {
+ /* try with new name */
break;
}
}
>Release-Note:
>Audit-Trail:
>Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200405111552.i4BFqO9P033698>