zsh-workers
 help / color / mirror / code / Atom feed
* [PATCH] replacement for mktemp and mkstemp code in Src/utils.c
@ 2019-04-19 19:54 Clinton Bunch
  2019-04-20  4:33 ` Matthew Martin
  0 siblings, 1 reply; 6+ messages in thread
From: Clinton Bunch @ 2019-04-19 19:54 UTC (permalink / raw)
  To: zsh-workers

On at least one system mktemp produces very predictable names:

% () { print File: $1; cat $1 } =(print "Hello World")
File: /tmp/zsh010785
Hello World
% () { print File: $1; cat $1 } =(print "Hello World")
File: /tmp/zsh010785
Hello World
% () { print File: $1; cat $1 } =(print "Hello World")
File: /tmp/zsh010785
Hello World
% echo $$
10785

This provides an alternate implementation for generating and opening 
temp file names.  I considered only using this implementation on known 
bad systems, but I have no way of knowing all of them (or testing for 
them in configure).  I see no reason to expect system implementations of 
mktemp or mkstemp to be significantly faster than mine unless written in 
assembly (which seems unlikely).

This also provides the necessary utility function for my plans to 
introduce a builtin for managing tempfiles.


Patch:

diff --git a/Src/utils.c b/Src/utils.c
index 32f600858..06802c1a1 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -2174,9 +2174,92 @@ zcloselockfd(int fd)
      return 0;
  }

-#ifdef HAVE__MKTEMP
-extern char *_mktemp(char *);
+/*
+ *  Replacement for mktemp,mkstemp, and mkstemps. */
+
+static char 
sc[]="abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
+#ifdef HAVE_ERAND48
+static unsigned short seed[3]={0,0,0};
+#endif
+
+/**/
+mod_export int
+ztempfile(char *template,int slen,enum zmt_action action, int *fd)
+{
+       char *padptr, *p, *path;
+       int padlen=0,r,i,len,success=0,failure=0,attempts=0;
+       struct stat dummy;
+
+       len=ztrlen(template);
+       if((action == ZMT_FILE && !fd) || slen < 0 || slen >= len-6) {
+           errno=EINVAL;
+           return 0;
+       }
+#ifdef HAVE_ERAND48
+       if(seed[0]==0 && seed[1] == 0 && seed[2] == 0){
+           seed[0]=getppid();
+           seed[1]=time(NULL) & 0xFFFF;
+           seed[2]=getpid();
+       }
  #endif
+       if(!(path=ztrdup(template)))
+           return 0;
+       padptr=path+(len-slen-1);
+
+       while(*padptr == 'X') {
+           padlen++;
+           padptr--;
+       }
+       padptr++;
+       if(padlen < 6) {
+           errno=EINVAL;
+           return 0;
+       }
+       do {
+            for(p=padptr,i=0;i<padlen;i++)
+           {
+#ifdef HAVE_ERAND48
+               r=erand48(seed)*(sizeof(sc)-2)+0.5;
+#else
+                r=rand()%(sizeof(sc)-1);
+#endif
+               *p++=sc[r];
+           }
+           switch(action)
+           {
+                case ZMT_NAME:
+                   if(lstat(path,&dummy) < 0  && errno == ENOENT)
+                       success=1;
+                   break;
+               case ZMT_DIR:
+                   if(mkdir(path,0700)) {
+                       success=1;
+                   }else if(errno!=EEXIST) {
+                       failure=1;
+                   }
+                   break;
+               case ZMT_FILE:
+ if((*fd=open(path,O_RDWR|O_CREAT|O_EXCL,0600))<0) {
+                       if(errno!=EEXIST)
+                           failure=1;
+                   }else
+                       success=1;
+                   break;
+                default:
+                   errno=ENOTSUP;
+                   return 1;
+                   break;
+           }
+       } while(!success && !failure && ++attempts < 16);
+       if(success) {
+           ztrncpy(template,path,len);
+            errno = 0;
+        }
+       if(attempts >= 16)
+           errno=ECANCELED;
+       zsfree(path);
+       return success?1:0;
+}

  /* Get a unique filename for use as a temporary file.  If "prefix" is
   * NULL, the name is relative to $TMPPREFIX; If it is non-NULL, the
@@ -2198,12 +2281,11 @@ gettempname(const char *prefix, int use_heap)
      else
         ret = bicat(unmeta(prefix), suffix);

-#ifdef HAVE__MKTEMP
-    /* Zsh uses mktemp() safely, so silence the warnings */
-    ret = (char *) _mktemp(ret);
-#else
-    ret = (char *) mktemp(ret);
-#endif
+    if(!ztempfile(ret,0,ZMT_NAME,NULL)){
+        if (use_heap)
+           free(ret);
+       ret = NULL;
+    }
      unqueue_signals();

      return ret;
@@ -2219,7 +2301,6 @@ gettempfile(const char *prefix, int use_heap, char 
**tempname)
      char *fn;
      int fd;
      mode_t old_umask;
-#if HAVE_MKSTEMP
      char *suffix = prefix ? ".XXXXXX" : "XXXXXX";

      queue_signals();
@@ -2231,29 +2312,11 @@ gettempfile(const char *prefix, int use_heap, 
char **tempname)
      else
         fn = bicat(unmeta(prefix), suffix);

-    fd = mkstemp(fn);
-    if (fd < 0) {
+    if(!ztempfile(fn,0,ZMT_FILE,&fd)) {
         if (!use_heap)
             free(fn);
         fn = NULL;
      }
-#else
-    int failures = 0;
-
-    queue_signals();
-    old_umask = umask(0177);
-    do {
-       if (!(fn = gettempname(prefix, use_heap))) {
-           fd = -1;
-           break;
-       }
-       if ((fd = open(fn, O_RDWR | O_CREAT | O_EXCL, 0600)) >= 0)
-           break;
-       if (!use_heap)
-           free(fn);
-       fn = NULL;
-    } while (errno == EEXIST && ++failures < 16);
-#endif
      *tempname = fn;

      umask(old_umask);
diff --git a/Src/zsh.h b/Src/zsh.h
index fc3ed2127..684ff4e4b 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -3338,3 +3338,12 @@ typedef int convchar_t;
  #define ZWS(s) s

  #endif /* MULTIBYTE_SUPPORT */
+
+/*************************************
+ * Tempfile actions                  *
+ *************************************/
+enum zmt_action {
+  ZMT_FILE  = 0,
+  ZMT_DIR   = 1,
+  ZMT_NAME  = 2
+};
diff --git a/configure.ac b/configure.ac
index 8a2664ed2..29d014372 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1251,7 +1251,7 @@ AC_CHECK_FUNCS(strftime strptime mktime timelocal \
                readlink faccessx fchdir ftruncate \
                fstat lstat lchown fchown fchmod \
                fseeko ftello \
-              mkfifo _mktemp mkstemp \
+              mkfifo \
                waitpid wait3 \
                sigaction sigblock sighold sigrelse sigsetmask sigprocmask \
                killpg setpgid setpgrp tcsetpgrp tcgetattr nice \




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

* Re: [PATCH] replacement for mktemp and mkstemp code in Src/utils.c
  2019-04-19 19:54 [PATCH] replacement for mktemp and mkstemp code in Src/utils.c Clinton Bunch
@ 2019-04-20  4:33 ` Matthew Martin
  2019-04-20 10:27   ` Mikael Magnusson
  0 siblings, 1 reply; 6+ messages in thread
From: Matthew Martin @ 2019-04-20  4:33 UTC (permalink / raw)
  To: Clinton Bunch; +Cc: zsh-workers

On Fri, Apr 19, 2019 at 02:54:47PM -0500, Clinton Bunch wrote:
> On at least one system mktemp produces very predictable names:
> 
> % () { print File: $1; cat $1 } =(print "Hello World")
> File: /tmp/zsh010785
> Hello World
> % () { print File: $1; cat $1 } =(print "Hello World")
> File: /tmp/zsh010785
> Hello World
> % () { print File: $1; cat $1 } =(print "Hello World")
> File: /tmp/zsh010785
> Hello World
> % echo $$
> 10785
> 
> This provides an alternate implementation for generating and opening temp
> file names.  I considered only using this implementation on known bad
> systems, but I have no way of knowing all of them (or testing for them in
> configure).  I see no reason to expect system implementations of mktemp or
> mkstemp to be significantly faster than mine unless written in assembly
> (which seems unlikely).

I would strongly prefer using the implementation only on known bad
systems (or prodding the relevant vendors to fix their system). I don't
think speed should be the main consideration here; rather the primary
concern should be security. While your patch is certainly better than
using the native mktemp on at least one system, it would be worse than
the native mktemp on say FreeBSD which uses arc4random_uniform which
does not require a user provided seed nor does it have modulo bias.

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

* Re: [PATCH] replacement for mktemp and mkstemp code in Src/utils.c
  2019-04-20  4:33 ` Matthew Martin
@ 2019-04-20 10:27   ` Mikael Magnusson
  2019-04-20 18:52     ` Clinton Bunch
  0 siblings, 1 reply; 6+ messages in thread
From: Mikael Magnusson @ 2019-04-20 10:27 UTC (permalink / raw)
  To: Clinton Bunch, zsh-workers

On 4/20/19, Matthew Martin <phy1729@gmail.com> wrote:
> On Fri, Apr 19, 2019 at 02:54:47PM -0500, Clinton Bunch wrote:
>> On at least one system mktemp produces very predictable names:
>>
>> % () { print File: $1; cat $1 } =(print "Hello World")
>> File: /tmp/zsh010785
>> Hello World
>> % () { print File: $1; cat $1 } =(print "Hello World")
>> File: /tmp/zsh010785
>> Hello World
>> % () { print File: $1; cat $1 } =(print "Hello World")
>> File: /tmp/zsh010785
>> Hello World
>> % echo $$
>> 10785
>>
>> This provides an alternate implementation for generating and opening temp
>> file names.  I considered only using this implementation on known bad
>> systems, but I have no way of knowing all of them (or testing for them in
>> configure).  I see no reason to expect system implementations of mktemp
>> or
>> mkstemp to be significantly faster than mine unless written in assembly
>> (which seems unlikely).
>
> I would strongly prefer using the implementation only on known bad
> systems (or prodding the relevant vendors to fix their system). I don't
> think speed should be the main consideration here; rather the primary
> concern should be security. While your patch is certainly better than
> using the native mktemp on at least one system, it would be worse than
> the native mktemp on say FreeBSD which uses arc4random_uniform which
> does not require a user provided seed nor does it have modulo bias.

The commit message is incomplete, it claims that mktemp is insecure on
one system, therefore it replaces mkstemp, which makes no sense. Is
mkstemp also insecure on that system, is it not available at all? Do
we not even attempt to use mkstemp?

Also, please make at least some attempt to use the same coding style
as the rest of the code base, ie try to use the space bar sometimes.
(You're not even consistent with yourself in some places).

-- 
Mikael Magnusson

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

* Re: [PATCH] replacement for mktemp and mkstemp code in Src/utils.c
  2019-04-20 10:27   ` Mikael Magnusson
@ 2019-04-20 18:52     ` Clinton Bunch
  2019-04-21  4:57       ` Matthew Martin
  2019-04-21 20:08       ` Mikael Magnusson
  0 siblings, 2 replies; 6+ messages in thread
From: Clinton Bunch @ 2019-04-20 18:52 UTC (permalink / raw)
  To: Mikael Magnusson, zsh-workers

Mikael Magnusson wrote:
> On 4/20/19, Matthew Martin <phy1729@gmail.com> wrote:
>> On Fri, Apr 19, 2019 at 02:54:47PM -0500, Clinton Bunch wrote:
>>> On at least one system mktemp produces very predictable names:
>>>
>>> % () { print File: $1; cat $1 } =(print "Hello World")
>>> File: /tmp/zsh010785
>>> Hello World
>>> % () { print File: $1; cat $1 } =(print "Hello World")
>>> File: /tmp/zsh010785
>>> Hello World
>>> % () { print File: $1; cat $1 } =(print "Hello World")
>>> File: /tmp/zsh010785
>>> Hello World
>>> % echo $$
>>> 10785
>>>
>>> This provides an alternate implementation for generating and opening temp
>>> file names.  I considered only using this implementation on known bad
>>> systems, but I have no way of knowing all of them (or testing for them in
>>> configure).  I see no reason to expect system implementations of mktemp
>>> or
>>> mkstemp to be significantly faster than mine unless written in assembly
>>> (which seems unlikely).
>> I would strongly prefer using the implementation only on known bad
>> systems (or prodding the relevant vendors to fix their system). I don't
>> think speed should be the main consideration here; rather the primary
>> concern should be security. While your patch is certainly better than
>> using the native mktemp on at least one system, it would be worse than
>> the native mktemp on say FreeBSD which uses arc4random_uniform which
>> does not require a user provided seed nor does it have modulo bias.
We still face the problem of determining which systems have broken 
implementations.  I know of one, that doesn't mean there aren't others.
  My implementation could easily be modified to use arc4random_uniform 
on those system on which it is available if that's the primary 
objection.  I'd have used /dev/urandom (at least as a seed) if it were 
available everywhere.
> The commit message is incomplete, it claims that mktemp is insecure on
> one system, therefore it replaces mkstemp, which makes no sense. Is
> mkstemp also insecure on that system, is it not available at all? Do
> we not even attempt to use mkstemp?
>
> Also, please make at least some attempt to use the same coding style
> as the rest of the code base, ie try to use the space bar sometimes.
> (You're not even consistent with yourself in some places).
>
mkstemp uses the same insecure naming structure.  That is the normal 
case so it seemed redundant to mention it.

Are the code style settings codified anywhere?  The tabs were added by 
Vim's autoident.  A set of options for indent or astyle would make it 
easier for anyone to meet the coding style.


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

* Re: [PATCH] replacement for mktemp and mkstemp code in Src/utils.c
  2019-04-20 18:52     ` Clinton Bunch
@ 2019-04-21  4:57       ` Matthew Martin
  2019-04-21 20:08       ` Mikael Magnusson
  1 sibling, 0 replies; 6+ messages in thread
From: Matthew Martin @ 2019-04-21  4:57 UTC (permalink / raw)
  To: Clinton Bunch; +Cc: Mikael Magnusson, zsh-workers

On Sat, Apr 20, 2019 at 01:52:24PM -0500, Clinton Bunch wrote:
> Mikael Magnusson wrote:
> > On 4/20/19, Matthew Martin <phy1729@gmail.com> wrote:
> > > On Fri, Apr 19, 2019 at 02:54:47PM -0500, Clinton Bunch wrote:
> > > > This provides an alternate implementation for generating and opening temp
> > > > file names.  I considered only using this implementation on known bad
> > > > systems, but I have no way of knowing all of them (or testing for them in
> > > > configure).  I see no reason to expect system implementations of mktemp
> > > > or
> > > > mkstemp to be significantly faster than mine unless written in assembly
> > > > (which seems unlikely).
> > > I would strongly prefer using the implementation only on known bad
> > > systems (or prodding the relevant vendors to fix their system). I don't
> > > think speed should be the main consideration here; rather the primary
> > > concern should be security. While your patch is certainly better than
> > > using the native mktemp on at least one system, it would be worse than
> > > the native mktemp on say FreeBSD which uses arc4random_uniform which
> > > does not require a user provided seed nor does it have modulo bias.
> We still face the problem of determining which systems have broken
> implementations.  I know of one, that doesn't mean there aren't others.
>  My implementation could easily be modified to use arc4random_uniform on
> those system on which it is available if that's the primary objection.  I'd
> have used /dev/urandom (at least as a seed) if it were available everywhere.

Whichever system has the predictable temp file names, it is a bug in
that system not zsh.  While zsh can paper over the bug, it would be
preferable to fix the broken implementation in that system's libc so
that all mk(s)temp calls are fixed not just the ones zsh would make.
Including a better alternative for that system is an okay interim
solution, but the ultimate goal should be to delete that code when
the system's implementation is fixed.

A bug in one system is not a reason to change behavior in a separate
system. Having all systems use the alternate code results in zsh missing
out on any bug fixes or later enhancements in those systems.

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

* Re: [PATCH] replacement for mktemp and mkstemp code in Src/utils.c
  2019-04-20 18:52     ` Clinton Bunch
  2019-04-21  4:57       ` Matthew Martin
@ 2019-04-21 20:08       ` Mikael Magnusson
  1 sibling, 0 replies; 6+ messages in thread
From: Mikael Magnusson @ 2019-04-21 20:08 UTC (permalink / raw)
  To: Clinton Bunch; +Cc: zsh-workers

On 4/20/19, Clinton Bunch <cdb_zshl@zentaur.org> wrote:
> Mikael Magnusson wrote:
>> On 4/20/19, Matthew Martin <phy1729@gmail.com> wrote:
>>> On Fri, Apr 19, 2019 at 02:54:47PM -0500, Clinton Bunch wrote:
>>>> On at least one system mktemp produces very predictable names:
>>>>
>>>> % () { print File: $1; cat $1 } =(print "Hello World")
>>>> File: /tmp/zsh010785
>>>> Hello World
>>>> % () { print File: $1; cat $1 } =(print "Hello World")
>>>> File: /tmp/zsh010785
>>>> Hello World
>>>> % () { print File: $1; cat $1 } =(print "Hello World")
>>>> File: /tmp/zsh010785
>>>> Hello World
>>>> % echo $$
>>>> 10785
>>>>
>>>> This provides an alternate implementation for generating and opening
>>>> temp
>>>> file names.  I considered only using this implementation on known bad
>>>> systems, but I have no way of knowing all of them (or testing for them
>>>> in
>>>> configure).  I see no reason to expect system implementations of mktemp
>>>> or
>>>> mkstemp to be significantly faster than mine unless written in assembly
>>>> (which seems unlikely).
>>> I would strongly prefer using the implementation only on known bad
>>> systems (or prodding the relevant vendors to fix their system). I don't
>>> think speed should be the main consideration here; rather the primary
>>> concern should be security. While your patch is certainly better than
>>> using the native mktemp on at least one system, it would be worse than
>>> the native mktemp on say FreeBSD which uses arc4random_uniform which
>>> does not require a user provided seed nor does it have modulo bias.
> We still face the problem of determining which systems have broken
> implementations.  I know of one, that doesn't mean there aren't others.
>   My implementation could easily be modified to use arc4random_uniform
> on those system on which it is available if that's the primary
> objection.  I'd have used /dev/urandom (at least as a seed) if it were
> available everywhere.
>> The commit message is incomplete, it claims that mktemp is insecure on
>> one system, therefore it replaces mkstemp, which makes no sense. Is
>> mkstemp also insecure on that system, is it not available at all? Do
>> we not even attempt to use mkstemp?
>>
>> Also, please make at least some attempt to use the same coding style
>> as the rest of the code base, ie try to use the space bar sometimes.
>> (You're not even consistent with yourself in some places).
>>
> mkstemp uses the same insecure naming structure.  That is the normal
> case so it seemed redundant to mention it.

mkstemp returns an open file descriptor whereas mktemp returns a
string. According to the specification using mkstemp will always
create a new file that didn't exist, and is safe. If an OS
implementation somehow clobbers existing files when you call mkstemp()
then that's very broken and should be immediately fixed. The worst
outcome of the name being predictable is that someone can perform a
DOS attack against you, ie preventing you from creating a file, but
they won't gain control of any created files.

> Are the code style settings codified anywhere?  The tabs were added by
> Vim's autoident.  A set of options for indent or astyle would make it
> easier for anyone to meet the coding style.

Yeah, Etc/zsh-development-guide. It doesn't explicitly mention that
you should put spaces around operators, but you should do that too.
Ie, these lines
 +ztempfile(char *template,int slen,enum zmt_action action, int *fd)
 +       int padlen=0,r,i,len,success=0,failure=0,attempts=0;
and others
should look like this
 +ztempfile(char *template, int slen, enum zmt_action action, int *fd)
 +       int r, i, len;
 +       int padlen = 0, success = 0, failure = 0, attempts = 0;

-- 
Mikael Magnusson

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

end of thread, other threads:[~2019-04-21 20:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-19 19:54 [PATCH] replacement for mktemp and mkstemp code in Src/utils.c Clinton Bunch
2019-04-20  4:33 ` Matthew Martin
2019-04-20 10:27   ` Mikael Magnusson
2019-04-20 18:52     ` Clinton Bunch
2019-04-21  4:57       ` Matthew Martin
2019-04-21 20:08       ` Mikael Magnusson

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

	https://git.vuxu.org/mirror/zsh/

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 NNTP newsgroup(s).