Fix sudoers locking in visudo. We now lock the sudoers file itself, not

the temp file (since locking the temp file can foul up editors).  The
previous locking scheme didn't work because the fd was closed too early.

diff --git a/CHANGES b/CHANGES
index 8b238329402043cace09e2ea1e318a57f2850d1a..a3730f0d91415e4f94609efa4b0a0cada545e1b3 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1236,3 +1236,6 @@ Sudo 1.6.1 released.
 390) Systems that return RLIM_INFINITY for RLIMIT_NOFILE (like AIX)
      would loop for a very loing time during sudo startup.  A value of
      RLIM_INFINITY is now ignored (getdtablesize/sysconf is used instead).
+
+391) Locking in visudo was broken.  We now lock the sudoers file, not the
+     sudoers temp file, which should be safe.

diff --git a/config.h.in b/config.h.in
index 768ea731f02d7d086aac29bfb79ef14e21f93f9f..f6e5b15de04d780f92022a0f86dc9e031046184c 100644 (file)
--- a/config.h.in
+++ b/config.h.in
@@ -251,6 +251,9 @@
 /* Define if you have flock(2).  */
 #undef HAVE_FLOCK
 
+/* Define if you have fstat(2).  */
+#undef HAVE_FSTAT
+
 /* Define if you have setrlimit(2).  */
 #undef HAVE_SETRLIMIT
 

diff --git a/configure b/configure
index ebbc19e5e54a836f6b4dd6a19b465f36b8f4cc89..876186a6875dde31aba915eb13b940fab54c428c 100755 (executable)
--- a/configure
+++ b/configure
@@ -5413,7 +5413,7 @@ EOF
 
 ;;
 esac
-for ac_func in strchr strrchr memchr memcpy memset sysconf sigaction tzset seteuid strftime setrlimit initgroups
+for ac_func in strchr strrchr memchr memcpy memset sysconf sigaction tzset seteuid strftime setrlimit initgroups fstat
 do
 echo $ac_n "checking for $ac_func""... $ac_c" 1>&6
 echo "configure:5420: checking for $ac_func" >&5

diff --git a/configure.in b/configure.in
index 5379d08eee3a979297415f497a70ede773b796b5..35b5eb90850fad2766fcf5927ed89a1233696029 100644 (file)
--- a/configure.in
+++ b/configure.in
@@ -1343,7 +1343,7 @@ esac
 dnl
 dnl Function checks
 dnl
-AC_CHECK_FUNCS(strchr strrchr memchr memcpy memset sysconf sigaction tzset seteuid strftime setrlimit initgroups)
+AC_CHECK_FUNCS(strchr strrchr memchr memcpy memset sysconf sigaction tzset seteuid strftime setrlimit initgroups fstat)
 if test -n "$SECUREWARE"; then
     AC_CHECK_FUNCS(bigcrypt)
     AC_CHECK_FUNCS(set_auth_parameters)

diff --git a/visudo.c b/visudo.c
index 68f69957d9785f532de9686736c501f6210f3cfb..f972e3c5c7bb8e950e5fd5dbd37122a13b7f61d4 100644 (file)
--- a/visudo.c
+++ b/visudo.c
@@ -169,44 +169,44 @@ main(argc, argv)
 #endif /* ENV_EDITOR */
 
     /*
-     * Open sudoers temp file and grab a lock.
+     * Open sudoers, lock it and stat it.  
+     * sudoers_fd must remain open throughout in order to hold the lock.
      */
-    stmp_fd = open(stmp, O_WRONLY | O_CREAT, 0600);
-    if (stmp_fd < 0) {
-       (void) fprintf(stderr, "%s: %s\n", Argv[0], strerror(errno));
-       exit(1);
+    sudoers_fd = open(sudoers, O_RDWR | O_CREAT);
+    if (sudoers_fd == -1) {
+       (void) fprintf(stderr, "%s: %s: %s\n", Argv[0], sudoers,
+           strerror(errno));
+       Exit(-1);
     }
-    if (!lock_file(stmp_fd, SUDO_TLOCK)) {
+    if (!lock_file(sudoers_fd, SUDO_TLOCK)) {
        (void) fprintf(stderr, "%s: sudoers file busy, try again later.\n",
            Argv[0]);
        exit(1);
     }
-#ifdef HAVE_FTRUNCATE
-    if (ftruncate(stmp_fd, 0) == -1) {
+#ifdef HAVE_FSTAT
+    if (fstat(sudoers_fd, &sudoers_sb) == -1) {
 #else
-    if (truncate(stmp, 0) == -1) {
+    if (stat(sudoers, &sudoers_sb) == -1) {
 #endif
-       (void) fprintf(stderr, "%s: can't truncate %s: %s\n", Argv[0],
-           stmp, strerror(errno));
+       (void) fprintf(stderr, "%s: can't stat %s: %s\n",
+           Argv[0], sudoers, strerror(errno));
        Exit(-1);
     }
 
+    /*
+     * Open sudoers temp file.
+     */
+    stmp_fd = open(stmp, O_WRONLY | O_CREAT | O_TRUNC, 0600);
+    if (stmp_fd < 0) {
+       (void) fprintf(stderr, "%s: %s: %s\n", Argv[0], stmp, strerror(errno));
+       exit(1);
+    }
+
     /* Install signal handlers to clean up stmp if we are killed. */
     setup_signals();
 
-    (void) memset(&sudoers_sb, 0, sizeof(sudoers_sb));
-    if (stat(sudoers, &sudoers_sb) == -1 && errno != ENOENT) {
-       (void) fprintf(stderr, "%s: %s\n", Argv[0], strerror(errno));
-       Exit(-1);
-    }
-    sudoers_fd = open(sudoers, O_RDONLY);
-    if (sudoers_fd == -1 && errno != ENOENT) {
-       (void) fprintf(stderr, "%s: %s\n", Argv[0], strerror(errno));
-       Exit(-1);
-    }
-
     /* Copy sudoers -> stmp and reset the mtime */
-    if (sudoers_fd != -1) {
+    if (sudoers_sb.st_size) {
        while ((n = read(sudoers_fd, buf, sizeof(buf))) > 0)
            if (write(stmp_fd, buf, n) != n) {
                (void) fprintf(stderr, "%s: Write failed: %s\n", Argv[0],
@@ -214,10 +214,10 @@ main(argc, argv)
                Exit(-1);
            }
 
-       (void) close(sudoers_fd);
-    }
-    (void) close(stmp_fd);
-    (void) touch(stmp, sudoers_sb.st_mtime);
+       (void) close(stmp_fd);
+       (void) touch(stmp, sudoers_sb.st_mtime);
+    } else
+       (void) close(stmp_fd);
 
     /*
      * Edit the temp file and parse it (for sanity checking)
@@ -369,7 +369,7 @@ main(argc, argv)
        }
     }
 
-    return(0);
+    exit(0);
 }
 
 /*