Merge pull request #549 from puppetlabs/GH-535-safe_directories

(GH-535) Fix for safe directories

Author: chelnak

README.md

@@ -39,7 +39,8 @@ The vcsrepo module provides a single type with providers to support the followin
 * [Subversion](#subversion)
 
 **Note:** `git` is the only vcs provider officially [supported by Puppet Inc.](https://forge.puppet.com/supported)
-**Note:** Release v4.0.1 has been removed from the Puppet Forge and was officially re-released as version v5.0.0 as it contained a breaking change. Details available [here](https://puppetlabs.github.io/iac/team/status/developer/2021/06/04/status-update.html)
+**Note:** Release v4.0.1 has been removed from the Puppet Forge and was officially re-released as version v5.0.0 as it contained a breaking change.
+Details available [here](https://puppetlabs.github.io/iac/team/status/developer/2021/06/04/status-update.html)
 
 <a id="setup"></a>
 ## Setup
@@ -813,6 +814,22 @@ The includes parameter is only supported when SVN client version is >= 1.6.
 
 For an extensive list of supported operating systems, see [metadata.json](https://github.com/puppetlabs/puppetlabs-vcsrepo/blob/main/metadata.json)
 
+### Response to CVE-2022-24765
+
+The vulnerability described in this CVE could impact users working on multi-user machines.
+A malicious actor could create a `.git` directory above the current working directory causing all git invocations to occur outside of a repository to read its configuration.
+
+For a more in-depth description of this vulnerability, check out [this blog post](https://github.blog/2022-04-12-git-security-vulnerability-announced/).
+
+Fixes were released in Git versions 2.35.2 and 1:2.25.1-1ubuntu3.4 respectively.
+
+VCSRepo users were impacted when running newer versions of Git and managing repositories that were owned by a user or group that differed from the user executing Git.
+
+For example, setting the `owner` parameter on a resource would cause Puppet runs to fail with a `Path /destination/path exists and is not the desired repository.` error.
+
+Impacted users are now advised to use the new `safe_directory` parameter on Git resources.
+Explicitily setting the value to `true` will add the current path specified on the resource to the `safe.directory` git configuration for the current user (global scope) allowing the Puppet run to continue without error.
+
 <a id="development"></a> 
 ## Development
 

lib/puppet/provider/vcsrepo/git.rb

@@ -6,7 +6,7 @@
   desc 'Supports Git repositories'
 
   has_features :bare_repositories, :reference_tracking, :ssh_identity, :multiple_remotes,
-               :user, :depth, :branch, :submodules
+               :user, :depth, :branch, :submodules, :safe_directory
 
   def create
     check_force
@@ -36,6 +36,7 @@ def create
   end
 
   def destroy
+    remove_safe_directory if safe_directories.include?(@resource.value(:path))
     FileUtils.rm_rf(@resource.value(:path))
   end
 
@@ -140,6 +141,7 @@ def working_copy_exists?
   end
 
   def exists?
+    update_safe_directory
     working_copy_exists? || bare_exists?
   end
 
@@ -564,6 +566,52 @@ def git_version
     exec_git('--version').match(%r{[0-9]+\.[0-9]+\.[0-9]+(\.[0-9]+)?})[0]
   end
 
+  # @!visibility private
+  def safe_directories
+    args = ['config', '--global', '--get-all', 'safe.directory']
+    begin
+      d = git_with_identity(*args) || ''
+      d.split('\n')
+       .reject { |v| v.empty? }
+       .map { |v| v.chomp }
+    rescue Puppet::ExecutionFailure
+      []
+    end
+  end
+
+  # @!visibility private
+  def update_safe_directory
+    # If the owner parameter is not set, then we don't need to do anything.
+    return unless @resource.value(:owner)
+
+    if should_add_safe_directory?
+      add_safe_directory
+    else
+      remove_safe_directory
+    end
+  end
+
+  # @!visibility private
+  def add_safe_directory
+    notice("Adding '#{@resource.value(:path)}' to safe directory list")
+    args = ['config', '--global', '--add', 'safe.directory', @resource.value(:path)]
+    git_with_identity(*args)
+  end
+
+  # @!visibility private
+  def remove_safe_directory
+    notice("Removing '#{@resource.value(:path)}' from safe directory list")
+    args = ['config', '--global', '--unset', 'safe.directory', @resource.value(:path)]
+    git_with_identity(*args)
+  end
+
+  # @!visibility private
+  def should_add_safe_directory?
+    (@resource.value(:owner) != @resource.value(:user)) && # user and owner should be different
+      @resource.value(:safe_directory) && # safe_directory should be true
+      !safe_directories.include?(@resource.value(:path)) # directory should not already be in the list
+  end
+
   # @!visibility private
   def git_with_identity(*args)
     if @resource.value(:trust_server_cert) == :true
@@ -599,10 +647,13 @@ def git_with_identity(*args)
 
   # Execute git with the given args, running it as the user specified.
   def exec_git(*args)
-    exec_args = { failonfail: true, combine: true }
+    exec_args = {
+      failonfail: true,
+      combine: true,
+      custom_environment: { 'HOME' => Dir.home },
+    }
     if @resource.value(:user) && @resource.value(:user) != Facter['id'].value
-      env = Etc.getpwnam(@resource.value(:user))
-      exec_args[:custom_environment] = { 'HOME' => env['dir'] }
+      exec_args[:custom_environment] = { 'HOME' => Dir.home(@resource.value(:user)) }
       exec_args[:uid] = @resource.value(:user)
     end
     Puppet::Util::Execution.execute([:git, args], **exec_args)

lib/puppet/type/vcsrepo.rb

@@ -61,6 +61,9 @@
   feature :keep_local_changes,
           'The provider supports keeping local changes on files tracked by the repository when changing revision'
 
+  feature :safe_directory,
+          'The provider supports setting a safe directory. This will only be used for newer versions of git.'
+
   ensurable do
     desc 'Ensure the version control repository.'
     attr_accessor :latest
@@ -112,7 +115,9 @@ def insync?(is)
     end
 
     newvalue :absent do
-      provider.destroy
+      if provider.exists?
+        provider.destroy
+      end
     end
 
     newvalue :latest, required_features: [:reference_tracking] do
@@ -301,6 +306,12 @@ def insync?(is)
     defaultto :false
   end
 
+  newparam :safe_directory, required_features: [:safe_directory] do
+    desc 'Marks the current directory specified by the path parameter as a safe directory.'
+    newvalues(true, false)
+    defaultto :false
+  end
+
   autorequire(:package) do
     ['git', 'git-core', 'mercurial', 'subversion']
   end

spec/acceptance/clone_repo_spec.rb

@@ -9,6 +9,7 @@
 
   after(:all) do
     run_shell("rm -rf #{tmpdir}/testrepo")
+    run_shell("rm -rf #{tmpdir}/testrepo_owner")
     run_shell("rm -rf #{tmpdir}/testrepo_mirror_repo")
   end
 
@@ -225,6 +226,7 @@
         provider => git,
         source => "file://#{tmpdir}/testrepo.git",
         owner => 'vagrant',
+        safe_directory => true,
       }
     MANIFEST
     it 'clones a repo' do

spec/unit/puppet/provider/vcsrepo/git_spec.rb

@@ -193,6 +193,7 @@ def branch_a_list(include_branch = nil?)
       expect(provider).to receive(:path_exists?).and_return(true)
       expect(provider).to receive(:path_empty?).and_return(false)
       provider.destroy
+      expect(provider).to receive(:exec_git).with('config', '--global', '--get-all', 'safe.directory')
       expect(provider).to receive(:exec_git).with('clone', resource.value(:source), resource.value(:path))
       expect(provider).to receive(:update_submodules)
       expect(provider).to receive(:update_remote_url).with('origin', resource.value(:source)).and_return false