Validate the module name used to pick an install directory

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.

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?
     #

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

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.

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) }

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('')