From patchwork Wed Jul 18 16:47:51 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joseph Myers X-Patchwork-Id: 28462 Received: (qmail 113878 invoked by alias); 18 Jul 2018 16:48:01 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 113399 invoked by uid 89); 18 Jul 2018 16:48:00 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS, URIBL_RED autolearn=ham version=3.3.2 spammy=INC, 2, 7, insecure, 25113 X-HELO: relay1.mentorg.com Date: Wed, 18 Jul 2018 16:47:51 +0000 From: Joseph Myers To: Carlos O'Donell CC: Subject: Re: Avoid insecure usage of tmpnam in tests In-Reply-To: Message-ID: References: User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 On Fri, 6 Jul 2018, Carlos O'Donell wrote: > > diff --git a/grp/tst_fgetgrent.c b/grp/tst_fgetgrent.c > > index 501ad99..d612445 100644 > > --- a/grp/tst_fgetgrent.c > > +++ b/grp/tst_fgetgrent.c > > @@ -21,6 +21,7 @@ > > #include > > #include > > #include > > +#include > > Why unistd.h? Because of the call to close that is added. > > @@ -784,7 +790,6 @@ get_null_defines (void) > > } > > result[result_len] = NULL; > > fclose (input); > > - remove (macrofile); > > Don't we still need to remove the temporary file? The same temporary file has a name generated once and is used in both get_null_defines and check_header. Thus, removing in get_null_defines renders the subsequent use in check_header (which has a corresponding removal) insecure. However, since check_header is called many times, for security it wasn't in fact sufficient to remove the remove call in get_null_defines; the second and subsequent calls to check_header would still be insecure. Thus, I am testing this revised patch that (for both affected tests) adds a remove call in main and removes that in check_header. (Each use of the file is via a call to system running a command redirecting to the file, so it gets truncated each time; it just needs to remain in existence between the calls, but it doesn't matter what contents are left there after each use because the next use will replace those contents.) Avoid insecure usage of tmpnam in tests. Various glibc testcases use tmpnam in ways subject to race conditions (generate a temporary file name, then later open that file without O_EXCL). This patch fixes those tests to use mkstemp - generally a minimal local fix to use mkstemp instead of tmpnam, rather than a larger fix to use other testsuite infrastructure for temporary files. The unchanged use of tmpnam in posix/wordexp-test.c would fail safe in the event of a race (it's generating a name for use with mkdir rather than for a file to be opened for writing). Tested for x86_64. 2018-07-18 Joseph Myers * grp/tst_fgetgrent.c: Include . (main): Use mkstemp instead of tmpnam. * io/test-utime.c (main): Likewise. * posix/annexc.c (macrofile): Change to modifiable array. (main): Remove macrofile here. (get_null_defines): Use mkstemp instead of tmpnam. Do not remove macrofile here. (check_header): Do not remove macrofile here. * posix/bug-getopt1.c: Include . (do_test): Use mkstemp instead of tmpnam. * posix/bug-getopt2.c: Include . (do_test): Use mkstemp instead of tmpnam. * posix/bug-getopt3.c: Include . (do_test): Use mkstemp instead of tmpnam. * posix/bug-getopt4.c: Include . (do_test): Use mkstemp instead of tmpnam. * posix/bug-getopt5.c: Include . (do_test): Use mkstemp instead of tmpnam. * stdio-common/bug7.c: Include and . (main): Use mkstemp instead of tmpnam. * stdio-common/tst-fdopen.c: Include . (main): Use mkstemp instead of tmpnam. * stdio-common/tst-ungetc.c: Include . (main): use mkstemp instead of tmpnam. * stdlib/isomac.c (macrofile): Change to modifiable array. (main): Remove macrofile here. (get_null_defines): Use mkstemp instead of tmpnam. Do not remove macrofile here. (check_header): Do not remove macrofile here. Reviewed-by: Carlos O'Donell diff --git a/grp/tst_fgetgrent.c b/grp/tst_fgetgrent.c index 501ad99..d612445 100644 --- a/grp/tst_fgetgrent.c +++ b/grp/tst_fgetgrent.c @@ -21,6 +21,7 @@ #include #include #include +#include static int errors; @@ -99,7 +100,14 @@ test_fgetgrent (const char *filename) int main (int argc, char *argv[]) { - char *file = tmpnam (NULL); + char file[] = "/tmp/tst_fgetgrent.XXXXXX"; + int fd = mkstemp (file); + if (fd == -1) + { + printf ("mkstemp failed: %m\n"); + return 1; + } + close (fd); int i = 0; if (argc > 1) diff --git a/io/test-utime.c b/io/test-utime.c index 2ad0995..0ab778e 100644 --- a/io/test-utime.c +++ b/io/test-utime.c @@ -27,23 +27,17 @@ int main (int argc, char *argv[]) { - char file[L_tmpnam]; + char file[] = "/tmp/test-utime.XXXXXX"; struct utimbuf ut; struct stat st; struct stat stnow; time_t now1, now2; int fd; - if (tmpnam (file) == 0) - { - perror ("tmpnam"); - return 1; - } - - fd = creat (file, 0666); + fd = mkstemp (file); if (fd < 0) { - perror ("creat"); + perror ("mkstemp"); return 1; } close (fd); diff --git a/posix/annexc.c b/posix/annexc.c index fe3a600..66768db 100644 --- a/posix/annexc.c +++ b/posix/annexc.c @@ -26,7 +26,7 @@ #define HEADER_MAX 256 -static const char *macrofile; +static char macrofile[] = "/tmp/annexc.XXXXXX"; /* . */ static const char *const aio_syms[] = @@ -657,6 +657,8 @@ main (int argc, char *argv[]) for (h = 0; h < NUMBER_OF_HEADERS; ++h) result |= check_header (&headers[h], ignore_list); + remove (macrofile); + /* The test suite should return errors but for now this is not practical. Give a warning and ask the user to correct the bugs. */ return result; @@ -712,7 +714,13 @@ get_null_defines (void) FILE *input; int first = 1; - macrofile = tmpnam (NULL); + int fd = mkstemp (macrofile); + if (fd == -1) + { + printf ("mkstemp failed: %m\n"); + exit (1); + } + close (fd); command = malloc (sizeof fmt + sizeof "/dev/null" + 2 * strlen (CC) + strlen (INC) + strlen (macrofile)); @@ -784,7 +792,6 @@ get_null_defines (void) } result[result_len] = NULL; fclose (input); - remove (macrofile); return (const char **) result; } @@ -879,7 +886,6 @@ check_header (const struct header *header, const char **except) result |= 1; } fclose (input); - remove (macrofile); for (i = 0; i < header->nsyms; ++i) if (found[i] == 0) diff --git a/posix/bug-getopt1.c b/posix/bug-getopt1.c index a47dc7e..a5a3711 100644 --- a/posix/bug-getopt1.c +++ b/posix/bug-getopt1.c @@ -1,6 +1,7 @@ /* BZ 11039 */ #include #include +#include static int one_test (const char *fmt, int argc, char *argv[], int expected[argc - 1]) @@ -39,12 +40,14 @@ one_test (const char *fmt, int argc, char *argv[], int expected[argc - 1]) static int do_test (void) { - char *fname = tmpnam (NULL); - if (fname == NULL) + char fname[] = "/tmp/bug-getopt1.XXXXXX"; + int fd = mkstemp (fname); + if (fd == -1) { - puts ("cannot generate name for temporary file"); + printf ("mkstemp failed: %m\n"); return 1; } + close (fd); if (freopen (fname, "w+", stderr) == NULL) { diff --git a/posix/bug-getopt2.c b/posix/bug-getopt2.c index 93c3035..8f92f0c 100644 --- a/posix/bug-getopt2.c +++ b/posix/bug-getopt2.c @@ -1,6 +1,7 @@ /* BZ 11039 */ #include #include +#include static int one_test (const char *fmt, int argc, char *argv[], int expected[argc - 1]) @@ -37,12 +38,14 @@ one_test (const char *fmt, int argc, char *argv[], int expected[argc - 1]) static int do_test (void) { - char *fname = tmpnam (NULL); - if (fname == NULL) + char fname[] = "/tmp/bug-getopt2.XXXXXX"; + int fd = mkstemp (fname); + if (fd == -1) { - puts ("cannot generate name for temporary file"); + printf ("mkstemp failed: %m\n"); return 1; } + close (fd); if (freopen (fname, "w+", stderr) == NULL) { diff --git a/posix/bug-getopt3.c b/posix/bug-getopt3.c index c3a8cb2..45a8d3e 100644 --- a/posix/bug-getopt3.c +++ b/posix/bug-getopt3.c @@ -2,6 +2,7 @@ #include #include #include +#include static const struct option opts[] = { @@ -48,12 +49,14 @@ one_test (const char *fmt, int argc, char *argv[], int n, int expected[n], static int do_test (void) { - char *fname = tmpnam (NULL); - if (fname == NULL) + char fname[] = "/tmp/bug-getopt3.XXXXXX"; + int fd = mkstemp (fname); + if (fd == -1) { - puts ("cannot generate name for temporary file"); + printf ("mkstemp failed: %m\n"); return 1; } + close (fd); if (freopen (fname, "w+", stderr) == NULL) { diff --git a/posix/bug-getopt4.c b/posix/bug-getopt4.c index 0956ca5..c5e3c14 100644 --- a/posix/bug-getopt4.c +++ b/posix/bug-getopt4.c @@ -2,6 +2,7 @@ #include #include #include +#include static const struct option opts[] = { @@ -52,12 +53,14 @@ one_test (const char *fmt, int argc, char *argv[], int n, int expected[n]) static int do_test (void) { - char *fname = tmpnam (NULL); - if (fname == NULL) + char fname[] = "/tmp/bug-getopt4.XXXXXX"; + int fd = mkstemp (fname); + if (fd == -1) { - puts ("cannot generate name for temporary file"); + printf ("mkstemp failed: %m\n"); return 1; } + close (fd); if (freopen (fname, "w+", stderr) == NULL) { diff --git a/posix/bug-getopt5.c b/posix/bug-getopt5.c index ed2639d..4f67d9b 100644 --- a/posix/bug-getopt5.c +++ b/posix/bug-getopt5.c @@ -2,6 +2,7 @@ #include #include #include +#include static const struct option opts[] = { @@ -47,12 +48,14 @@ one_test (const char *fmt, int argc, char *argv[], int n, int expected[n]) static int do_test (void) { - char *fname = tmpnam (NULL); - if (fname == NULL) + char fname[] = "/tmp/bug-getopt5.XXXXXX"; + int fd = mkstemp (fname); + if (fd == -1) { - puts ("cannot generate name for temporary file"); + printf ("mkstemp failed: %m\n"); return 1; } + close (fd); if (freopen (fname, "w+", stderr) == NULL) { diff --git a/stdio-common/bug7.c b/stdio-common/bug7.c index 2b1efe3..c9c2ef5 100644 --- a/stdio-common/bug7.c +++ b/stdio-common/bug7.c @@ -1,21 +1,25 @@ /* Regression test for fseek and freopen bugs. */ #include +#include +#include int main (int argc, char *argv[]) { int lose = 0; - char filename[L_tmpnam]; + char filename[] = "/tmp/bug7.XXXXXX"; FILE *fp; - if (tmpnam (filename) == NULL) + int fd = mkstemp (filename); + if (fd == -1) { - printf ("tmpnam failed\n"); + printf ("mkstemp failed\n"); lose = 1; } else { + close (fd); fp = fopen (filename, "w+"); fprintf (fp, "Hello world!\n"); fflush (fp); @@ -32,17 +36,21 @@ main (int argc, char *argv[]) { FILE *file1; FILE *file2; - char filename1[L_tmpnam]; - char filename2[L_tmpnam]; + char filename1[] = "/tmp/bug7.XXXXXX"; + char filename2[] = "/tmp/bug7.XXXXXX"; int ch; - if (tmpnam (filename1) == NULL || tmpnam (filename2) == NULL) + int fd1 = mkstemp (filename1); + int fd2 = mkstemp (filename2); + if (fd1 == -1 || fd2 == -1) { - printf ("tmpnam failed\n"); + printf ("mkstemp failed\n"); lose = 1; } else { + close (fd1); + close (fd2); file1 = fopen (filename1, "w"); fclose (file1); diff --git a/stdio-common/tst-fdopen.c b/stdio-common/tst-fdopen.c index e70a0cd..136fff5 100644 --- a/stdio-common/tst-fdopen.c +++ b/stdio-common/tst-fdopen.c @@ -1,6 +1,7 @@ /* Test for fdopen bugs. */ #include +#include #include #include @@ -18,12 +19,18 @@ char buffer[256]; int main (int argc, char *argv[]) { - char *name; + char name[] = "/tmp/tst-fdopen.XXXXXX"; FILE *fp = NULL; int retval = 0; int fd; - name = tmpnam (NULL); + fd = mkstemp (name); + if (fd == -1) + { + printf ("mkstemp failed: %m\n"); + return 1; + } + close (fd); fp = fopen (name, "w"); assert (fp != NULL) fputs ("foobar and baz", fp); diff --git a/stdio-common/tst-ungetc.c b/stdio-common/tst-ungetc.c index 44cf6a6..1344b2b 100644 --- a/stdio-common/tst-ungetc.c +++ b/stdio-common/tst-ungetc.c @@ -1,6 +1,7 @@ /* Test for ungetc bugs. */ #include +#include #include #undef assert @@ -15,13 +16,19 @@ int main (int argc, char *argv[]) { - char *name; + char name[] = "/tmp/tst-ungetc.XXXXXX"; FILE *fp = NULL; int retval = 0; int c; char buffer[64]; - name = tmpnam (NULL); + int fd = mkstemp (name); + if (fd == -1) + { + printf ("mkstemp failed: %m\n"); + return 1; + } + close (fd); fp = fopen (name, "w"); assert (fp != NULL) fputs ("bla", fp); diff --git a/stdlib/isomac.c b/stdlib/isomac.c index 8abf931..0873eaa 100644 --- a/stdlib/isomac.c +++ b/stdlib/isomac.c @@ -77,7 +77,7 @@ #define HEADER_MAX 256 -static const char *macrofile; +static char macrofile[] = "/tmp/isomac.XXXXXX"; /* ISO C header names including Amendment 1 (without ".h" suffix). */ static char *header[] = @@ -219,6 +219,8 @@ main (int argc, char *argv[]) result |= check_header (file_name, ignore_list); } + remove (macrofile); + /* The test suite should return errors but for now this is not practical. Give a warning and ask the user to correct the bugs. */ return result; @@ -249,7 +251,13 @@ get_null_defines (void) FILE *input; int first = 1; - macrofile = tmpnam (NULL); + int fd = mkstemp (macrofile); + if (fd == -1) + { + printf ("mkstemp failed: %m\n"); + exit (1); + } + close (fd); command = malloc (sizeof fmt + sizeof "/dev/null" + 2 * strlen (CC) + strlen (INC) + strlen (macrofile)); @@ -330,7 +338,6 @@ get_null_defines (void) } result[result_len] = NULL; fclose (input); - remove (macrofile); return (const char **) result; } @@ -439,7 +446,6 @@ check_header (const char *file_name, const char **except) } } fclose (input); - remove (macrofile); return result; }