[PUP-9997] Puppet must not call Dir.chdir() except in a child process Created: 2019/09/04 Updated: 2019/10/04
|Remaining Estimate:||Not Specified|
|Time Spent:||Not Specified|
|Original Estimate:||Not Specified|
|Template:||PUP Bug Template customfield_10700 323575|
|Method Found:||Needs Assessment|
|QA Risk Assessment:||Needs Assessment|
Puppet Version: 6.8.1
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:
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:
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:
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:
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:
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.
|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
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.