Bug 17523 - open() and openat() ignore 'mode' with O_TMPFILE
Summary: open() and openat() ignore 'mode' with O_TMPFILE
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: 2.21
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-30 04:46 UTC by Eric Rannaud
Modified: 2016-08-15 08:26 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments
Test case (293 bytes, text/x-csrc)
2014-10-30 04:46 UTC, Eric Rannaud
Details
Proposed fix (2.45 KB, patch)
2014-10-30 04:56 UTC, Eric Rannaud
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Rannaud 2014-10-30 04:46:11 UTC
Created attachment 7863 [details]
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.
Comment 1 Eric Rannaud 2014-10-30 04:56:05 UTC
Created attachment 7864 [details]
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.
Comment 2 joseph@codesourcery.com 2014-10-30 20:17:45 UTC
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).
Comment 3 Eric Rannaud 2014-10-30 22:15:19 UTC
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).
Comment 4 Florian Weimer 2014-11-06 20:57:08 UTC
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.
Comment 5 Eric Rannaud 2014-11-06 21:33:34 UTC
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
Comment 6 Florian Weimer 2014-11-06 21:41:48 UTC
(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.
Comment 7 Eric Rannaud 2014-11-06 21:57:30 UTC
(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
Comment 8 cvs-commit@gcc.gnu.org 2015-02-24 07:50:38 UTC
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(-)
Comment 9 Siddhesh Poyarekar 2015-02-24 08:04:06 UTC
Fixed in master.
Comment 10 cvs-commit@gcc.gnu.org 2016-08-15 08:26:01 UTC
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, release/2.19/master has been updated
       via  9d328da0a82c8cba8dd7f9e537af2dd8ca60ebdf (commit)
      from  66986dec455c2011085a04b72a5bd55d9f9c7d1c (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=9d328da0a82c8cba8dd7f9e537af2dd8ca60ebdf

commit 9d328da0a82c8cba8dd7f9e537af2dd8ca60ebdf
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.
    	* ports/sysdeps/unix/sysv/linux/generic/open.c (__libc_open): Likewise.
    	(__open_nocancel): Likewise.
    	* ports/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.
    
    (cherry picked from commit 65f6f938cd562a614a68e15d0581a34b177ec29d)

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

Summary of changes:
 ChangeLog                                      |   25 ++++++++++++++++++++++++
 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 +-
 ports/sysdeps/unix/sysv/linux/generic/open.c   |    6 ++--
 ports/sysdeps/unix/sysv/linux/generic/open64.c |    4 +-
 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/open64.c               |    4 +-
 sysdeps/unix/sysv/linux/openat.c               |    6 ++--
 20 files changed, 82 insertions(+), 47 deletions(-)