2010-04-09 Chris Allegretta <chrisa@asty.org>

        * files.c (do_writeout): Better security fixes for backup file writing,
          mangled from submission by Dan Rosenberg <dan.j.rosenberg at gmail>.

git-svn-id: svn://svn.savannah.gnu.org/nano/trunk/nano@4496 35c25a1d-7b9e-4130-9fde-d3aeb78583b8

diff --git a/ChangeLog b/ChangeLog
index f6664004d2eeb3cd49285decda3cedbcdc70b333..bffa9456a74bcee694015d6193ed12cd2df3a7a8 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,4 +1,8 @@
-2010-04-02 Chris Allegretta <chrisa@asty.org>
+2010-04-09 Chris Allegretta <chrisa@asty.org>
+       * files.c (do_writeout): Better security fixes for backup file writing, 
+         mangled from submission by Dan Rosenberg <dan.j.rosenberg at gmail>.
+
+2010-04-08 Chris Allegretta <chrisa@asty.org>
        * files.c (do_writeout): Previous fixes should not cause a crash 
          when saving a new file.  Discovered by Mike Frysinger <vapier@gentoo.org>.
 

diff --git a/src/files.c b/src/files.c
index dff741193c52cf440bbe608f79924417ea9ef0ed..29c56d494a18da83b2a4e1dc4a9c4747d316e832 100644 (file)
--- a/src/files.c
+++ b/src/files.c
@@ -1515,11 +1515,11 @@ bool write_file(const char *name, FILE *f_open, bool tmp, append_type
     if (ISSET(BACKUP_FILE) && !tmp && realexists && ((append !=
        OVERWRITE || openfile->mark_set) ||
        openfile->current_stat->st_mtime == st.st_mtime)) {
+       int backup_fd;
        FILE *backup_file;
        char *backupname;
        struct utimbuf filetime;
        int copy_status;
-       struct stat backupst;
 
        /* Save the original file's access and modification times. */
        filetime.actime = openfile->current_stat->st_atime;
@@ -1589,27 +1589,36 @@ bool write_file(const char *name, FILE *f_open, bool tmp, append_type
            sprintf(backupname, "%s~", realname);
        }
 
-       if (stat(backupname, &backupst) != -1 &&
-           (backupst.st_uid != st.st_uid)) {
-           statusbar(_("Error writing backup file %s: File owner mismatch"), backupname,
-               strerror(errno));
+       /* First, unlink any existing backups.  Next, open the backup
+          file with O_CREAT and O_EXCL.  If it succeeds, we
+          have a file descriptor to a new backup file. */
+       if (unlink(backupname) < 0 && errno != ENOENT) {
+           statusbar(_("Error writing backup file %s: %s"), backupname,
+                       strerror(errno));
            free(backupname);
            goto cleanup_and_exit;
        }
 
+       backup_fd = open(backupname, O_WRONLY | O_CREAT | O_APPEND,
+               S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);
+       /* Now we've got a safe file stream.  If the previous open()
+          call failed, this will return NULL. */
+       backup_file = fdopen(backup_fd, "wb");
 
-       /* Open the destination backup file.  Before we write to it, we
-        * set its permissions, so no unauthorized person can read it as
-        * we write. */
-       backup_file = fopen(backupname, "wb");
+       if (backup_fd < 0 || backup_file == NULL) {
+           statusbar(_("Error writing backup file %s: %s"), backupname,
+                       strerror(errno));
+           free(backupname);
+           goto cleanup_and_exit;
+       }
 
-       if (backup_file == NULL || chmod(backupname,
-               openfile->current_stat->st_mode) == -1) {
+       if (fchmod(backup_fd, openfile->current_stat->st_mode) == -1 ||
+           fchown(backup_fd, openfile->current_stat->st_uid,
+                  openfile->current_stat->st_gid) == -1 ) {
            statusbar(_("Error writing backup file %s: %s"), backupname,
                strerror(errno));
            free(backupname);
-           if (backup_file != NULL)
-               fclose(backup_file);
+           fclose(backup_file);
            /* If we can't write to the backup, DONT go on, since
               whatever caused the backup file to fail (e.g. disk
               full may well cause the real file write to fail, which
@@ -1625,10 +1634,7 @@ bool write_file(const char *name, FILE *f_open, bool tmp, append_type
        copy_status = copy_file(f, backup_file);
 
        /* And set its metadata. */
-       if (copy_status != 0 || chown(backupname,
-               openfile->current_stat->st_uid,
-               openfile->current_stat->st_gid) == -1 ||
-               utime(backupname, &filetime) == -1) {
+       if (copy_status != 0  || utime(backupname, &filetime) == -1) {
            if (copy_status == -1) {
                statusbar(_("Error reading %s: %s"), realname,
                        strerror(errno));