[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 10:08:20 +0200 |
User-agent: |
KMail/4.14.10 (Linux/4.9.16-gentoo; KDE/4.14.29; x86_64; ; ) |
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);
> > >
> > > }