[ruby-core:118932] [Ruby master Bug#20693] Dir.tmpdir should perform a real access check before warning about writability

[ruby-core:118932] [Ruby master Bug#20693] Dir.tmpdir should perform a real access check before warning about writability

[kjtsanaktsidis (KJ Tsanaktsidis) via ruby-core]

Issue #20693 has been reported by kjtsanaktsidis (KJ Tsanaktsidis).

----------------------------------------
Bug #20693: Dir.tmpdir should perform a real access check before warning about writability
https://bugs.ruby-lang.org/issues/20693

* Author: kjtsanaktsidis (KJ Tsanaktsidis)
* Status: Open
* Backport: 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN
----------------------------------------
The code in `Dir.tmpdir` attempts to warn the user if their temp directory is deficient for some reason:

```ruby
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.



-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/

[ruby-core:118936] [Ruby master Bug#20693] Dir.tmpdir should perform a real access check before warning about writability

[kjtsanaktsidis (KJ Tsanaktsidis) via ruby-core]

Issue #20693 has been updated by kjtsanaktsidis (KJ Tsanaktsidis).


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

----------------------------------------
Bug #20693: Dir.tmpdir should perform a real access check before warning about writability
https://bugs.ruby-lang.org/issues/20693#change-109505

* Author: kjtsanaktsidis (KJ Tsanaktsidis)
* Status: Open
* Backport: 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN
----------------------------------------
The code in `Dir.tmpdir` attempts to warn the user if their temp directory is deficient for some reason:

```ruby
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.



-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/

[ruby-core:118938] [Ruby master Bug#20693] Dir.tmpdir should perform a real access check before warning about writability

[Dan0042 (Daniel DeLorme) via ruby-core]

Issue #20693 has been updated by Dan0042 (Daniel DeLorme).


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.

----------------------------------------
Bug #20693: Dir.tmpdir should perform a real access check before warning about writability
https://bugs.ruby-lang.org/issues/20693#change-109507

* Author: kjtsanaktsidis (KJ Tsanaktsidis)
* Status: Open
* Backport: 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN
----------------------------------------
The code in `Dir.tmpdir` attempts to warn the user if their temp directory is deficient for some reason:

```ruby
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.



-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/

[ruby-core:118942] [Ruby master Bug#20693] Dir.tmpdir should perform a real access check before warning about writability

[kjtsanaktsidis (KJ Tsanaktsidis) via ruby-core]

Issue #20693 has been updated by kjtsanaktsidis (KJ Tsanaktsidis).


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?

----------------------------------------
Bug #20693: Dir.tmpdir should perform a real access check before warning about writability
https://bugs.ruby-lang.org/issues/20693#change-109512

* Author: kjtsanaktsidis (KJ Tsanaktsidis)
* Status: Open
* Backport: 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN
----------------------------------------
The code in `Dir.tmpdir` attempts to warn the user if their temp directory is deficient for some reason:

```ruby
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.



-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/