Re: mkstemp

[Eric Blake]

On 04/27/2011 07:52 AM, Reuben Thomas wrote:
> On 27 April 2011 14:00, Eric Blake <address@hidden> wrote:
>> On 04/27/2011 04:05 AM, Reuben Thomas wrote:
>>> On 27 April 2011 03:24, Bruno Haible <address@hidden> wrote:
>>>>> Does setting a 0600 umask (as glibc does) sound like a good thing to
>>>>> add to the mkstemp-safer functions?
>>
>> Setting umask() is bad for other reasons - it is global state,
> 
> I was being sloppy with my terminology, I meant of course file permissions.
> 
>> The
>> hardest part now is writing the m4 test to detect platforms whose
>> mkstemp is insecure.
> 
> Attached, a patch. I don't have a non-GNU system on which to test it;
> I can only confirm that it works on GNU/Linux (which doesn't prove
> anything really).

> commit 76d83ad4cac604b71489cf2d566db6dea65f92f3
> Author: Reuben Thomas <address@hidden>
> Date:   Wed Apr 27 14:51:22 2011 +0100
> 
>     Use gnulib's mkstemp if the system implementation is insecure.
>     
>     * m4/mkstemp.m4: Add test for non-owner read/write mode bits set
>       in file created by mkstemp.
> 
> diff --git a/m4/mkstemp.m4 b/m4/mkstemp.m4
> index c5cd282..e071bb4 100644
> --- a/m4/mkstemp.m4
> +++ b/m4/mkstemp.m4
> @@ -10,6 +10,8 @@
>  # Other systems lack mkstemp altogether.

Let's bump the serial number.

>  # On OSF1/Tru64 V4.0F, the system-provided mkstemp function can create
>  # only 32 files per process.
> +# On some hosts, mkstemp creates files with mode 0666, which is a security
> +# problem.

Also, it was valid in POSIX 2001, but violates POSIX 2008.

>  # On systems like the above, arrange to use the replacement function.
>  AC_DEFUN([gl_FUNC_MKSTEMP],
>  [
> @@ -44,6 +46,25 @@ AC_DEFUN([gl_FUNC_MKSTEMP],
>                        close (fd);
>                      }
>                  }
> +              if (result == 0)
> +                {
> +                  char templ[] = "conftest.mkstemp/coXXXXXX";
> +                  int (*mkstemp_function) (char *) = mkstemp;
> +                  int fd = mkstemp_function (templ);
> +
> +                  if (fd < 0)
> +                    result |= 4;
> +                  else
> +                    {
> +                       struct stat st;
> +
> +                       if (stat (templ, &st) < 0)

fstat() is better.

> +                         result |= 8;
> +                       else if (st.st_mode & 0077)

False negative if umask is already restrictive.  We need to first change
umask() before probing mkstemp().

> +                         result |= 16;
> +                       close (fd);
> +                    }
> +                }
>                return result;]])],
>            [gl_cv_func_working_mkstemp=yes],
>            [gl_cv_func_working_mkstemp=no],

We also need to document this fix.  I'm working on a v2 of this patch...

-- 
Eric Blake   address@hidden    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature