Skip to content

Commit

Permalink
(#14333) Ensure module permissions are sane.
Browse files Browse the repository at this point in the history
Prior to this change, tarballs unpacked by the root user would be unpacked
with the permissions and the ownership information present in the tarball.
In the best case, this made some modules fail to function after installation
since the newly installed module was owned by a different user. In the worst
case, this could be seen as a fairly serious security vulnerability.

This change ensures that the module's permissions are always owner-writable,
world-readable, and that the owner is always the same as the module's parent
directory.
  • Loading branch information
pvande authored and zaphod42 committed Aug 2, 2013
1 parent e160e99 commit 90d4180
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 1 deletion.
6 changes: 5 additions & 1 deletion lib/puppet/module_tool/applications/unpacker.rb
Expand Up @@ -9,7 +9,8 @@ def initialize(filename, options = {})
@filename = Pathname.new(filename)
parsed = parse_filename(filename)
super(options)
@module_dir = Pathname.new(options[:target_dir]) + parsed[:dir_name]
@module_path = Pathname(options[:target_dir])
@module_dir = @module_path + parsed[:dir_name]
end

def run
Expand Down Expand Up @@ -46,6 +47,9 @@ def extract_module_to_install_dir
else
Puppet::Util.execute("tar xzf #{@filename} -C #{build_dir}")
end
Puppet::Util.execute("find #{build_dir} -type d -exec chmod 755 {} +")
Puppet::Util.execute("find #{build_dir} -type f -exec chmod 644 {} +")
Puppet::Util.execute("chown -R #{@module_path.stat.uid} #{build_dir}")
rescue Puppet::ExecutionFailure => e
raise RuntimeError, "Could not extract contents of module archive: #{e.message}"
end
Expand Down
6 changes: 6 additions & 0 deletions spec/unit/module_tool/applications/unpacker_spec.rb
Expand Up @@ -36,6 +36,9 @@
context "on linux" do
it "should attempt to untar file to temporary location using system tar" do
Puppet::Util.expects(:execute).with("tar xzf #{filename} -C #{build_dir}").returns(true)
Puppet::Util.expects(:execute).with("find #{build_dir} -type d -exec chmod 755 {} +").returns(true)
Puppet::Util.expects(:execute).with("find #{build_dir} -type f -exec chmod 644 {} +").returns(true)
Puppet::Util.expects(:execute).with("chown -R #{build_dir.stat.uid} #{build_dir}").returns(true)
unpacker.run
end
end
Expand All @@ -48,6 +51,9 @@
it "should attempt to untar file to temporary location using gnu tar" do
Puppet::Util.stubs(:which).with('gtar').returns('/usr/sfw/bin/gtar')
Puppet::Util.expects(:execute).with("gtar xzf #{filename} -C #{build_dir}").returns(true)
Puppet::Util.expects(:execute).with("find #{build_dir} -type d -exec chmod 755 {} +").returns(true)
Puppet::Util.expects(:execute).with("find #{build_dir} -type f -exec chmod 644 {} +").returns(true)
Puppet::Util.expects(:execute).with("chown -R #{build_dir.stat.uid} #{build_dir}").returns(true)
unpacker.run
end

Expand Down

0 comments on commit 90d4180

Please sign in to comment.