Re: [PATCH] mktemp: don't use mktemp() function

[Bartosz Gołaszewski]

2014-12-11 18:08 GMT+01:00 Rich Felker dal...@libc.org:
 How is this an improvement? It increases the code size and performs
 unnecessary and potentially harmful filesystem operations. And it's
 just covering up the dangerous issue rather than fixing it -- using
 mkstemp then deleting the file and reusing the name is even MORE
 dangerous than using mktemp, since creating the file even momentarily
 exposed its name to an attacker. Of course if the code using the
 mktemp utility is written correctly, neither is dangerous anyway.

Ok, so submitting this might have been a bit rushed - in fact I
thought that this is the official upstream mktemp:
http://www.mktemp.org/mktemp/ and it does exactly that:

# strace ./mktemp -u
execve(./mktemp, [./mktemp, -u], [/* 40 vars */]) = 0
...
open(/tmp/tmp.AYcTpsHVko, O_RDWR|O_CREAT|O_EXCL, 0600) = 4
close(4)= 0
unlink(/tmp/tmp.AYcTpsHVko)   = 0
...
exit_group(0)   = ?

But there's also an mktemp implementation in coreutils which doesn't
create any files.

Let's drop it.

Bart
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH] mktemp: don't use mktemp() function

[Bartosz Golaszewski]

The linker emits this warning:

  warning: the use of `mktemp' is dangerous, better use `mkstemp' or `mkdtemp'

Fix it by using mkstemp() instead of mktemp().

function old new   delta
mktemp_main  214 233 +19
--
(add/remove: 0/0 grow/shrink: 1/0 up/down: 19/0)   Total: 19 bytes

Signed-off-by: Bartosz Golaszewski bartekg...@gmail.com
---
 debianutils/mktemp.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/debianutils/mktemp.c b/debianutils/mktemp.c
index 983d7a2..5ef557a 100644
--- a/debianutils/mktemp.c
+++ b/debianutils/mktemp.c
@@ -59,6 +59,8 @@ int mktemp_main(int argc UNUSED_PARAM, char **argv)
const char *path;
char *chp;
unsigned opts;
+   int fd;
+
enum {
OPT_d = 1  0,
OPT_q = 1  1,
@@ -93,8 +95,9 @@ int mktemp_main(int argc UNUSED_PARAM, char **argv)
chp = concat_path_file(path, chp);
 
if (opts  OPT_u) {
-   chp = mktemp(chp);
-   if (chp[0] == '\0')
+   close(fd = mkstemp(chp));
+   unlink(chp);
+   if (fd  0)
goto error;
} else if (opts  OPT_d) {
if (mkdtemp(chp) == NULL)
-- 
2.1.3

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] mktemp: don't use mktemp() function

[Rich Felker]

On Thu, Dec 11, 2014 at 06:01:57PM +0100, Bartosz Golaszewski wrote:
 The linker emits this warning:
 
   warning: the use of `mktemp' is dangerous, better use `mkstemp' or `mkdtemp'
 
 Fix it by using mkstemp() instead of mktemp().
 
 function old new   delta
 mktemp_main  214 233 +19
 --
 (add/remove: 0/0 grow/shrink: 1/0 up/down: 19/0)   Total: 19 bytes
 
 Signed-off-by: Bartosz Golaszewski bartekg...@gmail.com
 ---
  debianutils/mktemp.c | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)
 
 diff --git a/debianutils/mktemp.c b/debianutils/mktemp.c
 index 983d7a2..5ef557a 100644
 --- a/debianutils/mktemp.c
 +++ b/debianutils/mktemp.c
 @@ -59,6 +59,8 @@ int mktemp_main(int argc UNUSED_PARAM, char **argv)
   const char *path;
   char *chp;
   unsigned opts;
 + int fd;
 +
   enum {
   OPT_d = 1  0,
   OPT_q = 1  1,
 @@ -93,8 +95,9 @@ int mktemp_main(int argc UNUSED_PARAM, char **argv)
   chp = concat_path_file(path, chp);
  
   if (opts  OPT_u) {
 - chp = mktemp(chp);
 - if (chp[0] == '\0')
 + close(fd = mkstemp(chp));
 + unlink(chp);
 + if (fd  0)

How is this an improvement? It increases the code size and performs
unnecessary and potentially harmful filesystem operations. And it's
just covering up the dangerous issue rather than fixing it -- using
mkstemp then deleting the file and reusing the name is even MORE
dangerous than using mktemp, since creating the file even momentarily
exposed its name to an attacker. Of course if the code using the
mktemp utility is written correctly, neither is dangerous anyway.

Rich
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox