Re: [Nano-devel] [PATCH] pull in the futimens module from gnulib

[Kamil Dudka]

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);
> >     
> >      }