Jira (PUP-9997) Puppet must not call Dir.chdir() except in a child process

[James Ralston (JIRA)]

Title: Message Title

James Ralston created an issue   Puppet / PUP-9997 Puppet must not call Dir.chdir() except in a child process Issue Type: Bug Assignee: Unassigned Created: 2019/09/04 4:00 PM Priority: Minor Reporter: James Ralston 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: 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. 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. At some point in the recent past, Puppet::Util::Execution.execute added the ability to specify the cwd of the command to be executed, and 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, this 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. Add Comment   This message was sent by Atlassian JIRA (v7.7.1#77002-sha1:e75ca93) -- You received this message because you are subscribed to the Google Groups "Puppet Bugs" group. To unsubscribe from this group and stop receiving emails from it, send an email to puppet-bugs+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-bugs/JIRA.323575.1567638000000.86169.1567638011220%40Atlassian.JIRA.