[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Nano-devel] [PATCH] pull in the futimens module from gnulib
From: |
Kamil Dudka |
Subject: |
Re: [Nano-devel] [PATCH] pull in the futimens module from gnulib |
Date: |
Mon, 03 Apr 2017 08:29:31 +0200 |
User-agent: |
KMail/4.14.10 (Linux/4.9.16-gentoo; KDE/4.14.29; x86_64; ; ) |
Hi Benno,
On Sunday, April 02, 2017 18:01:47 Benno Schulenberg wrote:
> Hello Kamil,
>
> On Fri, Mar 31, 2017, at 13:45, Kamil Dudka wrote:
> > ... and use futimens() instead of utime() to prevent a symlink attack
> > while creating a backup file. Otherwise, a non-privileged user could
> > create an arbitrary symlink with name of the backup file and this way
> > fool a privileged user to call utime() on the attacker-chosen file.
>
> How exactly does the use of futimens prevent a symlink attack?
It changes timestamps on the file descriptor, instead of the file name. So,
if the attacker unlinks the backup file and creates a symlink with the same
file name (while the file descriptor is opened), futimens() will still change
timestamps on the backup file. Otherwise, utime() would change timestamps
on the attacker-provided symlink's target.
> You also change the order of things: first setting the metadata
> of the copy and then actually doing the copying.
Are you suggesting that changing the order could cause problems elsewhere?
> Is this an essential part of preventing the attack? Or is this a fix for
> something else?
No, it was just needed for the use of futimens() because copy_file() closes
the file descriptor.
Kamil
> Benno
>
> > /* Save the original file's access and modification times. */
> >
> > - filetime.actime = openfile->current_stat->st_atime;
> > - filetime.modtime = openfile->current_stat->st_mtime;
> > + filetime[/* atime */ 0].tv_sec = openfile->current_stat->st_atime;
> > + filetime[/* mtime */ 1].tv_sec = openfile->current_stat->st_mtime;
> >
> > if (f_open == NULL) {
> >
> > /* Open the original file to copy to the backup. */
> >
> > @@ -1789,17 +1789,8 @@ bool write_file(const char *name, FILE *f_open,
> > bool tmp,>
> > fprintf(stderr, "Backing up %s to %s\n", realname, backupname);
> >
> > #endif
> >
> > - /* Copy the file. */
> > - copy_status = copy_file(f, backup_file);
> > -
> > - if (copy_status != 0) {
> > - statusline(ALERT, _("Error reading %s: %s"), realname,
> > - strerror(errno));
> > - goto cleanup_and_exit;
> > - }
> > -
> > - /* And set its metadata. */
> > - if (utime(backupname, &filetime) == -1 && !ISSET(INSECURE_BACKUP)) {
> > + /* Set backup's file metadata. */
> > + if (futimens(backup_fd, filetime) == -1 && !ISSET(INSECURE_BACKUP)) {
> >
> > if (prompt_failed_backupwrite(backupname))
> >
> > goto skip_backup;
> >
> > statusline(HUSH, _("Error writing backup file %s: %s"),
> >
> > @@ -1811,6 +1802,15 @@ bool write_file(const char *name, FILE *f_open,
> > bool tmp,>
> > goto cleanup_and_exit;
> >
> > }
> >
> > + /* Copy the file. */
> > + copy_status = copy_file(f, backup_file);
> > +
> > + if (copy_status != 0) {
> > + statusline(ALERT, _("Error reading %s: %s"), realname,
> > + strerror(errno));
> > + goto cleanup_and_exit;
> > + }
> > +
> >
> > free(backupname);
> >
> > }