Re: [Xen-devel] [PATCH 11/13] libxl: use pipe instead of temporary file for VNC viewer --autopass

[Matthew Daley <mattd@xxxxxxxxxxx>]

On Tue, Dec 3, 2013 at 1:22 AM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:
> Matthew Daley writes ("[PATCH 11/13] libxl: use pipe instead of temporary 
> file for VNC viewer --autopass"):
>> Coverity was complaining about the permissions implicitly set on the
>> temporary file used to pass the VNC password to the viewer when using
>> the --autopass feature. By replacing the use of the temporary file
>> with a pipe, we fix the problem (well, quiesce Coverity at least), tidy
>> the code and remove the buildup of temporary file cruft all at once.
>
> I don't think this is a good idea.  The VNC client may want to read
> the file more than once.

I know it's a handwavey argument, but doesn't passing the password
through stdin (vs. giving an explicit filename) imply that the file
could very well be a pipe (or at least unseekable)?

>
>> Coverity-ID: 1055958
>
> I think this is a false positive.
>
> From mkstemp(3) on Debian wheezy:
>
>        The file is created with permissions 0600, that is, read plus write for
>        owner only.  (In glibc versions 2.06 and earlier, the file  is  created
>        with  permissions  0666,  that  is, read and write for all users.)  The
>        returned file descriptor provides both read and  write  access  to  the
>        file.   The  file  is opened with the open(2) O_EXCL flag, guaranteeing
>        that the caller is the process that creates the file.
>
> From mkstemp(3) on FreeBSD 9.2-RELEASE:
>
>      The mkstemp() function makes the same replacement to the template and
>      creates the template file, mode 0600, returning a file descriptor opened
>      for reading and writing.  [...]
>
> I was going to say that if the standard spec doesn't have this
> behaviour, it's a bug.  But SuS agrees too.  From
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/mkstemp.html:
>
>     The mkstemp() function shall create the file, and obtain a file
>     descriptor for it, as if by a call to:
>
>     open(pathname, O_RDWR|O_CREAT|O_EXCL, S_IRUSR|S_IWUSR)
>
> So I think Coverity is just wrong about this.  Perhaps it thinks you
> are using an old version of glibc with an (IMO outrageous) security
> bug.

My `man mkstemp` says: "In glibc versions 2.06 and earlier, the file
is created with permissions 0666, that is, read and write for all
users.". So, a long time ago, you could very well have been outraged.

That said, I'm happy to drop this patch and tell Coverity to hush if
that's preferable.

- Matthew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel