From 0df08e96c397014ec7d3b64dd9e6b5495c08462a Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Fri, 24 May 2024 17:51:35 -0700 Subject: [PATCH 1/4] (PUP-9997) Don't Dir.chdir when installing modules When the Dir.chdir block ends we may not have permission to switch our cwd back to where we started. So keep the cwd as is and pass the dest dir to the tar/find/chown commands. Since we're passing arbitrary destination directories, escape it. --- lib/puppet/module_tool/tar/gnu.rb | 18 ++++++++++-------- spec/unit/module_tool/tar/gnu_spec.rb | 22 +++++++++++++--------- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/lib/puppet/module_tool/tar/gnu.rb b/lib/puppet/module_tool/tar/gnu.rb index 8b2c59dd411..d3092e5683e 100644 --- a/lib/puppet/module_tool/tar/gnu.rb +++ b/lib/puppet/module_tool/tar/gnu.rb @@ -4,18 +4,20 @@ class Puppet::ModuleTool::Tar::Gnu def unpack(sourcefile, destdir, owner) - sourcefile = File.expand_path(sourcefile) + safe_sourcefile = Shellwords.shellescape(File.expand_path(sourcefile)) destdir = File.expand_path(destdir) + safe_destdir = Shellwords.shellescape(destdir) - Dir.chdir(destdir) do - Puppet::Util::Execution.execute("gzip -dc #{Shellwords.shellescape(sourcefile)} | tar xof -") - Puppet::Util::Execution.execute("find . -type d -exec chmod 755 {} +") - Puppet::Util::Execution.execute("find . -type f -exec chmod u+rw,g+r,a-st {} +") - Puppet::Util::Execution.execute("chown -R #{owner} .") - end + Puppet::Util::Execution.execute("gzip -dc #{safe_sourcefile} | tar --extract --no-same-owner --directory #{safe_destdir} --file -") + Puppet::Util::Execution.execute(['find', destdir, '-type', 'd', '-exec', 'chmod', '755', '{}', '+']) + Puppet::Util::Execution.execute(['find', destdir, '-type', 'f', '-exec', 'chmod', 'u+rw,g+r,a-st', '{}', '+']) + Puppet::Util::Execution.execute(['chown', '-R', owner, destdir]) end def pack(sourcedir, destfile) - Puppet::Util::Execution.execute("tar cf - #{sourcedir} | gzip -c > #{File.basename(destfile)}") + safe_sourcedir = Shellwords.shellescape(sourcedir) + safe_destfile = Shellwords.shellescape(File.basename(destfile)) + + Puppet::Util::Execution.execute("tar cf - #{safe_sourcedir} | gzip -c > #{safe_destfile}") end end diff --git a/spec/unit/module_tool/tar/gnu_spec.rb b/spec/unit/module_tool/tar/gnu_spec.rb index 4123303c7c7..54828e3e1de 100644 --- a/spec/unit/module_tool/tar/gnu_spec.rb +++ b/spec/unit/module_tool/tar/gnu_spec.rb @@ -1,23 +1,27 @@ require 'spec_helper' require 'puppet/module_tool' -describe Puppet::ModuleTool::Tar::Gnu do +describe Puppet::ModuleTool::Tar::Gnu, unless: Puppet::Util::Platform.windows? do + let(:sourcedir) { '/space path/the/src/dir' } let(:sourcefile) { '/space path/the/module.tar.gz' } let(:destdir) { '/space path/the/dest/dir' } - let(:sourcedir) { '/space path/the/src/dir' } - let(:destfile) { '/space path/the/dest/file.tar.gz' } + let(:destfile) { '/space path/the/dest/fi le.tar.gz' } + + let(:safe_sourcedir) { '/space\ path/the/src/dir' } + let(:safe_sourcefile) { '/space\ path/the/module.tar.gz' } + let(:safe_destdir) { '/space\ path/the/dest/dir' } + let(:safe_destfile) { 'fi\ le.tar.gz' } it "unpacks a tar file" do - expect(Dir).to receive(:chdir).with(File.expand_path(destdir)).and_yield - expect(Puppet::Util::Execution).to receive(:execute).with("gzip -dc #{Shellwords.shellescape(File.expand_path(sourcefile))} | tar xof -") - expect(Puppet::Util::Execution).to receive(:execute).with("find . -type d -exec chmod 755 {} +") - expect(Puppet::Util::Execution).to receive(:execute).with("find . -type f -exec chmod u+rw,g+r,a-st {} +") - expect(Puppet::Util::Execution).to receive(:execute).with("chown -R .") + expect(Puppet::Util::Execution).to receive(:execute).with("gzip -dc #{safe_sourcefile} | tar --extract --no-same-owner --directory #{safe_destdir} --file -") + expect(Puppet::Util::Execution).to receive(:execute).with(['find', destdir, '-type', 'd', '-exec', 'chmod', '755', '{}', '+']) + expect(Puppet::Util::Execution).to receive(:execute).with(['find', destdir, '-type', 'f', '-exec', 'chmod', 'u+rw,g+r,a-st', '{}', '+']) + expect(Puppet::Util::Execution).to receive(:execute).with(['chown', '-R', '', destdir]) subject.unpack(sourcefile, destdir, '') end it "packs a tar file" do - expect(Puppet::Util::Execution).to receive(:execute).with("tar cf - #{sourcedir} | gzip -c > #{File.basename(destfile)}") + expect(Puppet::Util::Execution).to receive(:execute).with("tar cf - #{safe_sourcedir} | gzip -c > #{safe_destfile}") subject.pack(sourcedir, destfile) end end From ee46e7866316801d1ffe7b1c85f29f78c733512d Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Fri, 24 May 2024 18:04:01 -0700 Subject: [PATCH 2/4] (PUP-9997) Pass cwd to execute method Dir.chdir is problematic because it affects all threads in the current process and if puppet is started with a current working directory it doesn't have traverse/execute permission to, then it won't be able to restore the cwd at the end of the Dir.chdir block. Puppet supports three execution implementations: posix, windows and stub. The first two already support the `cwd` option. Puppetserver injects its stub implementation and it recently added support for `cwd`, see SERVER-3051. --- lib/puppet/parser/functions/generate.rb | 3 +- spec/unit/parser/functions/generate_spec.rb | 73 ++++++++++++++++++--- 2 files changed, 66 insertions(+), 10 deletions(-) diff --git a/lib/puppet/parser/functions/generate.rb b/lib/puppet/parser/functions/generate.rb index b81b5c04cab..b52eb4a7f36 100644 --- a/lib/puppet/parser/functions/generate.rb +++ b/lib/puppet/parser/functions/generate.rb @@ -31,7 +31,8 @@ end begin - Dir.chdir(File.dirname(args[0])) { Puppet::Util::Execution.execute(args).to_str } + dir = File.dirname(args[0]) + Puppet::Util::Execution.execute(args, failonfail: true, combine: true, cwd: dir).to_str rescue Puppet::ExecutionFailure => detail raise Puppet::ParseError, _("Failed to execute generator %{generator}: %{detail}") % { generator: args[0], detail: detail }, detail.backtrace end diff --git a/spec/unit/parser/functions/generate_spec.rb b/spec/unit/parser/functions/generate_spec.rb index b038d606f2f..2d2254510b0 100644 --- a/spec/unit/parser/functions/generate_spec.rb +++ b/spec/unit/parser/functions/generate_spec.rb @@ -1,11 +1,33 @@ require 'spec_helper' +def with_executor + return yield unless Puppet::Util::Platform.jruby? + + begin + Puppet::Util::ExecutionStub.set do |command, options, stdin, stdout, stderr| + require 'open3' + # simulate what puppetserver does + Dir.chdir(options[:cwd]) do + out, err, _status = Open3.capture3(*command) + stdout.write(out) + stderr.write(err) + # execution api expects stdout to be returned + out + end + end + yield + ensure + Puppet::Util::ExecutionStub.reset + end +end + describe "the generate function" do include PuppetSpec::Files let :node do Puppet::Node.new('localhost') end let :compiler do Puppet::Parser::Compiler.new(node) end let :scope do Puppet::Parser::Scope.new(compiler) end + let :cwd do tmpdir('generate') end it "should exist" do expect(Puppet::Parser::Functions.function("generate")).to eq("function_generate") @@ -13,7 +35,7 @@ it "accept a fully-qualified path as a command" do command = File.expand_path('/command/foo') - expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay") + expect(Puppet::Util::Execution).to receive(:execute).with([command], anything).and_return("yay") expect(scope.function_generate([command])).to eq("yay") end @@ -35,33 +57,66 @@ end it "should execute the generate script with the correct working directory" do - command = File.expand_path("/command") - expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay") - expect(scope.function_generate([command])).to eq('yay') + command = File.expand_path("/usr/local/command") + expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: %r{/usr/local})).and_return("yay") + scope.function_generate([command]) + end + + it "should execute the generate script with failonfail" do + command = File.expand_path("/usr/local/command") + expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(failonfail: true)).and_return("yay") + scope.function_generate([command]) + end + + it "should execute the generate script with combine" do + command = File.expand_path("/usr/local/command") + expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(combine: true)).and_return("yay") + scope.function_generate([command]) + end + + it "executes a command in a working directory" do + if Puppet::Util::Platform.windows? + command = File.join(cwd, 'echo.bat') + File.write(command, <<~END) + @echo off + echo %CD% + END + expect(scope.function_generate([command]).chomp).to match(cwd.gsub('/', '\\')) + else + with_executor do + command = File.join(cwd, 'echo.sh') + File.write(command, <<~END) + #!/bin/sh + echo $PWD + END + Puppet::FileSystem.chmod(0755, command) + expect(scope.function_generate([command]).chomp).to eq(cwd) + end + end end describe "on Windows", :if => Puppet::Util::Platform.windows? do it "should accept the tilde in the path" do command = "C:/DOCUME~1/ADMINI~1/foo.bat" - expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay") + expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: "C:/DOCUME~1/ADMINI~1")).and_return("yay") expect(scope.function_generate([command])).to eq('yay') end it "should accept lower-case drive letters" do command = 'd:/command/foo' - expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay") + expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: "d:/command")).and_return("yay") expect(scope.function_generate([command])).to eq('yay') end it "should accept upper-case drive letters" do command = 'D:/command/foo' - expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay") + expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: "D:/command")).and_return("yay") expect(scope.function_generate([command])).to eq('yay') end it "should accept forward and backslashes in the path" do command = 'D:\command/foo\bar' - expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay") + expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: 'D:\command/foo')).and_return("yay") expect(scope.function_generate([command])).to eq('yay') end @@ -81,7 +136,7 @@ it "should accept plus and dash" do command = "/var/folders/9z/9zXImgchH8CZJh6SgiqS2U+++TM/-Tmp-/foo" - expect(Dir).to receive(:chdir).with(File.dirname(command)).and_return("yay") + expect(Puppet::Util::Execution).to receive(:execute).with([command], hash_including(cwd: '/var/folders/9z/9zXImgchH8CZJh6SgiqS2U+++TM/-Tmp-')).and_return("yay") expect(scope.function_generate([command])).to eq('yay') end end From cd0f3ce8ed374052d71717cef6f46f2e15e76180 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Thu, 27 Jun 2024 14:38:30 -0700 Subject: [PATCH 3/4] (PUP-9997) Remove dead code The Puppet::Util::Reference.pdf method has been broken since 2012 due to commit f0c99956e7e3dcd68bdec570cbc32adf88b53163, because it was calling Puppet::Util.replace_file with 1 argumentm, but it takes 2, resulting in: $ puppet doc --mode pdf --reference type creating pdf Error: Could not run: wrong number of arguments (given 1, expected 2) So delete the code and remove `pdf` as a valid reference doc mode. --- lib/puppet/application/doc.rb | 6 +----- lib/puppet/util/reference.rb | 31 +------------------------------ 2 files changed, 2 insertions(+), 35 deletions(-) diff --git a/lib/puppet/application/doc.rb b/lib/puppet/application/doc.rb index b6da159d96b..ae194a554b0 100644 --- a/lib/puppet/application/doc.rb +++ b/lib/puppet/application/doc.rb @@ -173,11 +173,7 @@ def other text += Puppet::Util::Reference.footer unless with_contents # We've only got one reference - if options[:mode] == :pdf - Puppet::Util::Reference.pdf(text) - else - puts text - end + puts text exit exit_code end diff --git a/lib/puppet/util/reference.rb b/lib/puppet/util/reference.rb index bba7d138783..8464cfe90c3 100644 --- a/lib/puppet/util/reference.rb +++ b/lib/puppet/util/reference.rb @@ -13,7 +13,7 @@ class Puppet::Util::Reference instance_load(:reference, 'puppet/reference') def self.modes - %w[pdf text] + %w[text] end def self.newreference(name, options = {}, &block) @@ -32,35 +32,6 @@ def self.page(*sections) end end - def self.pdf(text) - puts _("creating pdf") - rst2latex = which('rst2latex') || which('rst2latex.py') || - raise(_("Could not find rst2latex")) - - cmd = %(#{rst2latex} /tmp/puppetdoc.txt > /tmp/puppetdoc.tex) - Puppet::Util.replace_file("/tmp/puppetdoc.txt") { |f| f.puts text } - # There used to be an attempt to use secure_open / replace_file to secure - # the target, too, but that did nothing: the race was still here. We can - # get exactly the same benefit from running this effort: - begin - Puppet::FileSystem.unlink('/tmp/puppetdoc.tex') - rescue - nil - end - output = %x(#{cmd}) - unless $CHILD_STATUS == 0 - $stderr.puts _("rst2latex failed") - $stderr.puts output - exit(1) - end - $stderr.puts output - - # Now convert to pdf - Dir.chdir("/tmp") do - %x(texi2pdf puppetdoc.tex >/dev/null 2>/dev/null) - end - end - def self.references(environment) instance_loader(:reference).loadall(environment) loaded_instances(:reference).sort_by(&:to_s) From 3c1cac617f5a5cbc434becaadba90055982828b3 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Thu, 27 Jun 2024 17:51:06 -0700 Subject: [PATCH 4/4] (PUP-9997) Remove call to Dir.chdir when managing symlinks Symlinks can refer to their target using a relative or absolute path. If a relative path is used, then it's assumed to be relative to the symlink's parent directory. For example, /tmp/link refers to /tmp/target using a relative path: $ ls -l /tmp/link lrwxrwxrwx 1 josh 6 Jun 27 23:26 /tmp/link -> target In commit 02f91fcd55, symlinks were merged into the file (aka pfile) type. The `ensure` property was modified to validate whether the target existed or was a directory[1] In order for relative symlinks to be validated, it was necessary to call `Dir.chdir`. Later the call to `Dir.chdir` was moved to `target.rb`[2], and then the `FileTest.exists?(target)` check was deleted[3]. This commit removes the call to `Dir.chdir`. This means we no longer change our cwd prior to dropping privileges and creating the symlink. This should be ok because none of the code within the Dir.chdir block is sensitive to cwd. Also when we reach this block of code, we're committed to creating the symlink given an absolute `@resource[:path]`, so we don't need to be concerned about TOCTOU race conditions. [1] https://github.com/puppetlabs/puppet/blob/02f91fcd55aefe63a11a29c5608c0738867b8130/lib/puppet/type/pfile/ensure.rb#L76-L86 [2] https://github.com/puppetlabs/puppet/blob/2cd67ad843409a49747748a1278e5ef788dd97bd/lib/puppet/type/pfile/target.rb#L41-L45 [3] https://github.com/puppetlabs/puppet/commit/662fcafdb8020b34b060d70d54fca996ece36ab8#diff-03f0464c0f42c4b979f6150ec2f7e8043ec964fe4d401cd0cd57a82f43a856ba --- lib/puppet/type/file/target.rb | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/lib/puppet/type/file/target.rb b/lib/puppet/type/file/target.rb index 973e5ddf9fa..b7cd6b62a9e 100644 --- a/lib/puppet/type/file/target.rb +++ b/lib/puppet/type/file/target.rb @@ -44,22 +44,20 @@ def mklink raise Puppet::Error, "Could not remove existing file" if Puppet::FileSystem.exist?(@resource[:path]) - Dir.chdir(File.dirname(@resource[:path])) do - Puppet::Util::SUIDManager.asuser(@resource.asuser) do - mode = @resource.should(:mode) - if mode - Puppet::Util.withumask(0o00) do - Puppet::FileSystem.symlink(target, @resource[:path]) - end - else + Puppet::Util::SUIDManager.asuser(@resource.asuser) do + mode = @resource.should(:mode) + if mode + Puppet::Util.withumask(0o00) do Puppet::FileSystem.symlink(target, @resource[:path]) end + else + Puppet::FileSystem.symlink(target, @resource[:path]) end + end - @resource.send(:property_fix) + @resource.send(:property_fix) - :link_created - end + :link_created end def insync?(currentvalue)