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

[Kamil Dudka]

On Monday, April 03, 2017 08:29:31 Kamil Dudka wrote:
> 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?

I will answer myself:  The order of calls may affect the resulting timestamps.  
An improved patch is on the way...

Kamil

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