diff --git a/lib/puppet/forge.rb b/lib/puppet/forge.rb index 53d999699e..8c6699a15f 100644 --- a/lib/puppet/forge.rb +++ b/lib/puppet/forge.rb @@ -150,7 +150,7 @@ def initialize(source, data) def install(dir) staging_dir = prepare - module_dir = dir + name[/-(.*)/, 1] + module_dir = Puppet::ModuleTool.module_dir_for(dir, name) module_dir.rmtree if module_dir.exist? # Make sure unpacked module has the same ownership as the folder we are moving it into. diff --git a/lib/puppet/module_tool.rb b/lib/puppet/module_tool.rb index 08379dd21a..a113628bda 100644 --- a/lib/puppet/module_tool.rb +++ b/lib/puppet/module_tool.rb @@ -41,6 +41,38 @@ def self.username_and_modname_from(full_module_name) end end + # Return the directory that the module named +full_module_name+ should be + # installed into, underneath +install_dir+. + # + # A module's name is read from its own metadata, so it cannot be trusted, + # and installing removes whatever directory the name resolves to. The name + # is therefore validated as a well-formed, namespaced module name, and the + # result is then confirmed to sit directly beneath +install_dir+. That + # second check cannot fail while the first one rejects every name + # containing a path separator, but it keeps the guarantee beside the path + # arithmetic it protects. + # + # @param install_dir [Pathname, String] the directory modules are installed into + # @param full_module_name [String] a namespaced module name, e.g. 'puppetlabs-stdlib' + # @return [Pathname] the directory the module should be installed into + # @raise [ArgumentError] if +full_module_name+ is not a well-formed, + # namespaced module name, or does not resolve to a directory directly + # beneath +install_dir+ + def self.module_dir_for(install_dir, full_module_name) + # Metadata validates the name and parses out the module portion of it, so + # that both rules live in one place. + module_name = Metadata.new.update('name' => full_module_name).module_name + + install_dir = Pathname.new(install_dir) + module_dir = install_dir + module_name + + unless module_dir.dirname.cleanpath == install_dir.cleanpath + raise ArgumentError, _("Module name %{full_module_name} does not resolve to a directory inside %{install_dir}") % { full_module_name: full_module_name, install_dir: install_dir } + end + + module_dir + end + # Find the module root when given a path by checking each directory up from # its current location until it finds one that satisfies is_module_root? # diff --git a/lib/puppet/module_tool/applications/unpacker.rb b/lib/puppet/module_tool/applications/unpacker.rb index b02ff954d0..7c35bf0481 100644 --- a/lib/puppet/module_tool/applications/unpacker.rb +++ b/lib/puppet/module_tool/applications/unpacker.rb @@ -35,7 +35,7 @@ def initialize(filename, options = {}) def run unpack sanity_check - module_dir = @module_path + module_name + module_dir = Puppet::ModuleTool.module_dir_for(@module_path, full_module_name) move_into(module_dir) # Return the Pathname object representing the directory where the @@ -76,9 +76,9 @@ def root_dir end # @api private - def module_name + def full_module_name metadata = Puppet::Util::Json.load((root_dir + 'metadata.json').read) - metadata['name'][/-(.*)/, 1] + metadata['name'] end # @api private diff --git a/lib/puppet/module_tool/local_tarball.rb b/lib/puppet/module_tool/local_tarball.rb index a76d2f8010..dd31481133 100644 --- a/lib/puppet/module_tool/local_tarball.rb +++ b/lib/puppet/module_tool/local_tarball.rb @@ -33,7 +33,7 @@ def prepare(release) def install(release, dir) staging_dir = release.prepare - module_dir = dir + release.name[/-(.*)/, 1] + module_dir = Puppet::ModuleTool.module_dir_for(dir, release.name) module_dir.rmtree if module_dir.exist? # Make sure unpacked module has the same ownership as the folder we are moving it into. diff --git a/spec/unit/module_tool/applications/unpacker_spec.rb b/spec/unit/module_tool/applications/unpacker_spec.rb index 604a196858..e21e906a25 100644 --- a/spec/unit/module_tool/applications/unpacker_spec.rb +++ b/spec/unit/module_tool/applications/unpacker_spec.rb @@ -73,6 +73,35 @@ expect(File).to be_directory(File.join(target, 'mytarball')) end + it "should refuse to install a module whose metadata name resolves outside the target directory" do + # The target is nested inside a directory of its own so that a regression + # cannot reach anything beyond this test, and the sentinel confirms that + # the parent directory was left alone. + parent = tmpdir("unpacker_parent") + nested_target = File.join(parent, 'modules') + sentinel = File.join(parent, 'sentinel') + FileUtils.mkdir(nested_target) + FileUtils.touch(sentinel) + + untar = double('Tar') + expect(untar).to receive(:unpack).with(filename, anything, anything) do |src, dest, _| + FileUtils.mkdir(File.join(dest, 'extractedmodule')) + File.open(File.join(dest, 'extractedmodule', 'metadata.json'), 'w+') do |file| + file.puts Puppet::Util::Json.dump('name' => 'myusername-..', 'version' => '1.0.0') + end + true + end + + expect(Puppet::ModuleTool::Tar).to receive(:instance).and_return(untar) + + expect { + Puppet::ModuleTool::Applications::Unpacker.run(filename, :target_dir => nested_target) + }.to raise_error(ArgumentError, /Invalid 'name' field in metadata\.json/) + + expect(File).to exist(sentinel) + expect(File).to be_directory(nested_target) + end + describe '.harmonize_ownership' do let(:source_stat) { instance_double(File::Stat, uid: 1010, gid: 2020) } let(:source) { instance_double(Pathname, stat: source_stat) } diff --git a/spec/unit/module_tool_spec.rb b/spec/unit/module_tool_spec.rb index d4e46d8bd8..35a9bcfa16 100644 --- a/spec/unit/module_tool_spec.rb +++ b/spec/unit/module_tool_spec.rb @@ -41,6 +41,47 @@ end end + describe '.module_dir_for' do + let(:install_dir) { Pathname.new('/etc/puppetlabs/code/modules') } + + it 'should return the module directory for a dash separated name' do + expect(subject.module_dir_for(install_dir, 'puppetlabs-stdlib')).to eq(install_dir + 'stdlib') + end + + it 'should return the module directory for a slash separated name' do + expect(subject.module_dir_for(install_dir, 'puppetlabs/stdlib')).to eq(install_dir + 'stdlib') + end + + it 'should accept a string install directory' do + expect(subject.module_dir_for(install_dir.to_s, 'puppetlabs-stdlib')).to eq(install_dir + 'stdlib') + end + + ['puppetlabs-..', 'puppetlabs-../../..', 'puppetlabs-/etc/cron.d', 'puppetlabs-.ssh'].each do |full_module_name| + it "should reject #{full_module_name.inspect}, which would resolve outside the install directory" do + expect { subject.module_dir_for(install_dir, full_module_name) }. + to raise_error(ArgumentError, /Invalid 'name' field in metadata\.json/) + end + end + + it 'should reject a name that is not namespaced' do + expect { subject.module_dir_for(install_dir, 'stdlib') }. + to raise_error(ArgumentError, /must be a namespaced module name/) + end + + it 'should reject a name whose module portion is not a valid module name' do + expect { subject.module_dir_for(install_dir, 'puppetlabs-my-module') }. + to raise_error(ArgumentError, /non-alphanumeric/) + end + + it 'should reject a module name that resolves outside the install directory even if it validates' do + updated = double('updated metadata', :module_name => '..') + allow(Puppet::ModuleTool::Metadata).to receive(:new).and_return(double('metadata', :update => updated)) + + expect { subject.module_dir_for(install_dir, 'puppetlabs-stdlib') }. + to raise_error(ArgumentError, /does not resolve to a directory inside/) + end + end + describe '.format_tree' do it 'should return an empty tree when given an empty list' do expect(subject.format_tree([])).to eq('')