Project

General

Profile

Bug #15555

Dir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir

Added by kbogtob (Karim Bogtob) 27 days ago. Updated 20 days ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-linux-gnu]
[ruby-core:91216]

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.

Associated revisions

Revision 1fae154c
Added by nobu (Nobuyoshi Nakada) 26 days ago

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]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@66909 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 66909
Added by nobu (Nobuyoshi Nakada) 26 days ago

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]

Revision fdca654c
Added by naruse (Yui NARUSE) 20 days ago

merge revision(s) 66909: [Backport #15555]

    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]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_6@66941 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 66941
Added by naruse (Yui NARUSE) 20 days ago

merge revision(s) 66909: [Backport #15555]

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]

History

#1

Updated by kbogtob (Karim Bogtob) 27 days ago

  • Description updated (diff)

Updated by znz (Kazuhiro NISHIYAMA) 26 days 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.

#3

Updated by nobu (Nobuyoshi Nakada) 26 days 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]
#4

Updated by nobu (Nobuyoshi Nakada) 20 days ago

  • Backport changed from 2.4: UNKNOWN, 2.5: UNKNOWN, 2.6: UNKNOWN to 2.4: REQUIRED, 2.5: REQUIRED, 2.6: REQUIRED
  • Description updated (diff)

Updated by naruse (Yui NARUSE) 20 days 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.

Also available in: Atom PDF