[Bug libc/17523] New: open() and openat() ignore 'mode' with O_TMPFILE

[Bug libc/17523] New: open() and openat() ignore 'mode' with O_TMPFILE

[e at nanocritical dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=17523

            Bug ID: 17523
           Summary: open() and openat() ignore 'mode' with O_TMPFILE
           Product: glibc
           Version: 2.21
            Status: NEW
          Severity: normal
          Priority: P2
         Component: libc
          Assignee: unassigned at sourceware dot org
          Reporter: e at nanocritical dot com
                CC: drepper.fsp at gmail dot com

Created attachment 7863
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7863&action=edit
Test case

As the attached test case demonstrates (tested on glibc 2.20, Linux 3.13.5),

  open("/tmp", O_TMPFILE | O_RDWR, 0600) works
  openat(AT_FDCWD, "/tmp", O_TMPFILE | O_RDWR, 0600) does NOT work
  syscall(SYS_openat, AT_FDCWD, "/tmp", O_TMPFILE | O_RDWR, 0600) works

strace shows that the call to openat() fails with -1, EACCES

Note that if mode is 0,
  open("/tmp", O_TMPFILE | O_RDWR, 0) also fails with -1, EACCESS

Looking at the glibc source, it turns out that both open() and openat() are
incorrect: they load the 'mode' argument lazily, using va_arg() only if
O_CREAT is found in 'oflag'. This is wrong, 'mode' can also be used if
O_TMPFILE is in 'oflag'.

  if (oflag & O_CREAT)
    {
      va_list arg;
      va_start (arg, oflag);
      mode = va_arg (arg, int);
      va_end (arg);
    }

It is likely that no one has seen the problem until now because open()
gets away with it: indeed, at least on amd64, 'mode' is the 3rd argument to
open() and is in RDX. When open() gets to the SYSCALL instruction, even if
it skips va_arg(), 'mode' is still in RDX, where the syscall calling
convention expects it.

But openat() is not so lucky: 'mode' is the 4th argument, which is RCX. When
openat() get to SYSCALL, 'mode' is still in RCX, BUT the syscall calling
convention on amd64 requires that the 4th argument be in R10, which is 0 in
the test case.

Patch will follow.

Now, it is not clear to me why the kernel refuses to create an unamed
temporary file with mode 0, but that's a different bug.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/17523] open() and openat() ignore 'mode' with O_TMPFILE

[e at nanocritical dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=17523

Eric Rannaud <e at nanocritical dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |e at nanocritical dot com

--- Comment #1 from Eric Rannaud <e at nanocritical dot com> ---
Created attachment 7864
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7864&action=edit
Proposed fix

Patch written against 'master', successfully tested on top of Arch's glibc
2.20-2

Note that O_TMPFILE is a multi-bit mask, so the test becomes:

-  if (oflag & O_CREAT)
+  if ((oflag & O_CREAT) != 0 || (oflag & O_TMPFILE) == O_TMPFILE)

Using (oflag & O_TMPFILE) != 0 would trigger when just O_DIRECTORY is in oflag,
as the definition of O_TMPFILE is (see https://lwn.net/Articles/558951/)

#define O_TMPFILE (O_DIRECTORY | O_RDWR | __O_TMPFILE)

I've modified all instances of the test (oflag & O_CREAT) in the source as I
believe the change is required everywhere, but this should be reviewed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/17523] open() and openat() ignore 'mode' with O_TMPFILE

[joseph at codesourcery dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=17523

--- Comment #2 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
Please submit patches to libc-alpha.  Note that as O_TMPFILE is 
Linux-specific, it shouldn't be used in code outside of 
sysdeps/unix/sysv/linux/ (but it might be possible to add an additional 
macro that indicates whether there is a system-specific reason the mode 
argument is needed, and define that macro differently for Linux and other 
systems).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/17523] open() and openat() ignore 'mode' with O_TMPFILE

[e at nanocritical dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=17523

--- Comment #3 from Eric Rannaud <e at nanocritical dot com> ---
Understood: I defined a macro __OPEN_NEEDS_MODE(oflag) with a Linux-specific
variant. New patch sent to the list
(https://sourceware.org/ml/libc-alpha/2014-10/msg00773.html).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/17523] open() and openat() ignore 'mode' with O_TMPFILE

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=17523

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com
              Flags|                            |security-

--- Comment #4 from Florian Weimer <fweimer at redhat dot com> ---
I don't think this has any security impact in practice because the file mode is
not directly visible in the file system due to O_TMPFILE.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/17523] open() and openat() ignore 'mode' with O_TMPFILE

[e at nanocritical dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=17523

--- Comment #5 from Eric Rannaud <e at nanocritical dot com> ---
On x64_64:

  - open("/tmp", O_TMPFILE|O_WRONLY, 0600): the mode gets magically passed
correctly to the kernel, at least with the code generated by a "current" GCC
for glibc, at the "typical" optimization level for my distribution (Arch).

  - openat(AT_FDCWD, "/tmp", O_TMPFILE|O_WRONLY, 0600): the kernel gets
whatever happens to be in R10.

On other architectures, what mode the kernel sees really depends on the exact
code generated by the compiler, the calling conventions (userspace vs.
syscall), the exact glibc wrapper and whether nocancel or not.

It's possible the kernel reads an arbitrary mode value on non-x86_64 too.

Now, Florian is correct that the file created by O_TMPFILE is not immediately
visible on the filesystem. BUT, O_TMPFILE is likely to be used in the following
sequence, to implement a secure temporary file facility:

    fd = open("/path/to/dir", O_TMPFILE|O_WRONLY, 0600);
    snprintf(path, PATH_MAX,  "/proc/self/fd/%d", fd);
    linkat(AT_FDCWD, path, AT_FDCWD, "/path/for/file", AT_SYMLINK_FOLLOW);

Now, the file /path/for/file is created with an undefined mode, not 0600. A
workaround for such an implementation is to follow the open with an explicit
    fchmod(fd, 0600)

Also, the fortify wrappers are rendered ineffectual in the O_TMPFILE case.

So I believe this issue does have security implications.


Note: the kernel had a related issue, where the syscalls open/openat with
O_TMPFILE would fail with EACCES if mode was 0. A patch has gone in Linux
3.18-rc3 to allow that usage.
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=69a91c237ab0ebe4e9fdeaf6d0090c85275594ec

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/17523] open() and openat() ignore 'mode' with O_TMPFILE

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=17523

--- Comment #6 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Eric Rannaud from comment #5)
> Now, Florian is correct that the file created by O_TMPFILE is not
> immediately visible on the filesystem. BUT, O_TMPFILE is likely to be used
> in the following sequence, to implement a secure temporary file facility:
> 
>     fd = open("/path/to/dir", O_TMPFILE|O_WRONLY, 0600);
>     snprintf(path, PATH_MAX,  "/proc/self/fd/%d", fd);
>     linkat(AT_FDCWD, path, AT_FDCWD, "/path/for/file", AT_SYMLINK_FOLLOW);

Why would anyone want to do this instead of opening /path/for/file directly
with O_CREATE|O_EXCL?

Anyway, I can't find any applications using this pattern.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/17523] open() and openat() ignore 'mode' with O_TMPFILE

[e at nanocritical dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=17523

--- Comment #7 from Eric Rannaud <e at nanocritical dot com> ---
(In reply to Florian Weimer from comment #6)
> >     fd = open("/path/to/dir", O_TMPFILE|O_WRONLY, 0600);
> >     snprintf(path, PATH_MAX,  "/proc/self/fd/%d", fd);
> >     linkat(AT_FDCWD, path, AT_FDCWD, "/path/for/file", AT_SYMLINK_FOLLOW);
> 
> Why would anyone want to do this instead of opening /path/for/file directly
> with O_CREATE|O_EXCL?

An interesting application would set stricter permissions after open and before
linkat, such as:

    fsetxattr(fd, "security.selinux", ...)

This being said, I don't know if there any real users at this point. The need
for linkat on a mounted /proc (as opposed to a hypothetical flink(2)) appears
to make people think twice.

Samba people are looking to use this feature to emulate a regular open(O_CREAT)
that can atomically set ACLs before making the file visible in the filesystem:

https://lkml.org/lkml/2014/10/31/119
https://lkml.org/lkml/2014/11/3/843

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/17523] open() and openat() ignore 'mode' with O_TMPFILE

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=17523

--- Comment #8 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  65f6f938cd562a614a68e15d0581a34b177ec29d (commit)
       via  3e3002ffead0526d088c353f97475400367087da (commit)
      from  9813dd5835fa81c2e61c188fe08e28c9f3c60c07 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=65f6f938cd562a614a68e15d0581a34b177ec29d

commit 65f6f938cd562a614a68e15d0581a34b177ec29d
Author: Eric Rannaud <e@nanocritical.com>
Date:   Tue Feb 24 13:12:26 2015 +0530

    linux: open and openat ignore 'mode' with O_TMPFILE in flags

    Both open and openat load their last argument 'mode' lazily, using
    va_arg() only if O_CREAT is found in oflag. This is wrong, mode is also
    necessary if O_TMPFILE is in oflag.

    By chance on x86_64, the problem wasn't evident when using O_TMPFILE
    with open, as the 3rd argument of open, even when not loaded with
    va_arg, is left untouched in RDX, where the syscall expects it.

    However, openat was not so lucky, and O_TMPFILE couldn't be used: mode
    is the 4th argument, in RCX, but the syscall expects its 4th argument in
    a different register than the glibc wrapper, in R10.

    Introduce a macro __OPEN_NEEDS_MODE (oflag) to test if either O_CREAT or
    O_TMPFILE is set in oflag.

    Tested on Linux x86_64.

        [BZ #17523]
        * io/fcntl.h (__OPEN_NEEDS_MODE): New macro.
        * io/bits/fcntl2.h (open): Use it.
        (openat): Likewise.
        * io/open.c (__libc_open): Likewise.
        * io/open64.c (__libc_open64): Likewise.
        * io/open64_2.c (__open64_2): Likewise.
        * io/open_2.c (__open_2): Likewise.
        * io/openat.c (__openat): Likewise.
        * io/openat64.c (__openat64): Likewise.
        * io/openat64_2.c (__openat64_2): Likewise.
        * io/openat_2.c (__openat_2): Likewise.
        * sysdeps/mach/hurd/open.c (__libc_open): Likewise.
        * sysdeps/mach/hurd/openat.c (__openat): Likewise.
        * sysdeps/posix/open64.c (__libc_open64): Likewise.
        * sysdeps/unix/sysv/linux/dl-openat64.c (openat64): Likewise.
        * sysdeps/unix/sysv/linux/generic/open.c (__libc_open): Likewise.
        (__open_nocancel): Likewise.
        * sysdeps/unix/sysv/linux/generic/open64.c (__libc_open64): Likewise.
        * sysdeps/unix/sysv/linux/open64.c (__libc_open64): Likewise.
        * sysdeps/unix/sysv/linux/openat.c (__OPENAT): Likewise.

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=3e3002ffead0526d088c353f97475400367087da

commit 3e3002ffead0526d088c353f97475400367087da
Author: Siddhesh Poyarekar <siddhesh@redhat.com>
Date:   Tue Feb 24 12:57:26 2015 +0530

    Skip logging for DNSSEC responses [BZ 14841]

    DNSSEC defines a number of response types that one me expect when the
    DO bit is set.  We don't process any of them, but since we do allow
    setting the DO bit, skip them without logging an error since it is
    only a nuisance.

    Tested on x86_64.

        [BZ #14841]
        * resolv/gethnamaddr.c (getanswer): Skip logging if
        RES_USE_DNSSEC is set.
        * resolv/nss_dns/dns-host.c (getanswer_r): Likewise.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                |   31 ++++++++++++++++++++++++++++++
 NEWS                                     |    6 ++--
 io/bits/fcntl2.h                         |   18 ++++++++--------
 io/fcntl.h                               |   14 +++++++++++-
 io/open.c                                |    4 +-
 io/open64.c                              |    4 +-
 io/open64_2.c                            |    4 +-
 io/open_2.c                              |    4 +-
 io/openat.c                              |    4 +-
 io/openat64.c                            |    4 +-
 io/openat64_2.c                          |    4 +-
 io/openat_2.c                            |    4 +-
 resolv/gethnamaddr.c                     |   25 +++++++++--------------
 resolv/nss_dns/dns-host.c                |   23 +++++++--------------
 sysdeps/mach/hurd/open.c                 |    4 +-
 sysdeps/mach/hurd/openat.c               |    4 +-
 sysdeps/posix/open64.c                   |    4 +-
 sysdeps/unix/sysv/linux/dl-openat64.c    |    2 +-
 sysdeps/unix/sysv/linux/generic/open.c   |    6 ++--
 sysdeps/unix/sysv/linux/generic/open64.c |    4 +-
 sysdeps/unix/sysv/linux/open64.c         |    4 +-
 sysdeps/unix/sysv/linux/openat.c         |    6 ++--
 22 files changed, 106 insertions(+), 77 deletions(-)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/17523] open() and openat() ignore 'mode' with O_TMPFILE

[siddhesh at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=17523

Siddhesh Poyarekar <siddhesh at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |siddhesh at redhat dot com
         Resolution|---                         |FIXED

--- Comment #9 from Siddhesh Poyarekar <siddhesh at redhat dot com> ---
Fixed in master.

-- 
You are receiving this mail because:
You are on the CC list for the bug.