[PUP-9997] Puppet must not call Dir.chdir() except in a child process Created: 2019/09/04  Updated: 2019/10/04

Status: Accepted
Project: Puppet
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Minor
Reporter: James Ralston Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to PUP-5915 exec resources fail unless cwd is rea... Resolved
relates to PUP-5808 puppet via sudo from nfs home is not ... Closed
relates to PUP-6919 Puppet::Util::Windows::Process.execut... Closed
relates to PUP-8180 puppet agent -t fails from within ssh... Accepted
relates to PUP-4713 Race condition in node/environment.rb... Closed
relates to PUP-10080 Exec resources fail if the working di... Closed
Template: PUP Bug Template
Team: Coremunity
Method Found: Needs Assessment
QA Risk Assessment: Needs Assessment

 Description   

Puppet Version: 6.8.1
Puppet Server Version: 6.5.0
OS Name/Version: Red Hat Enterprise Linux 7

In Ruby, Dir.chdir changes the current working directory (cwd) of the Ruby process. Under the hood, it calls the the chdir() system call.

When Dir.chdir is passed a block, it notes the cwd, then calls chdir() and executes the block. When the block finishes, it calls chdir() again, supplying as the argument the previous cwd that it noted. (This is similar to the pushd/popd shell builtins.)

However, in Unix, it is possible for the cwd to be a directory that is inaccessible to the current process. This means that once you chdir() out of the cwd, attempting to chdir() to the old cwd will fail.

This is trivial to demonstrate for the non-root user:

$ cd /tmp
$ mkdir foo
$ chmod 0700 foo
$ cd foo
$ /usr/bin/pwd
/tmp/foo
$ cd /tmp/foo && echo noerror
noerror
$ chmod 0000 .
$ /usr/bin/pwd
/tmp/foo
$ cd /tmp/foo && echo noerror
-bash: cd: /tmp/foo: Permission denied

In most cases, the root user will never receive permission denied errors when attempting to chdir to a directory. But there are exceptions. One exception is a network filesystem where the local root user has no special privileges, such as Kerberized NFS. On Linux systems, removal of the CAP_DAC_OVERRIDE and CAP_DAC_READ_SEARCH capabilities will also prevent root from calling chdir() to an inaccessible directory.

So, putting this all together, we reach the following conclusion: the only time where it is safe to call Dir.chdir() is in a child process. Because:

  1. Calling Dir.chdir() without a block has a side-effect of changing the cwd for the execution of all subsequent code. This is undesirable, and dangerous.
  2. Calling Dir.chdir() with a block can fail to restore the cwd when the block exits (as per above), which then reduces to #1.

Previously, the cwd parameter of the exec resource was implemented using Dir.chdir() with a block argument. This caused failures for users who ran sudo puppet agent --test when sitting in a Kerberized NFS home directory that root could not execute, as the very first exec resource encountered would throw an exception when Dir.chdir() received EACCES when attempting to restore the old cwd when the block finished. This spawned at least two tickets: PUP-5808 and PUP-5915.

In PUP-6919 (specifically, commit ebb57c3e), the exec resource was rewritten to simply pass the cwd to Puppet::Util::Execution.execute instead of calling Dir.chdir() itself. Since Puppet::Util::Execution.execute doesn't call Dir.chdir() until after it forks the child process and before it calls exec(), and since calling chdir() in a child process doesn't change the cwd of the parent, PUP-6919 avoided calling Dir.chdir() in a non-child process, and thus quashed PUP-5808 and PUP-5915.

But: searching across the Puppet Ruby code, I see other instances where Dir.chdir() is being called.

For example, in lib/ruby/vendor_ruby/puppet/type/file/target.rb, we see this:

module Puppet
  Puppet::Type.type(:file).newproperty(:target) do
    ...
    def mklink
      ...
      Dir.chdir(File.dirname(@resource[:path])) do
        Puppet::Util::SUIDManager.asuser(@resource.asuser) do
          mode = @resource.should(:mode)
          if mode
            Puppet::Util.withumask(000) do
              Puppet::FileSystem.symlink(target, @resource[:path])
            end
          else
            Puppet::FileSystem.symlink(target, @resource[:path])
          end
        end
 
        @resource.send(:property_fix)
 
        :link_created
      end

I suspect this means that at least in some instances, using the file resource type to create symlinks can cause failures due to Dir.chdir() failing to restore the previous cwd when the block exits, which is exactly the same failure case as the old implementation of the exec resource.

As another example, in lib/ruby/vendor_ruby/puppet/node/environment.rb, we see this:

class Puppet::Node::Environment
  ...
  # Modules broken out by directory in the modulepath
  #
  # @note This method _changes_ the current working directory while enumerating
  #   the modules. This seems rather dangerous.
  #
  # @api public
  #
  # @return [Hash<String, Array<Puppet::Module>>] A hash whose keys are file
  #   paths, and whose values is an array of Puppet Modules for that path
  def modules_by_path
    modules_by_path = {}
    modulepath.each do |path|
      if Puppet::FileSystem.exist?(path)
        Dir.chdir(path) do
          module_names = Dir.entries(path).select do |name|
            Puppet::Module.is_module_directory?(name, path)
          end
          modules_by_path[path] = module_names.sort.map do |name|
            Puppet::Module.new(name, File.join(path, name), self)
          end
        end
      else
        modules_by_path[path] = []
      end
    end
    modules_by_path
  end

I would argue the comment has it right.

So, in summary, changes to the way the exec resource is implemented removed the most obvious showcase for the dangers of calling Dir.chdir(). But there are a handful of other calls to Dir.chdir() sprinkled throughout the Puppet code base. The code surrounding such calls should be rewritten such that the Dir.chdir() calls can be removed. If the Dir.chdir() call is unavoidable, it must occur in a child process, so as not to modify the cwd of the parent.



 Comments   
Comment by Jorie Tappa [ 2019/09/09 ]

Thanks for all the investigative work you've done on this James Ralston! If you want you are welcome to also file a PR to the puppet repo, otherwise, we'll pick this up as we are able.

Comment by John Bollinger [ 2019/10/04 ]

Does this mean that the fix for PUP-5915 didn't actually resolve that issue?  I'm inclined to think so, because I'm having trouble distinguishing it from PUP-10080, which I observed with Puppet 6.8.1 in a kerberized NFS home directory environment very similar to the one described here and in 5915.

In any case, I agree with the prescription that Dir.chdir() should be avoided wherever possible, and performed only in a child process when it cannot be avoided.  I suspect that making Execs avoid unnecessarily changing the working directory (i.e. when no cwd parameter is specified) will resolve 10080.

Generated at Thu Nov 21 02:49:58 PST 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.