Security: randomize path of git wrapper script

mattbrictson · mattbrictson · commit 9c92ad51ed29 · 2021-01-05T07:57:02.000-08:00
Before, the `:git_wrapper_path` was a somewhat predictable value and
located in `/tmp` by default, which is world-writable. That meant that
there was a chance (albeit very small) that another process could guess
the path and overwrite it with something malicious.

Fix by randomly generating a path name so that the git wrapper script
location cannot be predicted.

This change should be transparent to capistrano users since the
`:git_wrapper_path` is only intended to be used internally. If you need
a predictable value for this path, set a custom value for
`:git_wrapper_path` in your `deploy.rb` file.
diff --git a/features/step_definitions/assertions.rb b/features/step_definitions/assertions.rb
@@ -5,7 +5,7 @@
 end
 
 Then(/^git wrapper permissions are 0700$/) do
-  permissions_test = %Q([ $(stat -c "%a" #{TestApp.git_wrapper_path.shellescape}) == "700" ])
+  permissions_test = %Q([ $(stat -c "%a" #{TestApp.git_wrapper_path_glob}) == "700" ])
   _stdout, _stderr, status = vagrant_cli_command("ssh -c #{permissions_test.shellescape}")
 
   expect(status).to be_success
diff --git a/lib/capistrano/scm/git.rb b/lib/capistrano/scm/git.rb
@@ -1,16 +1,16 @@
 require "capistrano/scm/plugin"
 require "cgi"
+require "securerandom"
 require "shellwords"
 require "uri"
 
 class Capistrano::SCM::Git < Capistrano::SCM::Plugin
   def set_defaults
     set_if_empty :git_shallow_clone, false
     set_if_empty :git_wrapper_path, lambda {
-      # Try to avoid permissions issues when multiple users deploy the same app
-      # by using different file names in the same dir for each deployer and stage.
-      suffix = %i(application stage local_user).map { |key| fetch(key).to_s }.join("-")
-      "#{fetch(:tmp_dir)}/git-ssh-#{suffix}.sh"
+      # Use a unique name that won't collide with other deployments, and
+      # that cannot be guessed by other processes that have access to /tmp.
+      "#{fetch(:tmp_dir)}/git-ssh-#{SecureRandom.hex(10)}.sh"
     }
     set_if_empty :git_environmental_variables, lambda {
       {
diff --git a/spec/lib/capistrano/scm/git_spec.rb b/spec/lib/capistrano/scm/git_spec.rb
@@ -28,13 +28,10 @@ module Capistrano
     end
 
     describe "#set_defaults" do
-      it "makes git_wrapper_path using application, stage, and local_user" do
+      it "makes git_wrapper_path using a random hex value" do
         env.set(:tmp_dir, "/tmp")
-        env.set(:application, "my_app")
-        env.set(:stage, "staging")
-        env.set(:local_user, "(Git Web User) via ShipIt")
         subject.set_defaults
-        expect(env.fetch(:git_wrapper_path)).to eq("/tmp/git-ssh-my_app-staging-(Git Web User) via ShipIt.sh")
+        expect(env.fetch(:git_wrapper_path)).to match(%r{/tmp/git-ssh-\h{20}\.sh})
       end
 
       it "makes git_max_concurrent_connections" do
diff --git a/spec/support/test_app.rb b/spec/support/test_app.rb
@@ -185,8 +185,8 @@ def move_configuration_to_custom_location(location)
     FileUtils.mv(config_path, location)
   end
 
-  def git_wrapper_path
-    "/tmp/git-ssh-my_app_name-#{stage}-#{current_user}.sh"
+  def git_wrapper_path_glob
+    "/tmp/git-ssh-*.sh"
   end
 
   def with_clean_bundler_env(&block)
