[patch] make sure all uses of mkstemp follow umask

[Erik Hovland]

On Fri, Jul 21, 2006 at 11:33:11AM -0700, Erik Hovland wrote:
> On Fri, Jul 21, 2006 at 08:34:47AM +0200, Tito wrote:
> > On Friday 21 July 2006 00:27, Erik Hovland wrote:
> > > Using mkstemp without setting the permission mask with umask potentially
> > > harmful.
> > > 
> > > Since there are several uses of mkstemp in busybox and none of them use
> > > umask, I made a libbb/umaskmkstemp.c file. That file has a umask() then
> > > mkstemp() call in it called umaskmkstemp(). Then I switched all of the
> > > users of mkstemp to this call instead.
> > > 
> > > E
> > > 
> > Hi,
> > would it not be better to reset the old mask after the mkstemp call?
> > 
> > int umaskmkstemp(char *path)
> > {
> > 	mode_t   old_mask = umask(S_IRWXO | S_IRWXG);
> >        int ret = mkstemp(path);
> > 	umask(old_mask);
> > 	return ret;
> > }
> > 
> > 
> > Just a thought.
> 
> Dang good idea. I will update my patch.

Patch updated and attached.

Thanks

E

-- 
Erik Hovland
mail: erik AT hovland DOT org
web: http://hovland.org/
PGP/GPG public key available on request
-------------- next part --------------
diff -Nurd -x .svn busybox-ro/coreutils/dos2unix.c busybox-trunk/coreutils/dos2unix.c
--- busybox-ro/coreutils/dos2unix.c	2006-09-07 14:32:29.185602906 -0700
+++ busybox-trunk/coreutils/dos2unix.c	2006-08-16 12:03:00.216498903 -0700
@@ -38,7 +38,7 @@
 		   hold the full path.  However if the output is truncated the
 		   subsequent call to mkstemp would fail.
 		 */
-		if ((i = mkstemp(&bb_common_bufsiz1[0])) == -1
+		if ((i = umaskmkstemp(&bb_common_bufsiz1[0])) == -1
 			|| chmod(bb_common_bufsiz1, 0600) == -1) {
 			bb_perror_nomsg_and_die();
 		}
diff -Nurd -x .svn busybox-ro/debianutils/mktemp.c busybox-trunk/debianutils/mktemp.c
--- busybox-ro/debianutils/mktemp.c	2006-07-14 16:07:44.524806765 -0700
+++ busybox-trunk/debianutils/mktemp.c	2006-07-21 16:36:01.537210272 -0700
@@ -28,7 +28,7 @@
 			return EXIT_FAILURE;
 	}
 	else {
-		if (mkstemp(argv[optind]) < 0)
+		if (umaskmkstemp(argv[optind]) < 0)
 			return EXIT_FAILURE;
 	}
 
diff -Nurd -x .svn busybox-ro/e2fsprogs/blkid/save.c busybox-trunk/e2fsprogs/blkid/save.c
--- busybox-ro/e2fsprogs/blkid/save.c	2006-07-14 16:07:56.009162328 -0700
+++ busybox-trunk/e2fsprogs/blkid/save.c	2006-07-21 16:36:01.542209556 -0700
@@ -93,7 +93,7 @@
 	if (ret == 0 && S_ISREG(st.st_mode)) {
 		tmp = xmalloc(strlen(filename) + 8);
 		sprintf(tmp, "%s-XXXXXX", filename);
-		fd = mkstemp(tmp);
+		fd = umaskmkstemp(tmp);
 		if (fd >= 0) {
 			file = fdopen(fd, "w");
 			opened = tmp;
diff -Nurd -x .svn busybox-ro/editors/sed.c busybox-trunk/editors/sed.c
--- busybox-ro/editors/sed.c	2006-09-07 14:32:31.659248425 -0700
+++ busybox-trunk/editors/sed.c	2006-09-06 13:43:39.208260175 -0700
@@ -1174,7 +1174,7 @@
 
 						bbg.outname=xstrndup(argv[i],strlen(argv[i])+6);
 						strcat(bbg.outname,"XXXXXX");
-						if(-1==(nonstdoutfd=mkstemp(bbg.outname)))
+						if(-1==(nonstdoutfd=umaskmkstemp(bbg.outname)))
 							bb_error_msg_and_die("no temp file");
 						bbg.nonstdout=fdopen(nonstdoutfd,"w");
 
diff -Nurd -x .svn busybox-ro/include/libbb.h busybox-trunk/include/libbb.h
--- busybox-ro/include/libbb.h	2006-09-07 14:32:31.094329380 -0700
+++ busybox-trunk/include/libbb.h	2006-09-07 13:39:44.609215672 -0700
@@ -514,6 +514,7 @@
 extern const char bb_uuenc_tbl_base64[];
 extern const char bb_uuenc_tbl_std[];
 extern void bb_uuencode(const unsigned char *s, char *store, const int length, const char *tbl);
+extern int umaskmkstemp(char* path);
 
 #ifndef COMM_LEN
 #ifdef TASK_COMM_LEN
diff -Nurd -x .svn busybox-ro/libbb/Makefile.in busybox-trunk/libbb/Makefile.in
--- busybox-ro/libbb/Makefile.in	2006-09-07 14:32:30.003485701 -0700
+++ busybox-trunk/libbb/Makefile.in	2006-09-07 14:34:52.814021397 -0700
@@ -35,7 +35,7 @@
 	getopt_ulflags.c default_error_retval.c wfopen_input.c speed_table.c \
 	perror_nomsg_and_die.c perror_nomsg.c skip_whitespace.c bb_askpass.c \
 	warn_ignoring_args.c concat_subpath_file.c vfork_daemon_rexec.c \
-	bb_do_delay.c
+	bb_do_delay.c umaskmkstemp.c
 
 # conditionally compiled objects:
 LIBBB-$(CONFIG_FEATURE_MOUNT_LOOP)+= loop.c
diff -Nurd -x .svn busybox-ro/libbb/umaskmkstemp.c busybox-trunk/libbb/umaskmkstemp.c
--- busybox-ro/libbb/umaskmkstemp.c	1969-12-31 16:00:00.000000000 -0800
+++ busybox-trunk/libbb/umaskmkstemp.c	2006-07-25 14:24:19.678357534 -0700
@@ -0,0 +1,22 @@
+/* vi: set sw=4 ts=4: */
+/*
+ * umaskmkstemp - combines umask and mkstemp so we don't use mkstemp
+ * unsafely.
+ *
+ * Copyright (C) 2006 Erik Hovland
+ *
+ * Licensed under GPLv2 or later, see file LICENSE in this tarball for details.
+ */
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <stdlib.h>
+#include "libbb.h"
+
+int umaskmkstemp(char *path)
+{
+	mode_t prevumask = umask(S_IRWXO | S_IRWXG);
+	int tempfd = mkstemp(path);
+	umask(prevumask);
+	return tempfd;
+}
+
diff -Nurd -x .svn busybox-ro/shell/msh.c busybox-trunk/shell/msh.c
--- busybox-ro/shell/msh.c	2006-09-07 14:32:28.617684291 -0700
+++ busybox-trunk/shell/msh.c	2006-08-16 12:02:59.769562856 -0700
@@ -5085,7 +5085,7 @@
 
 	DBGPRINTF7(("READHERE: enter, name=%p, s=%p\n", name, s));
 
-	tf = mkstemp(tname);
+	tf = umaskmkstemp(tname);
 	if (tf < 0)
 		return;
 
@@ -5156,7 +5156,7 @@
 		char tname[30] = ".msh_XXXXXX";
 		jmp_buf ev;
 
-		tf = mkstemp(tname);
+		tf = umaskmkstemp(tname);
 		if (tf < 0)
 			return (-1);
 		if (newenv(setjmp(errpt = ev)) == 0) {