Bug #20693
closedDir.tmpdir should perform a real access check before warning about writability
Description
The code in Dir.tmpdir
attempts to warn the user if their temp directory is deficient for some reason:
case
when !stat.directory?
warn "#{name} is not a directory: #{dir}"
when !stat.writable?
warn "#{name} is not writable: #{dir}"
when stat.world_writable? && !stat.sticky?
warn "#{name} is world-writable: #{dir}"
else
break dir
end
This check for writability is looking at the user/group/world access bits on the stat output, and determining if the user running Ruby is allowed to write to the temp directory based on that.
However, modern operating systems contain other mechanisms apart from the user/group/world bits which can grant access to a directory that would otherwise be denied, or vice versa. Things like:
- Posix ACL's
- Linux's capabilities like CAP_DAC_OVERRIDE
- Linux Security Modules like SELinux or AppArmor
- Syscall filters like Linux's seccomp
- Granular capability systems like FreeBSD's Capsicum
- OpenBSD's pledge and unveil
- Windows too has a rich ACL system for controlling filesystem access
To address this, we should call File.writable?
instead of stat.writable?
, which asks the system whether the file is writable using the euidaccess()
function if available. On Linux/glibc, at least, this will issue an access(2)
syscall, and the Kernel can take all of the above into account.
n.b. if Ruby is running as suid, then glibc currently will NOT ask the kernel to perform the access check in euidaccess()
, and instead does a similar thing to what Stat#writable?
does (https://github.com/bminor/glibc/blob/7f04bb4e49413bd57ac3215f3480b09ae7131968/sysdeps/posix/euidaccess.c#L159-L162). This is because of the relatively new faccessat2(2)
syscall is required to do this properly, and there is some ecosystem issues with leveraging this by default (e.g. https://bugzilla.redhat.com/show_bug.cgi?id=1900021). Since running Ruby as suid is probably a very bad idea anyway, and the glibc implementation isn't any worse than the Stat#writable?
one, this seems OK though.
Updated by kjtsanaktsidis (KJ Tsanaktsidis) 2 months ago
I have a PR for doing this: https://github.com/ruby/ruby/pull/11403
But actually it breaks Bundler's tests, because they stub File.writable?
in a test. I opened a fix for Bundler which needs to land first: https://github.com/rubygems/rubygems/pull/7961
Updated by Dan0042 (Daniel DeLorme) 2 months ago · Edited
What about changing/fixing stat.writable?
to behave like File.writable?
It seems to me a source of confusion and subtle bugs that these two methods can return different values in edge cases.
It's not just the code in Dir.tmpdir
that is deficient, it's everything currently using stat.writable?
Updated by kjtsanaktsidis (KJ Tsanaktsidis) 2 months ago
I did think about this, but the way the stat methods work is that the stat(2)
syscall once on the path and returns a Stat
struct. Then all the predicate methods operate on that structure in Ruby without more syscalls.
There just isn’t really a sensible way to answer writable?
from that data, and it would be strange if that method (and readable?
) did new syscalls and got up to date data while the rest of the stat predicates did not.
I actually wonder if we should deprecate Stat#writable?
and Stat#readable?
because of this?
Updated by mame (Yusuke Endoh) about 1 month ago
- Assignee set to akr (Akira Tanaka)
Updated by akr (Akira Tanaka) about 1 month ago
I think the reason why File.writable?
is used should be described in the source.
Updated by kjtsanaktsidis (KJ Tsanaktsidis) 16 days ago
Sorry it took me so long to get back to you @akr (Akira Tanaka) . I've added a short comment about this - if you're OK with it I will commit it. Thanks!
Updated by kjtsanaktsidis (KJ Tsanaktsidis) 14 days ago
- Status changed from Open to Closed
- Backport changed from 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN to 3.1: DONTNEED, 3.2: DONTNEED, 3.3: DONTNEED
Merged in 7d254e4a2e16dd6275452a2a67b0fcd600cdc990