Bug #15555
closedDir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir
Description
The current implementation of the Dir.mktmpdir
is the following:
def Dir.mktmpdir(prefix_suffix=nil, *rest)
path = Tmpname.create(prefix_suffix || "d", *rest) {|n| mkdir(n, 0700)}
if block_given?
begin
yield path
ensure
stat = File.stat(File.dirname(path))
if stat.world_writable? and !stat.sticky?
raise ArgumentError, "parent directory is world writable but not sticky"
end
FileUtils.remove_entry path
end
else
path
end
end
When used explicitly with a directory, it raises an ArgumentError
when the directory is world writable (File::Stat#world_writable?
) but not sticky (File::Stat#sticky?
). This is a normal behavior. But with how the method is written, the behavior is inconsistent and raises an ArgumentError
only when using it with the block form. Worse, it raises the error in the ensure block instead of the first thing done in the method. So it raises the ArgumentError
after it yields to the block, so after the job is already done.
Proof:
irb(main):001:0> puts `ls -la /tmp`
total 4
drwxrwsrwx. 2 root 1000090000 6 Jan 22 10:02 .
drwxr-xr-x. 18 root root 4096 Jan 22 09:58 ..
=> nil
irb(main):002:0> # /tmp is non sticky but world writable
irb(main):003:0* Dir.mktmpdir(nil, '/tmp') do |tmpdir|
irb(main):004:1* puts "Hello, I'm using #{tmpdir}"
irb(main):005:1> end
Hello, I'm using /tmp/d20190122-26-5biuz2
ArgumentError: parent directory is world writable but not sticky
from /opt/rh/rh-ruby24/root/usr/share/ruby/tmpdir.rb:93:in `mktmpdir'
from (irb):3
from /opt/rh/rh-ruby24/root/usr/bin/irb:11:in `<top (required)>'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:74:in `load'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:74:in `kernel_load'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:27:in `run'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:362:in `exec'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:22:in `dispatch'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:13:in `start'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/exe/bundle:30:in `block in <top (required)>'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/friendly_errors.rb:121:in `with_friendly_errors'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/exe/bundle:22:in `<top (required)>'
from /opt/rh/rh-ruby24/root/usr/bin/bundle:23:in `load'
from /opt/rh/rh-ruby24/root/usr/bin/bundle:23:in `<main>'
irb(main):006:0> puts `ls -la /tmp`
total 4
drwxrwsrwx. 3 root 1000090000 33 Jan 22 10:03 .
drwxr-xr-x. 18 root root 4096 Jan 22 09:58 ..
drwx--S---. 2 1000090000 1000090000 6 Jan 22 10:03 d20190122-26-5biuz2
=> nil
We can also see that the allocated directory remains after the exception is raised as the implementation raised in the ensure block.
An advisable implementation would be:
def Dir.mktmpdir(prefix_suffix=nil, *rest)
path = Tmpname.create(prefix_suffix || "d", *rest) {|n| mkdir(n, 0700)}
stat = File.stat(File.dirname(path))
if stat.world_writable? and !stat.sticky?
FileUtils.remove_entry path
raise ArgumentError, "parent directory is world writable but not sticky"
end
if block_given?
begin
yield path
ensure
FileUtils.remove_entry path
end
else
path
end
end
This way, it will make the checks first and raise directly. And no temporary directory will be leaked with the block form.
As there is no maintainer for this lib, I can make a pull request with the following changes (with unit tests) if I'm asked to.
Updated by znz (Kazuhiro NISHIYAMA) over 5 years ago
https://github.com/ruby/ruby/commit/bcb9e567c422f535b4871ce2795179af808d0077 changes from FileUtils.remove_entry_secure
to checking stat and FileUtils.remove_entry
.
So I think splitting checking and FileUtils.remove_entry
is not good.
Updated by nobu (Nobuyoshi Nakada) about 5 years ago
- Status changed from Open to Closed
Applied in changeset trunk|r66909.
tmpdir.rb: permission of user given directory
- lib/tmpdir.rb (Dir.mktmpdir): check if the permission of the
parent directory only when using the default temporary
directory, and no check against user given directory. the
security is the user's responsibility in that case.
[ruby-core:91216] [Bug #15555]
Updated by nobu (Nobuyoshi Nakada) about 5 years ago
- Description updated (diff)
- Backport changed from 2.4: UNKNOWN, 2.5: UNKNOWN, 2.6: UNKNOWN to 2.4: REQUIRED, 2.5: REQUIRED, 2.6: REQUIRED
Updated by naruse (Yui NARUSE) about 5 years ago
- Backport changed from 2.4: REQUIRED, 2.5: REQUIRED, 2.6: REQUIRED to 2.4: REQUIRED, 2.5: REQUIRED, 2.6: DONE
ruby_2_6 r66941 merged revision(s) 66909.
Updated by usa (Usaku NAKAMURA) about 5 years ago
- Backport changed from 2.4: REQUIRED, 2.5: REQUIRED, 2.6: DONE to 2.4: DONE, 2.5: REQUIRED, 2.6: DONE
ruby_2_4 r67148 merged revision(s) 66909.
Updated by nagachika (Tomoyuki Chikanaga) about 5 years ago
- Backport changed from 2.4: DONE, 2.5: REQUIRED, 2.6: DONE to 2.4: DONE, 2.5: DONE, 2.6: DONE
ruby_2_5 r67241 merged revision(s) 66909.