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.
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.
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).
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).
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.
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
(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.
(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
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(-)
Fixed in master.
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(-)