From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 125309 invoked by alias); 18 Jul 2018 17:49:14 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 125264 invoked by uid 89); 18 Jul 2018 17:49:10 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,URIBL_RED autolearn=ham version=3.3.2 spammy= X-HELO: mail-qt0-f174.google.com Return-Path: Subject: Re: Avoid insecure usage of tmpnam in tests To: Joseph Myers Cc: libc-alpha@sourceware.org References: From: Carlos O'Donell Openpgp: preference=signencrypt Message-ID: Date: Wed, 18 Jul 2018 17:49:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-07/txt/msg00563.txt.bz2 On 07/18/2018 12:47 PM, Joseph Myers wrote: > 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. Thanks. >>> @@ -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.) Thanks, I missed that check_header() would remove it, but yes, if it's called multiple times then it should not be the site at which we call remove(). > > 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. This version looks good to me. 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 OK. > > 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); OK. > 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); OK. > 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); OK. This makes sense to me now. > + > /* 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); OK. > > return (const char **) result; > } > @@ -879,7 +886,6 @@ check_header (const struct header *header, const char **except) > result |= 1; > } > fclose (input); > - remove (macrofile); OK. > > for (i = 0; i < header->nsyms; ++i) > if (found[i] == 0) OK. Now annec.c looks correct to me. > 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); OK. > + > /* 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); OK. > > return (const char **) result; > } > @@ -439,7 +446,6 @@ check_header (const char *file_name, const char **except) > } > } > fclose (input); > - remove (macrofile); OK. > > return result; > } > -- Cheers, Carlos.