many packages write to `temporary-file-directory' insecurely

many packages write to `temporary-file-directory' insecurely

[Colin Walters]

Hi,

I discovered a security problem with M-x snake, and a number of other
packages. For example, snake writes "snake-scores" to
`temporary-file-directory' (which defaults to /tmp on my system).  If an
attacker creates a symlink /tmp/snake-scores -> /home/luser/.bashrc, and
"luser" later runs M-x snake, then their .bashrc will be happily
overwritten with their snake scores.  Try it.

After a quick grep through the Emacs source, terminal.el looks like it
does something similar in the function `te-create-terminfo'.  And eshell
appears to use `make-temp-name' insecurely in the function
`eshell-parse-variable-ref'., although it is difficult to follow the
code.  And there are a number of others that I haven't investigated too
closely.  Calc was creating a temporary gnuplot file insecurely; I've
just fixed it.  These all *must* be fixed.  

I gather that there have been reports about this problem in the past,
and this was the reason `make-temp-file' was introduced to replace
`make-temp-name'.  If you maintain a package that creates temporary
files, please make sure you are using `make-temp-file'!


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Pavel Janík]

   From: Colin Walters <walters@verbum.org>
   Date: 28 Feb 2002 20:15:51 -0500

   > I discovered a security problem with M-x snake, and a number of other
   > packages. For example, snake writes "snake-scores" to
   > `temporary-file-directory' (which defaults to /tmp on my system).  If an
   > attacker creates a symlink /tmp/snake-scores -> /home/luser/.bashrc, and
   > "luser" later runs M-x snake, then their .bashrc will be happily
   > overwritten with their snake scores.  Try it.

The problem is actually in gamegrid.el's gamegrid-add-score. We should not
write to file if it is symlink or hard link. Am I right?

--- gamegrid.el.~1.4.~	Wed Feb  6 13:39:50 2002
+++ gamegrid.el	Sat Mar  2 12:51:15 2002
@@ -405,26 +405,29 @@
 ;; ;;;;;;;;;;;;;;; high score functions ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (defun gamegrid-add-score (file score)
-  "Add the current score to the high score file."
-  (save-excursion
-  (find-file-other-window file)
-  (setq buffer-read-only nil)
-  (goto-char (point-max))
-  (insert (format "%05d\t%s\t%s <%s>\n"
-		  score
-		  (current-time-string)
-		  (user-full-name)
-		  (cond ((fboundp 'user-mail-address)
-			 (user-mail-address))
-			((boundp 'user-mail-address)
-			 user-mail-address)
-			(t ""))))
-  (sort-numeric-fields 1 (point-min) (point-max))
-    (reverse-region (point-min) (point-max))
-  (goto-line (1+ gamegrid-score-file-length))
-  (delete-region (point) (point-max))
-  (setq buffer-read-only t)
-    (save-buffer)))
+  "Add the current score to the high score file.
+If the high score file is a symlink or hard link, do nothing."
+  (unless (or (file-symlink-p file)
+	      (> (or (file-nlinks file) 0) 1))
+    (save-excursion
+      (find-file-other-window file)
+      (setq buffer-read-only nil)
+      (goto-char (point-max))
+      (insert (format "%05d\t%s\t%s <%s>\n"
+		      score
+		      (current-time-string)
+		      (user-full-name)
+		      (cond ((fboundp 'user-mail-address)
+			     (user-mail-address))
+			    ((boundp 'user-mail-address)
+			     user-mail-address)
+			    (t ""))))
+      (sort-numeric-fields 1 (point-min) (point-max))
+      (reverse-region (point-min) (point-max))
+      (goto-line (1+ gamegrid-score-file-length))
+      (delete-region (point) (point-max))
+      (setq buffer-read-only t)
+      (save-buffer))))
 
 ;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 

-- 
Pavel Janík

die_if_kernel("Whee... Hello Mr. Penguin", current->tss.kregs);
                  -- 2.2.16 arch/sparc/kernel/traps.c

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Colin Walters]

On Sat, 2002-03-02 at 06:52, Pavel Janík wrote:
> +  (unless (or (file-symlink-p file)
> +	      (> (or (file-nlinks file) 0) 1))
> +    (save-excursion
> +      (find-file-other-window file)

This won't work.  There is a race condition in between testing whether
or not the file is a symlink, and actually writing the file (which
happens in `save-buffer', far below).

The right way to do this, I think, is to put the score files in the
user's ~/.emacs.d directory.  Does anyone object to expanding the use of
the .emacs.d directory in this way?


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Pavel Janík]

   From: Colin Walters <walters@verbum.org>
   Date: 02 Mar 2002 16:12:30 -0500

   > The right way to do this, I think, is to put the score files in the
   > user's ~/.emacs.d directory.  Does anyone object to expanding the use of
   > the .emacs.d directory in this way?

How do you share high-scores between users then? We do want to share
scores...
-- 
Pavel Janík

pavel:x:500:100:Pavel Janík:/home/pavel:/usr/bin/emacs

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

    The problem is actually in gamegrid.el's gamegrid-add-score. We should not
    write to file if it is symlink or hard link. Am I right?

That is not guaranteed to prevent the problem, since someone could
create a symlink in between the testing and the writing.

It seems to me that we should always use make-temp-file for writing
into /tmp.  For now, I changed snake.el to specify a file in your
home dir.  It could also be a file in /var, if someone set up the file
in advance to make sure it can't be deleted, just edited.


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Stefan Monnier]

>    From: Colin Walters <walters@verbum.org>
>    Date: 02 Mar 2002 16:12:30 -0500
> 
>    > The right way to do this, I think, is to put the score files in the
>    > user's ~/.emacs.d directory.  Does anyone object to expanding the use of
>    > the .emacs.d directory in this way?
> 
> How do you share high-scores between users then? We do want to share
> scores...

It's difficult to do it safely.
We could try to allow sharing in the case where the parent dir of the
shared file is only writable by root.

But in any case sharing should not be done via /tmp.
We should instead define a `score-files-directory' which could default
to "/var/games" or to "~/.emacs.d".


	Stefan


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Al Petrofsky]

> From: "Stefan Monnier" <monnier+gnu/emacs@RUM.cs.yale.edu>
> >    From: Colin Walters <walters@verbum.org>
> > 
> > How do you share high-scores between users then? We do want to share
> > scores...
> 
> It's difficult to do it safely.

Is this sufficient?

   (let* ((scores-dir (expand-file-name "emacs-games-scores"
					temporary-file-directory))
	  (scores-basename "snake-scores")
	  (scores-file (expand-file-name scores-basename scores-dir))
	  (temp (make-temp-file scores-basename)))
     (unwind-protect
	 (progn 
	   (write-region (point-min) (point-max) temp)
	   (set-file-modes temp #o444)
	   (condition-case nil
	       (progn 
		 (make-directory scores-dir)
		 (set-file-modes scores-dir #o777))
	     (error nil))
	   (and (eq t (car (file-attributes scores-dir)))
		(rename-file temp scores-file t)))
       (condition-case nil
	   (delete-file temp)
	 (error nil))))

You might unwittingly overwrite the file named "snake-scores" in some
unknown directory if someone maliciously creates a /tmp/snake symlink
at just the right time, but that's not too bad.  (Maybe we should use
"/tmp/emacs-game-scores/this-file-name-is-not-used-for-any-important\
-file-in-any-directory-I-hope".)

> But in any case sharing should not be done via /tmp.
> We should instead define a `score-files-directory' which could default
> to "/var/games" or to "~/.emacs.d".

An advantage of using /tmp is that it exists on every (sane) system,
and does not require any help from the system administrator.

-al

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Stefan Monnier]

> An advantage of using /tmp is that it exists on every (sane) system,
> and does not require any help from the system administrator.

And gets cleared during reboot...


	Stefan


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

I think I fixed this problem in terminal.el.  Can anyone see if it
really works?

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

I fixed most of the uses of make-temp-name to use make-temp-file
instead.  I did not fix Gnus, and I asked maintainers of certain files
to fix those files.


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Colin Walters]

On Sun, 2002-03-03 at 23:08, Richard Stallman wrote:
> I fixed most of the uses of make-temp-name to use make-temp-file
> instead.  I did not fix Gnus, and I asked maintainers of certain files
> to fix those files.

Well, you have to look carefully, because not all uses of
`make-temp-name' are insecure.  If you create a new temporary
subdirectory that is not world-writable, and then create files inside of
there, you're fine (as, for example, gnus-uu.el appears to do). 
However, it is still probably better to use `make-temp-file' regardless,
but Gnus probably has to consider portability between Emacsen.







_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

    We should instead define a `score-files-directory' which could default
    to "/var/games" or to "~/.emacs.d".

That seems like a reasonable approach.
Would someone like to do it?

If /var/games is treated just like /tmp, meaning anyone can create a
file in it, then it will raise the same security issues as /tmp.  We
could perhaps use the code that Al Petrovsky sent, if that is correct.

Or we could say that the files should be created by root during
installation, and that /var/games should not allow anyone but root to
create files.



_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

    Is this sufficient?

That code needs comments to explain what it is trying to do and why
that is right.  After some study, I think I see WHAT it does, but I
can't see why one would want to do that.  It seems to make the file
read-only; why do that?

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Al Petrofsky]

> From: Richard Stallman <rms@gnu.org>

> That code needs comments to explain what it is trying to do and why
> that is right.  After some study, I think I see WHAT it does, but I
> can't see why one would want to do that.

The original problem was that when we wrote over /tmp/snake-scores we
couldn't be sure that /tmp/snake-scores hadn't just been changed from
a file to a symbolic link pointing to one of our important files.

My solution is to first write the scores securely into a temp file and
then move it to the desired place.  This is safe, because if someone
has made the destination filename a symbolic link, then the rename
system call removes the link, rather than overwriting the linked-to
file.

This requires storing the file in a subdirectory of /tmp that is
world-writable without restriction, as opposed to /tmp itself, which
normally has its sticky bit set, thus forbidding people from deleting
others' files or renaming over them.

The catch is that if someone has made /tmp/emacs-game-scores a
symbolic link to one of our directories, then we could overwrite the
file named snake-scores in that directory.  So the improvement is that
only our files named snake-scores are vulnerable, rather than all of
them.

>  It seems to make the file read-only; why do that?

The point of (set-file-modes temp #o444) is to ensure the file is
world-readable, in case the user has a paranoid umask.  Making the
file non-writable is not necessary.

-al

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Colin Walters]

On Mon, 2002-03-04 at 18:40, Richard Stallman wrote:
>     We should instead define a `score-files-directory' which could default
>     to "/var/games" or to "~/.emacs.d".
> 
> That seems like a reasonable approach.
> Would someone like to do it?

I'll work on this.


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Andreas Schwab]

Richard Stallman <rms@gnu.org> writes:

|> If /var/games is treated just like /tmp, meaning anyone can create a
|> file in it, then it will raise the same security issues as /tmp.  We
|> could perhaps use the code that Al Petrovsky sent, if that is correct.

The convention for /var/games is that it is writable for a special group
(game) only, and any program wanting to have access to it must be setgid
game.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE GmbH, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Stefan Monnier]

> My solution is to first write the scores securely into a temp file and
> then move it to the desired place.  This is safe, because if someone
> has made the destination filename a symbolic link, then the rename
> system call removes the link, rather than overwriting the linked-to file.

The idea is alright, but:

> This requires storing the file in a subdirectory of /tmp that is
> world-writable without restriction, as opposed to /tmp itself, which
> normally has its sticky bit set, thus forbidding people from deleting
> others' files or renaming over them.

This creates another problem, which comes from the fact that Emacs does
not have the notion of file descriptor: an attacker can change the
temp file into a symlink between the call to make-temp-file and the call
to write-region.

I really think it's better to require that the parent directory
of the file we're writing to is only writable by ourselves and/or
by root.


	Stefan


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Stefan Monnier]

> Richard Stallman <rms@gnu.org> writes:
> 
> |> If /var/games is treated just like /tmp, meaning anyone can create a
> |> file in it, then it will raise the same security issues as /tmp.  We
> |> could perhaps use the code that Al Petrovsky sent, if that is correct.
> 
> The convention for /var/games is that it is writable for a special group
> (game) only, and any program wanting to have access to it must be setgid
> game.

Which is not an option for Emacs.  I'd much rather have something like
/var/games/emacs-scores owned by root and only writable by root
with a file /var/games/emacs-scores/snake that's world-writable.


	Stefan


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

    > The convention for /var/games is that it is writable for a special group
    > (game) only, and any program wanting to have access to it must be setgid
    > game.

Emacs doesn't need to write in /var/games, it just needs to be able to
write certain specific files that are somewhere under /var/games.
Those files could be created by Emacs installation (which is run as
root) and given modes that allow Emacs to update them.


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Al Petrofsky]

> From: "Stefan Monnier" <monnier+gnu/emacs@RUM.cs.yale.edu>
> 
> > My solution is to first write the scores securely into a temp file and
> > then move it to the desired place.  This is safe, because if someone
> > has made the destination filename a symbolic link, then the rename
> > system call removes the link, rather than overwriting the linked-to file.
> 
> The idea is alright, but:
> 
> > This requires storing the file in a subdirectory of /tmp that is
> > world-writable without restriction, as opposed to /tmp itself, which
> > normally has its sticky bit set, thus forbidding people from deleting
> > others' files or renaming over them.
> 
> This creates another problem, which comes from the fact that Emacs does
> not have the notion of file descriptor: an attacker can change the
> temp file into a symlink between the call to make-temp-file and the call
> to write-region.

The temp file is created directly in /tmp, which has sticky bit
protection, thus preventing an attacker from changing the temp file
into a symlink.  If /tmp does not have sticky bit protection, then all
uses of make-temp-file are insecure.

> I really think it's better to require that the parent directory
> of the file we're writing to is only writable by ourselves and/or
> by root.

I agree, but it doesn't have to be root.  If joe user installs emacs
with --prefix=/home/joe and the install process makes a world-writable
snake-scores file in directory /home/joe/var/emacs/game-scores, which
is unwritable by anyone but joe, then that is sufficient.  Anyone who
uses the binaries has to trust joe anyway.

-al

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

    The catch is that if someone has made /tmp/emacs-game-scores a
    symbolic link to one of our directories, then we could overwrite the
    file named snake-scores in that directory.

I see there is no way to fix that--the user that owns the directory
could always delete it and replace it with a link at just the wrong time.

But /tmp is the wrong place for score files anyway.

    The point of (set-file-modes temp #o444) is to ensure the file is
    world-readable, in case the user has a paranoid umask.  Making the
    file non-writable is not necessary.

For a score file, it's a bug, isn't it?


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Colin Walters]

[ We're all on the list, so I'll drop the CCs ]

On Tue, 2002-03-05 at 10:20, Stefan Monnier wrote:

> Which is not an option for Emacs.  I'd much rather have something like
> /var/games/emacs-scores owned by root and only writable by root
> with a file /var/games/emacs-scores/snake that's world-writable.

Well, for the Debian packaged version of Emacs, I think we will set it
up so emacs is setgid games, and writes to /var/games/emacs or so.

I almost have a patch ready for this.


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Colin Walters]

[-- Attachment #1: Type: text/plain, Size: 178 bytes --]

How about the following?  One issue still to consider is whether or not
we should default to writing game state in ~/.emacs.d, or in
`temporary-file-directory'.  Any opinions?



[-- Attachment #2: gamestate.patch --]
[-- Type: text/plain, Size: 7027 bytes --]

Index: src/filelock.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/filelock.c,v
retrieving revision 1.96
diff -u -r1.96 filelock.c
--- src/filelock.c	6 Feb 2002 15:44:28 -0000	1.96
+++ src/filelock.c	6 Mar 2002 07:34:59 -0000
@@ -762,6 +762,7 @@
 {
   DEFVAR_LISP ("temporary-file-directory", &Vtemporary_file_directory,
 	       doc: /* The directory for writing temporary files.  */);
+  /* We initialize this more intelligently later in startup.el */
   Vtemporary_file_directory = Qnil;
 
   defsubr (&Sunlock_buffer);
Index: src/callproc.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/callproc.c,v
retrieving revision 1.182
diff -u -r1.182 callproc.c
--- src/callproc.c	4 Mar 2002 23:41:00 -0000	1.182
+++ src/callproc.c	6 Mar 2002 07:35:00 -0000
@@ -104,7 +104,7 @@
 #endif
 
 Lisp_Object Vexec_path, Vexec_directory, Vexec_suffixes;
-Lisp_Object Vdata_directory, Vdoc_directory;
+Lisp_Object Vdata_directory, Vdoc_directory, Vgame_state_directory;
 Lisp_Object Vconfigure_info_directory;
 Lisp_Object Vtemp_file_name_pattern;
 
@@ -1534,6 +1534,9 @@
 	}
     }
 
+  /* We initialize this more intelligently later in startup.el */
+  Vgame_state_directory = Qnil;
+  
 #ifndef CANNOT_DUMP
   if (initialized)
 #endif
@@ -1614,6 +1617,11 @@
   DEFVAR_LISP ("data-directory", &Vdata_directory,
 	       doc: /* Directory of machine-independent files that come with GNU Emacs.
 These are files intended for Emacs to use while it runs.  */);
+
+  DEFVAR_LISP ("game-state-directory", &Vgame_state_directory,
+	       doc: /* Directory of high-score and other state files for games.
+Depending on your system setup, this may or may not default to a
+shared directory. */);
 
   DEFVAR_LISP ("doc-directory", &Vdoc_directory,
 	       doc: /* Directory containing the DOC file that comes with GNU Emacs.
Index: lisp/startup.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/startup.el,v
retrieving revision 1.290
diff -u -r1.290 startup.el
--- lisp/startup.el	6 Feb 2002 14:59:10 -0000	1.290
+++ lisp/startup.el	6 Mar 2002 07:35:01 -0000
@@ -653,6 +653,19 @@
 	(if (eq system-type 'ms-dos)
 	    (getenv "TMPDIR")))
 
+  (unless game-state-directory
+    (setq game-state-directory
+	  (let (ret
+		choice
+		(choices (list "/var/games/emacs" "/var/games"
+			       temporary-file-directory)))
+	    (while (and (not ret) (setq choice (car choices)))
+	      (when (and (eq (car (file-attributes choice)) t)
+			 (file-writable-p choice))
+		(setq ret choice))
+	      (setq choices (cdr choices)))
+	    ret)))
+
   ;; See if we should import version-control from the environment variable.
   (let ((vc (getenv "VERSION_CONTROL")))
     (cond ((eq vc nil))			;don't do anything if not set
Index: lisp/play/gamegrid.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/play/gamegrid.el,v
retrieving revision 1.5
diff -u -r1.5 gamegrid.el
--- lisp/play/gamegrid.el	3 Mar 2002 14:13:53 -0000	1.5
+++ lisp/play/gamegrid.el	6 Mar 2002 07:35:02 -0000
@@ -404,27 +404,39 @@
 
 ;; ;;;;;;;;;;;;;;; high score functions ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
-(defun gamegrid-add-score (file score)
-  "Add the current score to the high score file."
+(defun gamegrid-add-score (filename score &optional directory)
+  "Add SCORE to the high score file named FILENAME.
+If DIRECTORY is non-nil, then place the high-score file in that
+directory.  Otherwise, place the high score file in
+`game-state-directory'.
+Note that there is no attempt made to guarantee that all scores will
+be written to the score file if multiple Emacs users are writing to
+the score file using this function at the same time."
   (save-excursion
-  (find-file-other-window file)
-  (setq buffer-read-only nil)
-  (goto-char (point-max))
-  (insert (format "%05d\t%s\t%s <%s>\n"
-		  score
-		  (current-time-string)
-		  (user-full-name)
-		  (cond ((fboundp 'user-mail-address)
-			 (user-mail-address))
-			((boundp 'user-mail-address)
-			 user-mail-address)
-			(t ""))))
-  (sort-numeric-fields 1 (point-min) (point-max))
-    (reverse-region (point-min) (point-max))
-  (goto-line (1+ gamegrid-score-file-length))
-  (delete-region (point) (point-max))
-  (setq buffer-read-only t)
-    (save-buffer)))
+    (let* (buf
+	   (file (expand-file-name filename (or directory game-state-directory)))
+	   (tempfile (make-temp-file file)))
+      (while (setq buf (find-buffer-visiting file))
+	(kill-buffer buf))
+      (with-temp-file tempfile
+	(when (file-exists-p file)
+	  (insert-file-contents file nil nil nil t))
+	(goto-char (point-max))
+	(insert (format "%05d\t%s\t%s <%s>\n"
+			score
+			(current-time-string)
+			(user-full-name)
+			(cond ((fboundp 'user-mail-address)
+			       (user-mail-address))
+			      ((boundp 'user-mail-address)
+			       user-mail-address)
+			      (t ""))))
+	(sort-numeric-fields 1 (point-min) (point-max))
+	(reverse-region (point-min) (point-max))
+	(goto-line (1+ gamegrid-score-file-length))
+	(delete-region (point) (point-max)))
+      (rename-file tempfile file t)
+      (find-file-other-window file))))
 
 ;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
Index: lisp/play/snake.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/play/snake.el,v
retrieving revision 1.8
diff -u -r1.8 snake.el
--- lisp/play/snake.el	3 Mar 2002 16:09:28 -0000	1.8
+++ lisp/play/snake.el	6 Mar 2002 07:35:02 -0000
@@ -82,10 +82,7 @@
 (defvar snake-score-y snake-height
   "Y position of score.")
 
-;; It is not safe to put this in /tmp.
-;; Someone could make a symlink in /tmp
-;; pointing to a file you don't want to clobber.
-(defvar snake-score-file "~/.snake-scores"
+(defvar snake-score-file "snake-scores"
   "File for holding high scores.")
 
 ;; ;;;;;;;;;;;;; display options ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Index: lisp/play/tetris.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/play/tetris.el,v
retrieving revision 1.7
diff -u -r1.7 tetris.el
--- lisp/play/tetris.el	3 Mar 2002 16:09:28 -0000	1.7
+++ lisp/play/tetris.el	6 Mar 2002 07:35:02 -0000
@@ -150,10 +150,7 @@
 (defvar tetris-score-y (+ tetris-next-y 6)
   "Y position of score.")
 
-;; It is not safe to put this in /tmp.
-;; Someone could make a symlink in /tmp
-;; pointing to a file you don't want to clobber.
-(defvar tetris-score-file "~/.tetris-scores"
+(defvar tetris-score-file "tetris-scores"
 ;; anybody with a well-connected server want to host this?
 ;(defvar tetris-score-file "/anonymous@ftp.pgt.com:/pub/cgw/tetris-scores"
   "File for holding high scores.")

Re: many packages write to `temporary-file-directory' insecurely

[Stefan Monnier]

> How about the following?  One issue still to consider is whether or not
> we should default to writing game state in ~/.emacs.d, or in
> `temporary-file-directory'.  Any opinions?
[...]
> +  DEFVAR_LISP ("game-state-directory", &Vgame_state_directory,
> +	       doc: /* Directory of high-score and other state files for games.
> +Depending on your system setup, this may or may not default to a
> +shared directory. */);

There's no point in declaring it in the C code since it's only
used from elisp.


	Stefan


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Stefan Monnier]

> [ We're all on the list, so I'll drop the CCs ]
> 
> On Tue, 2002-03-05 at 10:20, Stefan Monnier wrote:
> 
> > Which is not an option for Emacs.  I'd much rather have something like
> > /var/games/emacs-scores owned by root and only writable by root
> > with a file /var/games/emacs-scores/snake that's world-writable.
> 
> Well, for the Debian packaged version of Emacs, I think we will set it
> up so emacs is setgid games, and writes to /var/games/emacs or so.

This is then equivalent to making all users members of the `games'
group, so what's the point ?  You might as well make the thing world-writable
and not bother with the games group.


	Stefan


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Colin Walters]

On Wed, 2002-03-06 at 11:59, Stefan Monnier wrote:

> This is then equivalent to making all users members of the `games'
> group, so what's the point ?  You might as well make the thing world-writable
> and not bother with the games group.

True.


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Colin Walters]

On Wed, 2002-03-06 at 11:59, Stefan Monnier wrote:

> There's no point in declaring it in the C code since it's only
> used from elisp.

Do you have a suggestion for where to define it, then?  I couldn't find
any place that seemed "right".  startup.el seems to be the closest.  I
suppose we could just define it in gamegrid.el, but then every Emacs
game that wants to save state would have to require that file.  Perhaps
this isn't a bad thing, but I think of "game-state-directory" much like
"temporary-file-directory".



_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Eli Zaretskii]

On 6 Mar 2002, Colin Walters wrote:

> On Wed, 2002-03-06 at 11:59, Stefan Monnier wrote:
> 
> > There's no point in declaring it in the C code since it's only
> > used from elisp.
> 
> Do you have a suggestion for where to define it, then?  I couldn't find
> any place that seemed "right".  startup.el seems to be the closest.

How about files.el?

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

    Well, for the Debian packaged version of Emacs, I think we will set it
    up so emacs is setgid games, and writes to /var/games/emacs or so.

Why are you working on forks for the Debian modified version
instead of working on making Emacs itself do the right thing?
Whatever that right thing may be, it doesn't make sense
to do it only in the Debian modified version.

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

    How about the following?  One issue still to consider is whether or not
    we should default to writing game state in ~/.emacs.d, or in
    `temporary-file-directory'.  Any opinions?

If it can't put the scores in the proper shared directory for scores,
it should use your home dir.  At least your scores won't get deleted
there.


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

    +		(choices (list "/var/games/emacs" "/var/games"
    +			       temporary-file-directory)))
    +	    (while (and (not ret) (setq choice (car choices)))
    +	      (when (and (eq (car (file-attributes choice)) t)
    +			 (file-writable-p choice))
    +		(setq ret choice))

The game-state-directory should not be world-writable.  If it is
world-writable, it will have the same security problem as /tmp, except
worse if it does not have the sticky bit--make-temp-file won't
be reliable in that case.

One way to solve this problem is by having Emacs installation create
the desired files under /var/games/emacs, make them world-writable,
and make /var/games/emacs read-only.

Does anyone see a better way?


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Colin Walters]

On Fri, 2002-03-08 at 04:07, Richard Stallman wrote:
>     Well, for the Debian packaged version of Emacs, I think we will set it
>     up so emacs is setgid games, and writes to /var/games/emacs or so.
> 
> Why are you working on forks for the Debian modified version
> instead of working on making Emacs itself do the right thing?
> Whatever that right thing may be, it doesn't make sense
> to do it only in the Debian modified version.

My original thought was that it might not be appropriate for Emacs in
general, since Emacs runs on so many platforms, which have differing
ideas of where to put things like this.  But I guess that someone who
wants this to be configurable could use the --localstatedir argument to
configure.


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

    My original thought was that it might not be appropriate for Emacs in
    general, since Emacs runs on so many platforms, which have differing
    ideas of where to put things like this.  But I guess that someone who
    wants this to be configurable could use the --localstatedir argument to
    configure.

We can easily provide many ways to configure things.
So please do this work in the Emacs sources
rather than in Debian's modified version.  Ok?

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Colin Walters]

On Fri, 2002-03-08 at 04:08, Richard Stallman wrote:
> The game-state-directory should not be world-writable.  If it is
> world-writable, it will have the same security problem as /tmp, except
> worse if it does not have the sticky bit--make-temp-file won't
> be reliable in that case.

The original security problem was in the way `gamegrid-add-score'
created files in /tmp, not in the attributes of /tmp itself.  If /tmp is
world-writable with the sticky bit, then it is possible for applications
to securely create files in it.

> One way to solve this problem is by having Emacs installation create
> the desired files under /var/games/emacs, make them world-writable,
> and make /var/games/emacs read-only.

The problem I see with this is that we can't use `rename-file', and thus
we lose atomicity of score file updates.  If multiple users are
concurrently reading and writing the same file, it will eventually be
corrupted.  We could perhaps try to come up with a locking scheme, but
things get very complicated at that point.

We also can't use my proposed solution, which is secure and atomic, but
as I realized right after posting it, fails to allow people to share
scores :)

> Does anyone see a better way?

At this point, my gut feeling is that we're going to be better off just
definining `game-state-directory' to be ~/.emacs.d/games or something,
and leave it at that.


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

    > One way to solve this problem is by having Emacs installation create
    > the desired files under /var/games/emacs, make them world-writable,
    > and make /var/games/emacs read-only.

    The problem I see with this is that we can't use `rename-file', and thus
    we lose atomicity of score file updates.  If multiple users are
    concurrently reading and writing the same file, it will eventually be
    corrupted.

You are right.

How do other game programs handle this?

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Colin Walters]

On Mon, 2002-03-11 at 04:01, Richard Stallman wrote:
>     > One way to solve this problem is by having Emacs installation create
>     > the desired files under /var/games/emacs, make them world-writable,
>     > and make /var/games/emacs read-only.
> 
>     The problem I see with this is that we can't use `rename-file', and thus
>     we lose atomicity of score file updates.  If multiple users are
>     concurrently reading and writing the same file, it will eventually be
>     corrupted.
> 
> You are right.
> 
> How do other game programs handle this?

Well, from a selection of the games I looked at in the Debian "games"
section, none of them seem to make any attempt to handle it at all.  In
the "xjewel" source, in "hscore.c", there are empty methods like:

void File_Lock()
	{
	}

void File_Unlock()
	{
	}

...which apparently the author was going to fill in later.  The
"conquest" game mmap()s the score file, which has the same problems. 
And the "lbreakout2" game just uses read and write.

Really, I can't see a way to solve this without using some locking
mechanism.  But that has its own problems, e.g. what to do if a
malicious user locks the file and never unlocks it?

My gut feeling is that it really would be best to go with
~/.emacs.d/games or so.


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

    My gut feeling is that it really would be best to go with
    ~/.emacs.d/games or so.

That means users do not see each others' scores.
I think this is a mistake.

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Colin Walters]

On Mon, 2002-03-18 at 15:06, Richard Stallman wrote:
>     My gut feeling is that it really would be best to go with
>     ~/.emacs.d/games or so.
> 
> That means users do not see each others' scores.
> I think this is a mistake.

I was talking with some Debian hackers on IRC about this problem, and
one of them suggested a setgid helper program, sort of like movemail. 
This seems to me to be the best solution.  We could give it arguments
like:

/usr/lib/emacs/21.3/update-score --file=/var/games/emacs/snake-scores 
 --add-score "Jane Hacker <jane@gnu.org>   12345 points"

And we should probably impose a limit of, say, 50 scores, and 200
characters in a score line.

One problem with this is I have no idea what should be done on Windows;
I guess we can not care about that for now though.

Any thoughts/objections regarding this solution?


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Steve Kemp]

On Mon, Mar 18, 2002 at 05:36:23PM -0500, Colin Walters wrote:

> I was talking with some Debian hackers on IRC about this problem, and
> one of them suggested a setgid helper program, sort of like movemail. 
> This seems to me to be the best solution.  We could give it arguments
> like:
> 
> /usr/lib/emacs/21.3/update-score --file=/var/games/emacs/snake-scores 
>  --add-score "Jane Hacker <jane@gnu.org>   12345 points"

> Any thoughts/objections regarding this solution?

  It would solve the file locking problem, and allow shared scores.

  But it would appear to have the major flaw that a malicious user
 could fake their scores with almost no effort, eg:

[matrix] skx > update-score --file=/var/games/emacs/snake-score --add-score "Steve 99999999999999 points"

  I assume that you'd be making this setgid games, so that other files
 wouldn't be overwritable..?

Steve
---


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Colin Walters]

On Mon, 2002-03-18 at 18:49, Steve Kemp wrote:

>   But it would appear to have the major flaw that a malicious user
>  could fake their scores with almost no effort, eg:

There is really no realistic way we can prevent that; it would be silly
to try, IMO.



_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Pavel Janík]

   From: Steve Kemp <skx@tardis.ed.ac.uk>
   Date: Mon, 18 Mar 2002 23:49:01 +0000

   >   But it would appear to have the major flaw that a malicious user
   >  could fake their scores with almost no effort, eg:
   > 
   > [matrix] skx > update-score --file=/var/games/emacs/snake-score --add-score "Steve 99999999999999 points"

I do not think we should/could prevent this. Note, that this is possible
right now too...
-- 
Pavel Janík

Have you considered defenestrating your computer and replacing Windows with
GNU/Linux?  It is a shame for you to be in bondage to Microsoft.
                  -- Richard Stallman in emacs-devel

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

    And we should probably impose a limit of, say, 50 scores, and 200
    characters in a score line.

Please avoid arbitrary limits such as those.  The GNU coding standards
say we should avoid arbitrary limits whenever possible.

Other than that, it sounds like a good solution except that it should
be more general, not limited to Emacs alone.

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Colin Walters]

On Wed, 2002-03-20 at 00:10, Richard Stallman wrote:
>     And we should probably impose a limit of, say, 50 scores, and 200
>     characters in a score line.
> 
> Please avoid arbitrary limits such as those.  The GNU coding standards
> say we should avoid arbitrary limits whenever possible.

My concern is that since Emacs is often used on large, multiuser
systems, many of which use disk quotas, a setgid program without any
limits on the files it creates would be a way for users to get around
their disk quotas.  

> Other than that, it sounds like a good solution except that it should
> be more general, not limited to Emacs alone.

Ok, I've committed the C portion of the code to
lib-src/update-game-score.c.  It's not enabled yet.  Also, would be nice
if other people could give it a quick audit; I plan to do this more
thoroughly myself soonish.


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

    My concern is that since Emacs is often used on large, multiuser
    systems, many of which use disk quotas, a setgid program without any
    limits on the files it creates would be a way for users to get around
    their disk quotas.  

One solution for that is to limit the format of the data
that goes in the file so as to specialize it for game scores.

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: many packages write to `temporary-file-directory' insecurely

[Colin Walters]

On Sat, 2002-03-30 at 20:24, Richard Stallman wrote:
>     My concern is that since Emacs is often used on large, multiuser
>     systems, many of which use disk quotas, a setgid program without any
>     limits on the files it creates would be a way for users to get around
>     their disk quotas.  
> 
> One solution for that is to limit the format of the data
> that goes in the file so as to specialize it for game scores.

Well, I guess what bugs me about this is presumably people will want to
have at least their names and/or email addresses in there, and I don't
see how to restrict the "format" of the data such that its total size is
limited.  

On the other hand, I've realized it's a good idea to put the actual
username (or at least the uid) into the score lines, so if someone is
using it to store a substantial amount of data, then it will be
blatantly obvious who is doing it.

By the way, I'm almost done with the autoconf magic necessary to support
this; it's been a bit more painful than I thought.

Re: many packages write to `temporary-file-directory' insecurely

[Richard Stallman]

    On the other hand, I've realized it's a good idea to put the actual
    username (or at least the uid) into the score lines, so if someone is
    using it to store a substantial amount of data, then it will be
    blatantly obvious who is doing it.

yes.  With the uid, it doesn't need any other name info.
Beyond that, there could be a limit of 1k bytes per user.