backup: prevent a symlink attack by operating on the file descriptor

Use futimens() instead of utime() to change the timestamps on a backup
file.  Otherwise, a non-privileged user could create an arbitrary symlink
with the name of the backup file and in this way fool a privileged user
to call utime() on the attacker-chosen file.

Import the relevant gnulib module to make sure futimens() is available.
This commit is contained in:
Kamil Dudka 2017-04-04 09:29:31 +02:00 committed by Benno Schulenberg
parent 8f2b5bbf3d
commit 70bcf752dc
3 changed files with 16 additions and 11 deletions

View File

@ -5,6 +5,7 @@ gnulib_url="git://git.sv.gnu.org/gnulib.git"
gnulib_hash="4084b3a1094372b960ce4a97634e08f4538c8bdd" gnulib_hash="4084b3a1094372b960ce4a97634e08f4538c8bdd"
modules=" modules="
futimens
getdelim getdelim
getline getline
getopt-gnu getopt-gnu

View File

@ -1541,12 +1541,14 @@ void init_backup_dir(void)
/* Read from inn, write to out. We assume inn is opened for reading, /* Read from inn, write to out. We assume inn is opened for reading,
* and out for writing. We return 0 on success, -1 on read error, or -2 * and out for writing. We return 0 on success, -1 on read error, or -2
* on write error. */ * on write error. inn is always closed by this function, out is closed
int copy_file(FILE *inn, FILE *out) * only if close_out is true. */
int copy_file(FILE *inn, FILE *out, bool close_out)
{ {
int retval = 0; int retval = 0;
char buf[BUFSIZ]; char buf[BUFSIZ];
size_t charsread; size_t charsread;
int (*flush_out_fnc)(FILE *) = (close_out) ? fclose : fflush;
assert(inn != NULL && out != NULL && inn != out); assert(inn != NULL && out != NULL && inn != out);
@ -1564,7 +1566,7 @@ int copy_file(FILE *inn, FILE *out)
if (fclose(inn) == EOF) if (fclose(inn) == EOF)
retval = -1; retval = -1;
if (fclose(out) == EOF) if (flush_out_fnc(out) == EOF)
retval = -2; retval = -2;
return retval; return retval;
@ -1655,13 +1657,13 @@ bool write_file(const char *name, FILE *f_open, bool tmp,
int backup_fd; int backup_fd;
FILE *backup_file; FILE *backup_file;
char *backupname; char *backupname;
struct utimbuf filetime; static struct timespec filetime[2];
int copy_status; int copy_status;
int backup_cflags; int backup_cflags;
/* Save the original file's access and modification times. */ /* Save the original file's access and modification times. */
filetime.actime = openfile->current_stat->st_atime; filetime[0].tv_sec = openfile->current_stat->st_atime;
filetime.modtime = openfile->current_stat->st_mtime; filetime[1].tv_sec = openfile->current_stat->st_mtime;
if (f_open == NULL) { if (f_open == NULL) {
/* Open the original file to copy to the backup. */ /* Open the original file to copy to the backup. */
@ -1790,7 +1792,7 @@ bool write_file(const char *name, FILE *f_open, bool tmp,
#endif #endif
/* Copy the file. */ /* Copy the file. */
copy_status = copy_file(f, backup_file); copy_status = copy_file(f, backup_file, FALSE);
if (copy_status != 0) { if (copy_status != 0) {
statusline(ALERT, _("Error reading %s: %s"), realname, statusline(ALERT, _("Error reading %s: %s"), realname,
@ -1799,7 +1801,8 @@ bool write_file(const char *name, FILE *f_open, bool tmp,
} }
/* And set its metadata. */ /* And set its metadata. */
if (utime(backupname, &filetime) == -1 && !ISSET(INSECURE_BACKUP)) { if (futimens(backup_fd, filetime) == -1 && !ISSET(INSECURE_BACKUP)) {
fclose(backup_file);
if (prompt_failed_backupwrite(backupname)) if (prompt_failed_backupwrite(backupname))
goto skip_backup; goto skip_backup;
statusline(HUSH, _("Error writing backup file %s: %s"), statusline(HUSH, _("Error writing backup file %s: %s"),
@ -1811,6 +1814,7 @@ bool write_file(const char *name, FILE *f_open, bool tmp,
goto cleanup_and_exit; goto cleanup_and_exit;
} }
fclose(backup_file);
free(backupname); free(backupname);
} }
@ -1867,7 +1871,7 @@ bool write_file(const char *name, FILE *f_open, bool tmp,
} }
} }
if (f_source == NULL || copy_file(f_source, f) != 0) { if (f_source == NULL || copy_file(f_source, f, TRUE) != 0) {
statusline(ALERT, _("Error writing temp file: %s"), statusline(ALERT, _("Error writing temp file: %s"),
strerror(errno)); strerror(errno));
unlink(tempname); unlink(tempname);
@ -1975,7 +1979,7 @@ bool write_file(const char *name, FILE *f_open, bool tmp,
goto cleanup_and_exit; goto cleanup_and_exit;
} }
if (copy_file(f_source, f) == -1) { if (copy_file(f_source, f, TRUE) == -1) {
statusline(ALERT, _("Error writing %s: %s"), realname, statusline(ALERT, _("Error writing %s: %s"), realname,
strerror(errno)); strerror(errno));
goto cleanup_and_exit; goto cleanup_and_exit;

View File

@ -298,7 +298,7 @@ void init_backup_dir(void);
int delete_lockfile(const char *lockfilename); int delete_lockfile(const char *lockfilename);
int write_lockfile(const char *lockfilename, const char *origfilename, bool modified); int write_lockfile(const char *lockfilename, const char *origfilename, bool modified);
#endif #endif
int copy_file(FILE *inn, FILE *out); int copy_file(FILE *inn, FILE *out, bool close_out);
bool write_file(const char *name, FILE *f_open, bool tmp, bool write_file(const char *name, FILE *f_open, bool tmp,
kind_of_writing_type method, bool nonamechange); kind_of_writing_type method, bool nonamechange);
#ifndef NANO_TINY #ifndef NANO_TINY