writing files securely

[Josip Rodin <joy@cibalia.gkvk.hr>]

Hi,

There was a problem with joe editor overwriting files with a symlink attack
using the DEADJOE file. Wichert fixed this in our package with this:

[...]
  long tim=time(0);
  B *b;
  int fd;
  FILE *f=NULL;
  const char* mode ="a";
  if ((fd=open("DEADJOE",(O_WRONLY|O_APPEND|O_NOFOLLOW)))==-1)
   {
   if ((fd=open("DEADJOE",(O_CREAT|O_EXCL|O_WRONLY),(S_IRUSR|S_IWUSR)))==-1)
    {
    fprintf(stderr,"Cannot save DEADJOE file: %s\n", strerror(errno));
    _exit(1);
    }
   mode="w";
   }
  fprintf(stderr, "will fdopen fd %d with mode %s\n", fd, mode);
  if ((f=fdopen(fd,mode))==NULL)
   {
   fprintf(stderr, "Cannot save DEADJOE file: %s\n", strerror(errno));
   close(fd);
   _exit(1);
   }
  fprintf(f,"\n*** Modified files in JOE when it aborted on %s",ctime(&tim));
[...]

I'm currently reviewing a new version of joe that uses patches from other
sources, and this is the chunk of code they used to fix this issue:

[...]
	long tim = time (0);
	B *b;
	FILE *f;
	int tmpfd;
	struct stat sbuf;

	if ((tmpfd = open ("DEADJOE", O_RDWR | O_EXCL | O_CREAT, 0600)) < 0)
	  {
		  if (lstat ("DEADJOE", &sbuf) < 0)
			  _exit (-1);
		  if (!S_ISREG (sbuf.st_mode) || sbuf.st_uid != geteuid ())
			  _exit (-1);
		  /*
		     A race condition still exists between the lstat() and the open()
		     systemcall, which leads to a possible denial-of-service attack
		     by setting the file access mode to 600 for every file the
		     user executing joe has permissions to.
		     This can't be fixed w/o breacking the behavior of the orig. joe!
		   */
		  if ((tmpfd = open ("DEADJOE", O_RDWR | O_APPEND)) < 0)
			  _exit (-1);
		  if (fchmod (tmpfd, S_IRUSR | S_IWUSR) < 0)
			  _exit (-1);
	  }
	if ((f = fdopen (tmpfd, "a")) == NULL)
		_exit (-1);

	fprintf (f, "\n*** Modified files in JOE when it aborted on %s",
		 ctime (&tim));
[...]

I'm security-impaired :) so I'd like someone to comment on this.
I'm inclined to think Wichert's fix is better, esp. since it doesn't have
any amusing comments... but maybe that's just security through obscurity ;)

TIA.

-- 
Digital Electronic Being Intended for Assassination and Nullification