sh-tmp-file inserts unsafe code

sh-tmp-file inserts unsafe code

[Sven Joachim]

The command `sh-tmp-file' (bound to `C-c C-t' in shell-script-mode)
inserts code to setup temporary file handling based on the script's
name and its pid at runtime.  E.g., for foo.sh:

TMP=${TMPDIR:-/tmp}/foo.sh.$$
trap "rm $TMP* 2>/dev/null" 0

Or for foo.csh:

set tmp = /tmp/foo.csh.$$
onintr exit

Such handling of temporary files used to be common practice, but it is
_unsafe_, exposing the user of the script to symlink attacks.  This is
especially bad if the script is to be run by the superuser, but even
an ordinary user could suffer data loss.  I think Emacs should not
encourage such dangerous coding practice.

How about rewriting sh-tmp-file so that it uses mktemp(1) to create
the temporary file?

Re: sh-tmp-file inserts unsafe code

[Richard M. Stallman]

    How about rewriting sh-tmp-file so that it uses mktemp(1) to create
    the temporary file?

Would you like to send a patch?

Re: sh-tmp-file inserts unsafe code

[Sven Joachim]

Richard M. Stallman wrote:
>     How about rewriting sh-tmp-file so that it uses mktemp(1) to create
>     the temporary file?
> 
> Would you like to send a patch?

Well, it will take me quite some time to even understand the code for
this function. ;-)  But if nobody else volunteers, I'll dig into it.
I think it should be worth the effort.

Re: sh-tmp-file inserts unsafe code

[Emanuele Giaquinta]

Here it is one for c?sh.

*** sh-script.el	Wed Sep 21 13:02:55 2005
--- sh-script.el	Mon Oct 10 11:57:41 2005
***************
*** 3392,3398 ****
    "Insert code to setup temporary file handling.  See `sh-feature'."
    (bash sh-append ksh88)
    (csh (file-name-nondirectory (buffer-file-name))
!        "set tmp = /tmp/" str ".$$" \n
         "onintr exit" \n _
         (and (goto-char (point-max))
  	    (not (bolp))
--- 3392,3398 ----
    "Insert code to setup temporary file handling.  See `sh-feature'."
    (bash sh-append ksh88)
    (csh (file-name-nondirectory (buffer-file-name))
!        "set tmp = `mktemp /tmp/" str ".XXXXXX`" \n
         "onintr exit" \n _
         (and (goto-char (point-max))
  	    (not (bolp))
***************
*** 3415,3421 ****
        > "tmp = /tmp/" str ".$pid" \n
        "fn sigexit { rm $tmp^* >[2]/dev/null }" \n)
    (sh (file-name-nondirectory (buffer-file-name))
!       > "TMP=${TMPDIR:-/tmp}/" str ".$$" \n
        "trap \"rm $TMP* 2>/dev/null\" " ?0 \n))


--- 3415,3421 ----
        > "tmp = /tmp/" str ".$pid" \n
        "fn sigexit { rm $tmp^* >[2]/dev/null }" \n)
    (sh (file-name-nondirectory (buffer-file-name))
!       > "TMP=`mktemp ${TMPDIR:-/tmp}/" str ".XXXXXX`" \n
        "trap \"rm $TMP* 2>/dev/null\" " ?0 \n))

Re: sh-tmp-file inserts unsafe code

[Reiner Steib]

On Mon, Oct 10 2005, Emanuele Giaquinta wrote:

>         > "tmp = /tmp/" str ".$pid" \n
>         "fn sigexit { rm $tmp^* >[2]/dev/null }" \n)
>     (sh (file-name-nondirectory (buffer-file-name))
> !       > "TMP=`mktemp ${TMPDIR:-/tmp}/" str ".XXXXXX`" \n
>         "trap \"rm $TMP* 2>/dev/null\" " ?0 \n))

Why not use `-t' instead of dealing explicitly with TMPDIR?

--8<---------------cut here---------------start------------->8---
--- sh-script.el	19 Sep 2005 11:36:31 +0200	1.166
+++ sh-script.el	10 Oct 2005 17:08:56 +0200	
@@ -3392,7 +3392,7 @@
   "Insert code to setup temporary file handling.  See `sh-feature'."
   (bash sh-append ksh88)
   (csh (file-name-nondirectory (buffer-file-name))
-       "set tmp = /tmp/" str ".$$" \n
+       "set tmp = `mktemp -t " str ".XXXXXXXXXX`" \n
        "onintr exit" \n _
        (and (goto-char (point-max))
 	    (not (bolp))
@@ -3415,7 +3415,7 @@
       > "tmp = /tmp/" str ".$pid" \n
       "fn sigexit { rm $tmp^* >[2]/dev/null }" \n)
   (sh (file-name-nondirectory (buffer-file-name))
-      > "TMP=${TMPDIR:-/tmp}/" str ".$$" \n
+      > "TMP=`mktemp -t " str ".XXXXXXXXXX`" \n
       "trap \"rm $TMP* 2>/dev/null\" " ?0 \n))
 
--8<---------------cut here---------------end--------------->8---

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: sh-tmp-file inserts unsafe code

[Richard M. Stallman]

I installed your changes, and tried extrapolating them to the other shells.
Thanks.

Re: sh-tmp-file inserts unsafe code

[Sven Joachim]

Richard M. Stallman wrote:
 > I installed your changes, and tried extrapolating them to the other shells.
 > Thanks.

Well, I wasn't lazy either. :-)  Yesterday I installed the es and rc
shells, read their manpages and worked out how they do command
substitution.  Both of them use the syntax

                      `{ commands }

to substitute the group of COMMANDS.  Together with Reiner's suggestion
to use the "-t" flag of mktemp (which is more compliant with the GNU
coding standards, honoring the user's TMPDIR environment variable) I
worked out the following patch:

*** sh-script.el	2005-10-10 21:23:45	+0200	1.167
--- sh-script.el	2005-10-11 15:40:08	+0200
***************
*** 3392,3411 ****
     "Insert code to setup temporary file handling.  See `sh-feature'."
     (bash sh-append ksh88)
     (csh (file-name-nondirectory (buffer-file-name))
!        "set tmp = `mktemp /tmp/" str ".XXXXXX`" \n
          "onintr exit" \n _
          (and (goto-char (point-max))
   	    (not (bolp))
   	    ?\n)
          "exit:\n"
          "rm $tmp* >&/dev/null" > \n)
-   ;; The change to use mktemp here has not been tested;
-   ;; I don't know es syntax, so I had to guess.
-   ;; If you try it, or if you know es syntax and can check it,
-   ;; please tell me whether it needs any change.  --rms.
     (es (file-name-nondirectory (buffer-file-name))
!       > "local( signals = $signals sighup sigint; tmp = `mktemp /tmp/" str
!       ".XXXXXX` ) {" \n
         > "catch @ e {" \n
         > "rm $tmp^* >[2]/dev/null" \n
         "throw $e" \n
--- 3392,3407 ----
     "Insert code to setup temporary file handling.  See `sh-feature'."
     (bash sh-append ksh88)
     (csh (file-name-nondirectory (buffer-file-name))
!        "set tmp = `mktemp -t " str ".XXXXXX`" \n
          "onintr exit" \n _
          (and (goto-char (point-max))
   	    (not (bolp))
   	    ?\n)
          "exit:\n"
          "rm $tmp* >&/dev/null" > \n)
     (es (file-name-nondirectory (buffer-file-name))
!       > "local( signals = $signals sighup sigint;" \n
!       > "tmp = `{ mktemp -t " str ".XXXXXX } ) {" \n
         > "catch @ e {" \n
         > "rm $tmp^* >[2]/dev/null" \n
         "throw $e" \n
***************
*** 3415,3429 ****
         ?\} > \n)
     (ksh88 sh-modify sh
   	 7 "EXIT")
-   ;; The change to use mktemp here has not been tested;
-   ;; I don't know rc syntax, so I had to guess.
-   ;; If you try it, or if you know rc syntax and can check it,
-   ;; please tell me whether it needs any change.  --rms.
     (rc (file-name-nondirectory (buffer-file-name))
!       > "tmp = `mktemp /tmp/" str ".XXXXXX`" \n
         "fn sigexit { rm $tmp^* >[2]/dev/null }" \n)
     (sh (file-name-nondirectory (buffer-file-name))
!       > "TMP=`mktemp ${TMPDIR:-/tmp}/" str ".XXXXXX`" \n
         "trap \"rm $TMP* 2>/dev/null\" " ?0 \n))


--- 3411,3421 ----
         ?\} > \n)
     (ksh88 sh-modify sh
   	 7 "EXIT")
     (rc (file-name-nondirectory (buffer-file-name))
!       > "tmp = `{ mktemp -t " str ".XXXXXX }" \n
         "fn sigexit { rm $tmp^* >[2]/dev/null }" \n)
     (sh (file-name-nondirectory (buffer-file-name))
!       > "TMP=`mktemp -t " str ".XXXXXX`" \n
         "trap \"rm $TMP* 2>/dev/null\" " ?0 \n))




I have tested it with example scripts, the code seems to be correct.
Note that font-lock-mode will mis-fontify es and rc scripts because of
the single backtick.

Re: sh-tmp-file inserts unsafe code

[Sven Joachim]

Emanuele Giaquinta wrote:
> In the FreeBSD implementation of mktemp the '-t' option has a slightly
> different meaning, it would be better to test it before.

I don´t have FreeBSD, but looking at the manpage, the difference should
not matter.  Anybody here to test it?

Re: sh-tmp-file inserts unsafe code

[Reiner Steib]

On Tue, Oct 11 2005, Sven Joachim wrote:

> Emanuele Giaquinta wrote:
>> In the FreeBSD implementation of mktemp the '-t' option has a slightly
>> different meaning, it would be better to test it before.
>
> I don´t have FreeBSD, but looking at the manpage, the difference should
> not matter.  Anybody here to test it?

$ mktemp -t foo.sh.XXXXXXXXXX
/tmp/foo.sh.XXXXXXXXXX.KnRhKukl
$ uname -a
FreeBSD x86-freebsd1 4.11-RELEASE FreeBSD 4.11-RELEASE #0: [...]

$ mktemp -t foo.sh.XXXXXXXXXX
bash: mktemp: command not found
$ uname -a
SunOS sparc-solaris1 5.9 Generic_112233-03 sun4u sparc SUNW,Ultra-60

$ mktemp -t foo.sh.XXXXXXXXXX
/tmp/foo.sh.XXXXgHaO1t
$ uname -a
SunOS wega 5.10 Generic_118822-02 sun4u sparc SUNW,Ultra-Enterprise Solaris

$ mktemp -t foo.sh.XXXXXXXXXX
/tmp/foo.sh.J7Wmqo2lpy
$ uname -a
Darwin ppc-osx2.[...] 6.8 Darwin Kernel Version 6.8: [...]

HP-UX 10.20 also has maktemp, but doesn't support `-t':

,----[ mktemp(1) on HP-UX 10.20 ]
|  SYNOPSIS
|       mktemp [-c] [-d directory_name] [-p prefix]
`----

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: sh-tmp-file inserts unsafe code

[Richard M. Stallman]

Nobody HAS to use that obscure feature of sh-script mode.  So it can
use -t.  If that doesn't work on your operating system, switch to
GNU/Linux.

Re: sh-tmp-file inserts unsafe code

[Richard M. Stallman]

I installed this.  Thanks.

Re: sh-tmp-file inserts unsafe code

[Kevin Rodgers]

Sven Joachim wrote:
> How about rewriting sh-tmp-file so that it uses mktemp(1) to create
> the temporary file?

What about those of us on systems that do not provide mktemp(1)?

I'm running ksh 93 on Solaris 9.

-- 
Kevin Rodgers

Re: sh-tmp-file inserts unsafe code

[Sven Joachim]

Kevin Rodgers wrote:
> What about those of us on systems that do not provide mktemp(1)?

> I'm running ksh 93 on Solaris 9.

You should install it from www.mktemp.org.
Does Sun provide mktemp in Solaris 10?

Re: sh-tmp-file inserts unsafe code

[Sven Joachim]

Reiner Steib wrote:

> $ mktemp -t foo.sh.XXXXXXXXXX
> /tmp/foo.sh.XXXXXXXXXX.KnRhKukl
> $ uname -a
> FreeBSD x86-freebsd1 4.11-RELEASE FreeBSD 4.11-RELEASE #0: [...]
>
> $ mktemp -t foo.sh.XXXXXXXXXX
> bash: mktemp: command not found
> $ uname -a
> SunOS sparc-solaris1 5.9 Generic_112233-03 sun4u sparc SUNW,Ultra-60
>
> $ mktemp -t foo.sh.XXXXXXXXXX
> /tmp/foo.sh.XXXXgHaO1t
> $ uname -a
> SunOS wega 5.10 Generic_118822-02 sun4u sparc SUNW,Ultra-Enterprise Solaris
>
> $ mktemp -t foo.sh.XXXXXXXXXX
> /tmp/foo.sh.J7Wmqo2lpy
> $ uname -a
> Darwin ppc-osx2.[...] 6.8 Darwin Kernel Version 6.8: [...]
>
> HP-UX 10.20 also has maktemp, but doesn't support `-t':
>
> ,----[ mktemp(1) on HP-UX 10.20 ]
> |  SYNOPSIS
> |       mktemp [-c] [-d directory_name] [-p prefix]
> `----

Thank you.  That's quite a mess indeed. :-(
It confirms that portable scripts best start with

#!/usr/bin/perl

;-)

Cheers, Sven.

Re: sh-tmp-file inserts unsafe code

[Sven Joachim]

Richard M. Stallman wrote:

> Nobody HAS to use that obscure feature of sh-script mode.

It's not _that_ obscure, it even has a menu entry.

> So it can use -t. If that doesn't work on your operating system,
> switch to GNU/Linux.

Incidentally, the systems where it does _not_ work are all propietary,
according to Reiner's message.  That may be no coincidence...

Cheers, Sven.

Re: sh-tmp-file inserts unsafe code

[Kevin Rodgers]

Sven Joachim wrote:
> Incidentally, the systems where it does _not_ work are all propietary,
> according to Reiner's message.  That may be no coincidence...

Incidentally, mktemp(1) is not a POSIX shell utility.

-- 
Kevin Rodgers