unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* "Preserve owner and group" on MSDOS/Windows
@ 2005-06-24  9:08 Juanma Barranquero
  2005-06-24 11:45 ` Eli Zaretskii
  2005-06-24 15:07 ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Gaëtan LEURENT
  0 siblings, 2 replies; 20+ messages in thread
From: Juanma Barranquero @ 2005-06-24  9:08 UTC (permalink / raw)


This change fails on Windows (no chown function):

2005-06-23  Richard M. Stallman  <rms@gnu.org>

	* fileio.c (Frename_file): Preserve owner and group, if possible,
	when copying.

I've wrapped it with #ifndef DOS_NT. I don't think any other action is needed.

-- 
                    /L/e/k/t/u

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: "Preserve owner and group" on MSDOS/Windows
  2005-06-24 11:45 ` Eli Zaretskii
@ 2005-06-24 11:02   ` Juanma Barranquero
  0 siblings, 0 replies; 20+ messages in thread
From: Juanma Barranquero @ 2005-06-24 11:02 UTC (permalink / raw)
  Cc: emacs-devel

> I've just installed a change along these lines (and undid yours).

Cool, thanks. Mine was mainly a fix to allow compilation to proceed.

> (A more meaningful implementation of `chown' for w32, that uses
> GetNamedSecurityInfo and SetNamedSecurityInfo, is left as an exercise
> to the reader ;-)

I feared someone was going to say that...

-- 
                    /L/e/k/t/u

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: "Preserve owner and group" on MSDOS/Windows
  2005-06-24  9:08 "Preserve owner and group" on MSDOS/Windows Juanma Barranquero
@ 2005-06-24 11:45 ` Eli Zaretskii
  2005-06-24 11:02   ` Juanma Barranquero
  2005-06-24 15:07 ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Gaëtan LEURENT
  1 sibling, 1 reply; 20+ messages in thread
From: Eli Zaretskii @ 2005-06-24 11:45 UTC (permalink / raw)
  Cc: emacs-devel

> Date: Fri, 24 Jun 2005 11:08:20 +0200
> From: Juanma Barranquero <lekktu@gmail.com>
> 
> This change fails on Windows (no chown function):
> 
> 2005-06-23  Richard M. Stallman  <rms@gnu.org>
> 
> 	* fileio.c (Frename_file): Preserve owner and group, if possible,
> 	when copying.
> 
> I've wrapped it with #ifndef DOS_NT. I don't think any other action is needed.

I don't think this is the best solution.  It is IMHO much better to
add to w32.c a trivial `chown' that does nothing; then the ugly ifdef
can go away.  (The DOS port doesn't need the ifdef anyway, since the
DJGPP library already has such a no-op implementation of `chown'.)

I think system-dependcent ifdef's should be the last resort in such
situations.

I've just installed a change along these lines (and undid yours).

(A more meaningful implementation of `chown' for w32, that uses
GetNamedSecurityInfo and SetNamedSecurityInfo, is left as an exercise
to the reader ;-)

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows)
  2005-06-24  9:08 "Preserve owner and group" on MSDOS/Windows Juanma Barranquero
  2005-06-24 11:45 ` Eli Zaretskii
@ 2005-06-24 15:07 ` Gaëtan LEURENT
  2005-06-24 20:07   ` Eli Zaretskii
                     ` (2 more replies)
  1 sibling, 3 replies; 20+ messages in thread
From: Gaëtan LEURENT @ 2005-06-24 15:07 UTC (permalink / raw)



Juanma Barranquero wrote on 24 Jun 2005 11:08:20 +0200:

> 2005-06-23  Richard M. Stallman  <rms@gnu.org>
>
> 	* fileio.c (Frename_file): Preserve owner and group, if possible,
> 	when copying.

This is done with a call to chown, and I think this is a source of
race-conditions, like the one that was recently discovered in bzip2
(someone could have replaced the file by a link to another file between
Fcopy_file and chown).

I believe we should use fchown instead.

In fileio.c there is also a call to chmod in copy-file which seem to
suffer the same problem. This one is also in emacs 21.4.

I think lisp calls to set-file-modes should also be checked carefully.

-- 
Gaëtan LEURENT

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows)
  2005-06-24 15:07 ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Gaëtan LEURENT
@ 2005-06-24 20:07   ` Eli Zaretskii
  2005-06-24 20:46     ` Race-condition ? Gaëtan LEURENT
  2005-06-24 21:01     ` David Kastrup
  2005-06-26  4:46   ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Richard M. Stallman
  2005-06-26  4:46   ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Richard M. Stallman
  2 siblings, 2 replies; 20+ messages in thread
From: Eli Zaretskii @ 2005-06-24 20:07 UTC (permalink / raw)


> From: gaetan.leurent@ens.fr (=?iso-8859-1?Q?Ga=EBtan?= LEURENT)
> Date: Fri, 24 Jun 2005 17:07:57 +0200
> 
> > 	* fileio.c (Frename_file): Preserve owner and group, if possible,
> > 	when copying.
> 
> This is done with a call to chown, and I think this is a source of
> race-conditions, like the one that was recently discovered in bzip2
> (someone could have replaced the file by a link to another file between
> Fcopy_file and chown).

So?  What problems would that cause, except defeating the call to
chown itself?  Previous versions of Emacs didn't call chown at all, so
how is the current version worse?

It's possible that this race condition is harmful in the context of
bzip2, but that doesn't necessarily mean it's as harmful in Emacs.

> I believe we should use fchown instead.

Only if the danger is real, IMHO: fchown requires that we open the
file, which is expensive.  If we go that way, we might as well check
if we are root, and only open the file and call fchown if we are: no
need to punish mere mortals if we know in advance the call will fail
for them anyway.

> In fileio.c there is also a call to chmod in copy-file which seem to
> suffer the same problem. This one is also in emacs 21.4.

Yeah, and many versions before that.

Anyway, how portable are fchown and fchmod?  If not all platforms
support them, we shouldn't introduce them without an Autoconf test.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ?
  2005-06-24 20:07   ` Eli Zaretskii
@ 2005-06-24 20:46     ` Gaëtan LEURENT
  2005-06-25  9:27       ` Eli Zaretskii
  2005-06-24 21:01     ` David Kastrup
  1 sibling, 1 reply; 20+ messages in thread
From: Gaëtan LEURENT @ 2005-06-24 20:46 UTC (permalink / raw)
  Cc: emacs-devel


Eli Zaretskii wrote on 24 Jun 2005 22:07:35 +0200:

> So?  What problems would that cause, except defeating the call to
> chown itself?  Previous versions of Emacs didn't call chown at all, so
> how is the current version worse?
>
> It's possible that this race condition is harmful in the context of
> bzip2, but that doesn't necessarily mean it's as harmful in Emacs.
>
>> I believe we should use fchown instead.
>
> Only if the danger is real, IMHO: fchown requires that we open the
> file, which is expensive.  If we go that way, we might as well check
> if we are root, and only open the file and call fchown if we are: no
> need to punish mere mortals if we know in advance the call will fail
> for them anyway.

I guess I haven't been clear enough. The scenario is:

- Suppose the users have two directory in two different drives. For
  instance in /home which is a small drive with frequent backups, and in
  /large which is a large drive without backups.

- Now, root is using emacs. He decides to move the file
  /home/joe/somefile to /large/joe/somefile because it takes too much
  place on the small drive.

- Since /home and /large are on different filesystems, emacs will copy
  the content of /home/joe/somefile to /large/joe/somefile and will then
  call chown on /large/joe/somefile.

- But joe is a bad guy, and while emacs is copying the file, he removes
  /large/joe/somefile and replaces it with a hardlink to
  /large/root/importantfile (possibly /etc/passwd if it's on the same
  drive).

- When emacs finishes to copy the file, it call chown on
  /large/joe/somefile, and joe now owns /large/root/importantfile.

The problem is that chown takes a path as argument, and paths are not
safe in a Unix environment. What we must do is use fchown which takes a
filedescriptor as argument, and give it the fd we got *when we opened
the new file in the first place*. We must not close the file before we
call fchown, and we must not open it second time. This induce no
additional cost and it is the only way to be sure we are dealing with
the same file.

You can build the same kind of attack with chmod.

The attack is not very likely because it requires that the target
directory is writable by the victim and by the attacker, which can
happen it root is playing with someone else's file or if the victim is
doing things in a world writable directory, but it the same as the one
that was found in bzip2.

> Anyway, how portable are fchown and fchmod?  If not all platforms
> support them, we shouldn't introduce them without an Autoconf test.

According to Linux's man files:
# The fchmod call conforms to 4.4BSD and SVr4.  SVr4 documents additional
# EINTR and ENOLINK error conditions.  POSIX requires the fchmod function
# if at least one of _POSIX_MAPPED_FILES and _POSIX_SHARED_MEMORY_OBJECTS
# is  defined,  and  documents  additional ENOSYS and EINVAL error condi-
# tions, but does not document EIO.

If a platform does not support them, I think we should not try to change
access permissions and/or file owner at all on these platforms.

-- 
Gaëtan LEURENT

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ?
  2005-06-24 20:07   ` Eli Zaretskii
  2005-06-24 20:46     ` Race-condition ? Gaëtan LEURENT
@ 2005-06-24 21:01     ` David Kastrup
  2005-06-25 22:25       ` Gaëtan LEURENT
  1 sibling, 1 reply; 20+ messages in thread
From: David Kastrup @ 2005-06-24 21:01 UTC (permalink / raw)
  Cc: emacs-devel

Eli Zaretskii <eliz@gnu.org> writes:

>> From: gaetan.leurent@ens.fr (=?iso-8859-1?Q?Ga=EBtan?= LEURENT)
>> Date: Fri, 24 Jun 2005 17:07:57 +0200
>> 
>> > 	* fileio.c (Frename_file): Preserve owner and group, if possible,
>> > 	when copying.
>> 
>> This is done with a call to chown, and I think this is a source of
>> race-conditions, like the one that was recently discovered in bzip2
>> (someone could have replaced the file by a link to another file between
>> Fcopy_file and chown).
>
> So?  What problems would that cause, except defeating the call to
> chown itself?  Previous versions of Emacs didn't call chown at all, so
> how is the current version worse?
>
> It's possible that this race condition is harmful in the context of
> bzip2, but that doesn't necessarily mean it's as harmful in Emacs.
>
>> I believe we should use fchown instead.
>
> Only if the danger is real, IMHO: fchown requires that we open the
> file, which is expensive.  If we go that way, we might as well check
> if we are root, and only open the file and call fchown if we are: no
> need to punish mere mortals if we know in advance the call will fail
> for them anyway.
>
>> In fileio.c there is also a call to chmod in copy-file which seem to
>> suffer the same problem. This one is also in emacs 21.4.
>
> Yeah, and many versions before that.
>
> Anyway, how portable are fchown and fchmod?  If not all platforms
> support them, we shouldn't introduce them without an Autoconf test.

I fail to see the advantage of using chown, or using fopen and
fchown.  In both cases the file name can be changed to refer to
something else before the operation starts.

The only situation where fchown offers any advantage is where you
_already_ have a file open, like when you write the file after fopen,
and then change its permissions.

That is, the owner change must be accomplished in something like
write-region, or it is pointless.  As an isolated operation,
fopen/fchown offers no advantage whatsoever.

You could conceivable to fopen, followed by fstat, check that you are
not talking about a symbolic link, and only then to fchown, and this
would be safe against symlink attacks.

But apart from that, I see no specific advantage.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ?
  2005-06-24 20:46     ` Race-condition ? Gaëtan LEURENT
@ 2005-06-25  9:27       ` Eli Zaretskii
  0 siblings, 0 replies; 20+ messages in thread
From: Eli Zaretskii @ 2005-06-25  9:27 UTC (permalink / raw)


> Cc: emacs-devel@gnu.org
> From: gaetan.leurent@ens.fr (=?iso-8859-1?Q?Ga=EBtan?= LEURENT)
> Date: Fri, 24 Jun 2005 22:46:59 +0200
> 
> I guess I haven't been clear enough. The scenario is:

No, you were very clear.  I still am not sure whether the danger is
real, but then I'm not an expert on security.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ?
  2005-06-24 21:01     ` David Kastrup
@ 2005-06-25 22:25       ` Gaëtan LEURENT
  2005-06-26 18:48         ` David Kastrup
  0 siblings, 1 reply; 20+ messages in thread
From: Gaëtan LEURENT @ 2005-06-25 22:25 UTC (permalink / raw)
  Cc: Eli Zaretskii, emacs-devel


David Kastrup wrote on 24 Jun 2005 23:01:50 +0200:

> I fail to see the advantage of using chown, or using fopen and
> fchown.  In both cases the file name can be changed to refer to
> something else before the operation starts.
>
> The only situation where fchown offers any advantage is where you
> _already_ have a file open, like when you write the file after fopen,
> and then change its permissions.

Yes, that is true. But we are exactly in the situation were fchown is
useful.

> That is, the owner change must be accomplished in something like
> write-region, or it is pointless.  As an isolated operation,
> fopen/fchown offers no advantage whatsoever.

Yes, fopen/fchown would be the same as calling chown. But here we have
just created a new file, and we want to change it's owner, so this
should be done with fchown.

The code is:

|  Fcopy_file (file, newname,
|                /* We have already prompted if it was an integer,
|                   so don't have copy-file prompt again.  */
|                NILP (ok_if_already_exists) ? Qnil : Qt,
|		Qt, Qnil);
|
|  /* Preserve owner and group, if possible (if we are root).  */
|  if (stat (SDATA (encoded_file), &data) >= 0)
|    chown (SDATA (encoded_file), data.st_uid, data.st_gid);
|
|  Fdelete_file (file);

So, we first open a file and write something in it inside Fcopy_file,
then we call chown on the same path. But the file that is at this path
could be something else that what the we created in Fcopy_file. We
should keep that file open, and call fchown on its filedescriptor,
instead of calling chown on it's path.

This will need some change in Fcopy_file, to be able to do something
with the filedescriptor before the file is closed; the most easy way
seems to make a subroutine that does the job of Fcopy_file and returns
the fd before closing it, and use this subroutine both in Fcopy_file and
here in Frename_file.


We also have the same kind of thing inside Fcopy_file (and this one is
also in stable emacs) where we call chmod on the file path just after
closing the file.
[I have replaced some part of the code with //... to make it shorter]

|  #ifdef VMS
|  // ...
|  #else
|  #ifdef MSDOS
|  // ...
|  #else  /* not MSDOS */
|    ofd = emacs_open (SDATA (encoded_newname),
|  		    O_WRONLY | O_TRUNC | O_CREAT
|  		    | (EQ (mustbenew, Qexcl) ? O_EXCL : 0),
|  		    0666);
|  #endif /* not MSDOS */
|  #endif /* VMS */
|    if (ofd < 0)
|      report_file_error ("Opening output file", Fcons (newname, Qnil));
|  
|    record_unwind_protect (close_file_unwind, make_number (ofd));
|  
|    immediate_quit = 1;
|    QUIT;
|    while ((n = emacs_read (ifd, buf, sizeof buf)) > 0)
|      if (emacs_write (ofd, buf, n) != n)
|        report_file_error ("I/O error", Fcons (newname, Qnil));
|    immediate_quit = 0;
|  
|    /* Closing the output clobbers the file times on some systems.  */
|    if (emacs_close (ofd) < 0)
|      report_file_error ("I/O error", Fcons (newname, Qnil));
|  
|    if (input_file_statable_p)
|      {
|        if (!NILP (keep_time))
|  	{
|  		// ...
|  	}
|  #ifndef MSDOS
|        chmod (SDATA (encoded_newname), st.st_mode & 07777);
|  #else /* MSDOS */
|  // ...
|  #endif /* MSDOS */
|      }

Here the change is very easy: we should just do fchmod(ofd,...) before
emacs_close(ofd).

-- 
Gaëtan LEURENT

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows)
  2005-06-24 15:07 ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Gaëtan LEURENT
  2005-06-24 20:07   ` Eli Zaretskii
@ 2005-06-26  4:46   ` Richard M. Stallman
  2005-06-26 19:52     ` Race-condition ? Gaëtan LEURENT
  2005-06-26  4:46   ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Richard M. Stallman
  2 siblings, 1 reply; 20+ messages in thread
From: Richard M. Stallman @ 2005-06-26  4:46 UTC (permalink / raw)
  Cc: emacs-devel

    I think lisp calls to set-file-modes should also be checked carefully.

Would you like to check some of them?  The crucial question is, does a
call to set-file-modes introduce a worse problem than what existed
anyway.  For instance, if someone could put a hardlink where you will
chmod it, could he also put a hardlink where you will write the
contents of the file?  In cases where the latter problem exists,
adding the former problem to it does not really make things any worse.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows)
  2005-06-24 15:07 ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Gaëtan LEURENT
  2005-06-24 20:07   ` Eli Zaretskii
  2005-06-26  4:46   ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Richard M. Stallman
@ 2005-06-26  4:46   ` Richard M. Stallman
  2005-06-26 17:12     ` Race-condition ? Gaëtan LEURENT
  2005-06-26 18:59     ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Eli Zaretskii
  2 siblings, 2 replies; 20+ messages in thread
From: Richard M. Stallman @ 2005-06-26  4:46 UTC (permalink / raw)
  Cc: emacs-devel

How about this fix?

*** fileio.c	24 Jun 2005 15:43:20 -0400	1.547
--- fileio.c	25 Jun 2005 16:56:28 -0400	
***************
*** 2406,2412 ****
    return;
  }
  
! DEFUN ("copy-file", Fcopy_file, Scopy_file, 2, 5,
         "fCopy file: \nGCopy %s to file: \np\nP",
         doc: /* Copy FILE to NEWNAME.  Both args must be strings.
  If NEWNAME names a directory, copy FILE there.
--- 2406,2412 ----
    return;
  }
  
! DEFUN ("copy-file", Fcopy_file, Scopy_file, 2, 6,
         "fCopy file: \nGCopy %s to file: \np\nP",
         doc: /* Copy FILE to NEWNAME.  Both args must be strings.
  If NEWNAME names a directory, copy FILE there.
***************
*** 2426,2434 ****
  that means to get an error if the file already exists; never overwrite.
  If MUSTBENEW is neither nil nor `excl', that means ask for
  confirmation before overwriting, but do go ahead and overwrite the file
! if the user confirms.  */)
!   (file, newname, ok_if_already_exists, keep_time, mustbenew)
       Lisp_Object file, newname, ok_if_already_exists, keep_time, mustbenew;
  {
    int ifd, ofd, n;
    char buf[16 * 1024];
--- 2426,2438 ----
  that means to get an error if the file already exists; never overwrite.
  If MUSTBENEW is neither nil nor `excl', that means ask for
  confirmation before overwriting, but do go ahead and overwrite the file
! if the user confirms.
! 
! If PRESERVE-UID-GID is non-nil, we try to transfer the
! uid and gid of FILE to NEWNAME.  */)
!   (file, newname, ok_if_already_exists, keep_time, mustbenew, preserve_uid_gid)
       Lisp_Object file, newname, ok_if_already_exists, keep_time, mustbenew;
+      Lisp_Object preserve_uid_gid;
  {
    int ifd, ofd, n;
    char buf[16 * 1024];
***************
*** 2570,2575 ****
--- 2574,2599 ----
        report_file_error ("I/O error", Fcons (newname, Qnil));
    immediate_quit = 0;
  
+   /* Preserve the owner and group, if requested.  */
+   if (input_file_statable_p && ! NILP (preserve_uid_gid))
+     fchown (ofd, st.st_uid, st.st_gid);
+ 
+   if (input_file_statable_p)
+     {
+ #ifndef MSDOS
+       fchmod (ofd, st.st_mode & 07777);
+ #else /* MSDOS */
+ #if defined (__DJGPP__) && __DJGPP__ > 1
+       /* In DJGPP v2.0 and later, fstat usually returns true file mode bits,
+          and if it can't, it tells so.  Otherwise, under MSDOS we usually
+          get only the READ bit, which will make the copied file read-only,
+          so it's better not to chmod at all.  */
+       if ((_djstat_flags & _STFAIL_WRITEBIT) == 0)
+ 	chmod (SDATA (encoded_newname), st.st_mode & 07777);
+ #endif /* DJGPP version 2 or newer */
+ #endif /* MSDOS */
+     }
+ 
    /* Closing the output clobbers the file times on some systems.  */
    if (emacs_close (ofd) < 0)
      report_file_error ("I/O error", Fcons (newname, Qnil));
***************
*** 2587,2604 ****
  		     Fcons (build_string ("Cannot set file date"),
  			    Fcons (newname, Qnil)));
  	}
- #ifndef MSDOS
-       chmod (SDATA (encoded_newname), st.st_mode & 07777);
- #else /* MSDOS */
- #if defined (__DJGPP__) && __DJGPP__ > 1
-       /* In DJGPP v2.0 and later, fstat usually returns true file mode bits,
-          and if it can't, it tells so.  Otherwise, under MSDOS we usually
-          get only the READ bit, which will make the copied file read-only,
-          so it's better not to chmod at all.  */
-       if ((_djstat_flags & _STFAIL_WRITEBIT) == 0)
- 	chmod (SDATA (encoded_newname), st.st_mode & 07777);
- #endif /* DJGPP version 2 or newer */
- #endif /* MSDOS */
      }
  
    emacs_close (ifd);
--- 2611,2616 ----
***************
*** 2775,2781 ****
      {
        if (errno == EXDEV)
  	{
- 	  struct stat data;
  #ifdef S_IFLNK
            symlink_target = Ffile_symlink_p (file);
            if (! NILP (symlink_target))
--- 2787,2792 ----
***************
*** 2783,2797 ****
                                   NILP (ok_if_already_exists) ? Qnil : Qt);
            else
  #endif
!             Fcopy_file (file, newname,
!                         /* We have already prompted if it was an integer,
!                            so don't have copy-file prompt again.  */
!                         NILP (ok_if_already_exists) ? Qnil : Qt,
! 			Qt, Qnil);
! 
! 	  /* Preserve owner and group, if possible (if we are root).  */
! 	  if (stat (SDATA (encoded_file), &data) >= 0)
! 	    chown (SDATA (encoded_file), data.st_uid, data.st_gid);
  
  	  Fdelete_file (file);
  	}
--- 2794,2804 ----
                                   NILP (ok_if_already_exists) ? Qnil : Qt);
            else
  #endif
! 	    Fcopy_file (file, newname,
! 			/* We have already prompted if it was an integer,
! 			   so don't have copy-file prompt again.  */
! 			NILP (ok_if_already_exists) ? Qnil : Qt,
! 			Qt, Qnil, Qt);
  
  	  Fdelete_file (file);
  	}

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ?
  2005-06-26  4:46   ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Richard M. Stallman
@ 2005-06-26 17:12     ` Gaëtan LEURENT
  2005-06-26 18:59     ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Eli Zaretskii
  1 sibling, 0 replies; 20+ messages in thread
From: Gaëtan LEURENT @ 2005-06-26 17:12 UTC (permalink / raw)
  Cc: emacs-devel


Richard M. Stallman wrote on 26 Jun 2005 06:46:54 +0200:

> How about this fix?

It seems fine to me.

-- 
Gaëtan LEURENT

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ?
  2005-06-25 22:25       ` Gaëtan LEURENT
@ 2005-06-26 18:48         ` David Kastrup
  0 siblings, 0 replies; 20+ messages in thread
From: David Kastrup @ 2005-06-26 18:48 UTC (permalink / raw)
  Cc: Eli Zaretskii

gaetan.leurent@ens.fr (Gaëtan LEURENT) writes:

> David Kastrup wrote on 24 Jun 2005 23:01:50 +0200:
>
>> I fail to see the advantage of using chown, or using fopen and
>> fchown.  In both cases the file name can be changed to refer to
>> something else before the operation starts.
>>
>> The only situation where fchown offers any advantage is where you
>> _already_ have a file open, like when you write the file after fopen,
>> and then change its permissions.
>
> Yes, that is true. But we are exactly in the situation were fchown is
> useful.

I saw the detailed description afterwards.  You are right.  fchown
would be desirable to use where available.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows)
  2005-06-26  4:46   ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Richard M. Stallman
  2005-06-26 17:12     ` Race-condition ? Gaëtan LEURENT
@ 2005-06-26 18:59     ` Eli Zaretskii
  1 sibling, 0 replies; 20+ messages in thread
From: Eli Zaretskii @ 2005-06-26 18:59 UTC (permalink / raw)
  Cc: gaetan.leurent, emacs-devel

> From: "Richard M. Stallman" <rms@gnu.org>
> Date: Sun, 26 Jun 2005 00:46:54 -0400
> Cc: emacs-devel@gnu.org
> 
> How about this fix?
> 
> *** fileio.c	24 Jun 2005 15:43:20 -0400	1.547
> --- fileio.c	25 Jun 2005 16:56:28 -0400	

This change will do Bad Things on MSDOS, because it calls chmod
before closing the file.

And fchown needs a similar trick on w32 as did chown.

I will fix these problems unless someone beats me to it.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ?
  2005-06-26  4:46   ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Richard M. Stallman
@ 2005-06-26 19:52     ` Gaëtan LEURENT
  2005-06-27  5:38       ` Richard M. Stallman
  0 siblings, 1 reply; 20+ messages in thread
From: Gaëtan LEURENT @ 2005-06-26 19:52 UTC (permalink / raw)
  Cc: emacs-devel


Richard M. Stallman wrote on 26 Jun 2005 06:46:13 +0200:

> Would you like to check some of them?

I'll look at it.

> The crucial question is, does a call to set-file-modes introduce a
> worse problem than what existed anyway.  For instance, if someone
> could put a hardlink where you will chmod it, could he also put a
> hardlink where you will write the contents of the file?

Yes, that needs to be checked in each case.

In the case of copy_file, someone could put a hardlink when you write
the file, but then emacs should warn you that the file already exist
(you should never overwrite files in public writable directories
because it is unsafe -- you must first delete the file).

In fact, by looking again at the code of Fcopy_file, I see that the way
emacs does it is also vulnerable to race-conditions attack: we first
check if the file exist with barf_or_query_if_file_exists, and then we
open the file (it is also done in a few other functions in fileio.c). If
the file was created in-between, it will be overwritten without any
warning. The correct way to do this is to open the file with
O_CREAT|O_EXCL and ask the user what to do if it fails with EEXIST. As
far as I know, it's the only way to check for the existence of a file
and create it atomically.

I don't know how we should fix this one, maybe by replacing
barf_or_query_if_file_exists with some
open_and_barf_or_query_if_file_exists ?

-- 
Gaëtan LEURENT

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ?
  2005-06-26 19:52     ` Race-condition ? Gaëtan LEURENT
@ 2005-06-27  5:38       ` Richard M. Stallman
  2005-06-28 22:57         ` Gaëtan LEURENT
  0 siblings, 1 reply; 20+ messages in thread
From: Richard M. Stallman @ 2005-06-27  5:38 UTC (permalink / raw)
  Cc: emacs-devel

    I don't know how we should fix this one, maybe by replacing
    barf_or_query_if_file_exists with some
    open_and_barf_or_query_if_file_exists ?

I think it may be too difficult to fix all the places
that do this, but would you like to try fixing one or two
as a first step?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ?
  2005-06-27  5:38       ` Richard M. Stallman
@ 2005-06-28 22:57         ` Gaëtan LEURENT
  2005-06-28 23:50           ` Lennart Borgman
  2005-07-03 15:49           ` Richard M. Stallman
  0 siblings, 2 replies; 20+ messages in thread
From: Gaëtan LEURENT @ 2005-06-28 22:57 UTC (permalink / raw)
  Cc: emacs-devel


Richard M. Stallman wrote on 27 Jun 2005 07:38:04 +0200:

> I think it may be too difficult to fix all the places
> that do this, but would you like to try fixing one or two
> as a first step?

In fact, doing the right thing is not so obvious as I thought. I've
looked at various implementations of mv and cp (from GNU, OpenSolaris,
OpenBSD and FreeBSD) and at the Single Unix Specification, and almost
each one uses a subtly different way, and they all allow some kind of
race-condition. Some (including GNU mv and GNU cp) do even use chown and
chmod in the same unsafe way that we did previously, so I will report
that to them.

Now, as for as I can tell, there is no way to move a file asking the
user what to do if needed and avoid every race-conditions in a Unix-like
OS. I will try to come with a solution that avoids the worse problems.

An other question is to decide what to do when the destination file
exists: we can either overwrite it or remove it and create a new
one. Those two ways will have different effects if the file we
overwrite/remove has a link count > 1. Current emacs implementation use
"overwrite mode" in copy and "remove mode" in rename if it is in the
same filesystem, but cross-filesystem rename use the "overwrite
mode". We should maybe do something more consistent. FWIW, the Single
Unix specification asks that mv removes the file, and cp overwrites it
(but some implementation don't follow strictly this rule).

In the case of copy, I think we can do it without race-conditions using
open in O_EXCL|O_CREAT mode, and if it fails, using unlink and trying
again. That would mean to use the "remove mode". In fact the "overwrite
mode" is unsafe as soon as someone can replace the file with a hardlink
to something else.

-- 
Gaëtan LEURENT

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ?
  2005-06-28 22:57         ` Gaëtan LEURENT
@ 2005-06-28 23:50           ` Lennart Borgman
  2005-06-30  5:42             ` David Kastrup
  2005-07-03 15:49           ` Richard M. Stallman
  1 sibling, 1 reply; 20+ messages in thread
From: Lennart Borgman @ 2005-06-28 23:50 UTC (permalink / raw)


Gaëtan LEURENT wrote:

>Now, as for as I can tell, there is no way to move a file asking the
>user what to do if needed and avoid every race-conditions in a Unix-like
>OS. I will try to come with a solution that avoids the worse problems.
>  
>
That reminds me of that I long ago read someone who found that the most 
interesting piece of programming he had ever done was for implementing 
file move. Unfortunately I do not remember who or where I read this - of 
course...

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ?
  2005-06-28 23:50           ` Lennart Borgman
@ 2005-06-30  5:42             ` David Kastrup
  0 siblings, 0 replies; 20+ messages in thread
From: David Kastrup @ 2005-06-30  5:42 UTC (permalink / raw)
  Cc: emacs-devel

Lennart Borgman <lennart.borgman.073@student.lu.se> writes:

> Gaëtan LEURENT wrote:
>
>>Now, as for as I can tell, there is no way to move a file asking the
>>user what to do if needed and avoid every race-conditions in a Unix-like
>>OS. I will try to come with a solution that avoids the worse problems.
>>  
>>
> That reminds me of that I long ago read someone who found that the
> most interesting piece of programming he had ever done was for
> implementing file move. Unfortunately I do not remember who or where I
> read this - of course...

movemail in Emacs?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Race-condition ?
  2005-06-28 22:57         ` Gaëtan LEURENT
  2005-06-28 23:50           ` Lennart Borgman
@ 2005-07-03 15:49           ` Richard M. Stallman
  1 sibling, 0 replies; 20+ messages in thread
From: Richard M. Stallman @ 2005-07-03 15:49 UTC (permalink / raw)
  Cc: emacs-devel

    Some (including GNU mv and GNU cp) do even use chown and
    chmod in the same unsafe way that we did previously, so I will report
    that to them.

Thank you.

    Now, as for as I can tell, there is no way to move a file asking the
    user what to do if needed and avoid every race-conditions in a Unix-like
    OS. I will try to come with a solution that avoids the worse problems.

That is good.

     Those two ways will have different effects if the file we
    overwrite/remove has a link count > 1. Current emacs implementation use
    "overwrite mode" in copy and "remove mode" in rename if it is in the
    same filesystem,

Those are clearly the right things.  copy-file is defined to overwrite
an existing file; it would be wrong to do anything else (at least in
the default case).  Rename, in the case where it is implemented by the
rename system call, will replace the existing file.

		     but cross-filesystem rename use the "overwrite
    mode".

In principle, that ought to replace the existing file, to be
consistent with other cases of renaming.

The easiest way might be to create an option (or reuse an existing
option) in copy-file, so that rename-file can continue to handle this
case by calling copy-file.

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2005-07-03 15:49 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2005-06-24  9:08 "Preserve owner and group" on MSDOS/Windows Juanma Barranquero
2005-06-24 11:45 ` Eli Zaretskii
2005-06-24 11:02   ` Juanma Barranquero
2005-06-24 15:07 ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Gaëtan LEURENT
2005-06-24 20:07   ` Eli Zaretskii
2005-06-24 20:46     ` Race-condition ? Gaëtan LEURENT
2005-06-25  9:27       ` Eli Zaretskii
2005-06-24 21:01     ` David Kastrup
2005-06-25 22:25       ` Gaëtan LEURENT
2005-06-26 18:48         ` David Kastrup
2005-06-26  4:46   ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Richard M. Stallman
2005-06-26 19:52     ` Race-condition ? Gaëtan LEURENT
2005-06-27  5:38       ` Richard M. Stallman
2005-06-28 22:57         ` Gaëtan LEURENT
2005-06-28 23:50           ` Lennart Borgman
2005-06-30  5:42             ` David Kastrup
2005-07-03 15:49           ` Richard M. Stallman
2005-06-26  4:46   ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Richard M. Stallman
2005-06-26 17:12     ` Race-condition ? Gaëtan LEURENT
2005-06-26 18:59     ` Race-condition ? (was: "Preserve owner and group" on MSDOS/Windows) Eli Zaretskii

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).