Include contents of external scripts in hook signatures

Commit 11c8ad6 introduced a security vulnerability where defining a hook
that simply called an external script would not sign the external
script.

Fix this by enforcing that these kinds of hooks must have
`required_executable`/`command` options that are paths to
executables stored in source control (relative to the repository root),
and furthermore that the contents of those scripts actually are included
in the signature.

Author: sds

lib/overcommit.rb

@@ -11,7 +11,6 @@
 require 'overcommit/git_repo'
 require 'overcommit/hook_signer'
 require 'overcommit/hook_loader/base'
-require 'overcommit/hook_loader/ad_hoc_hook_loader'
 require 'overcommit/hook_loader/built_in_hook_loader'
 require 'overcommit/hook_loader/plugin_hook_loader'
 require 'overcommit/interrupt_handler'

lib/overcommit/exceptions.rb

@@ -30,6 +30,9 @@ class InvalidCommandArgs < StandardError; end
   # Raised when an installation target is not a valid git repository.
   class InvalidGitRepo < StandardError; end
 
+  # Raised when a hook was defined incorrectly.
+  class InvalidHookDefinition < StandardError; end
+
   # Raised when one or more hook plugin signatures have changed.
   class InvalidHookSignature < StandardError; end
 

lib/overcommit/hook_loader/ad_hoc_hook_loader.rb

@@ -7,12 +7,16 @@ class PluginHookLoader < Base
     def load_hooks
       check_for_modified_plugins if @config.verify_plugin_signatures?
 
-      plugin_paths.map do |plugin_path|
+      hooks = plugin_paths.map do |plugin_path|
         require plugin_path
 
         hook_name = Overcommit::Utils.camel_case(File.basename(plugin_path, '.rb'))
         create_hook(hook_name)
       end
+
+      hooks + ad_hoc_hook_names.map do |hook_name|
+        create_ad_hoc_hook(hook_name)
+      end
     end
 
     def update_signatures
@@ -31,9 +35,19 @@ def plugin_paths
       Dir[File.join(directory, '*.rb')].sort
     end
 
+    def plugin_hook_names
+      plugin_paths.map do |path|
+        Overcommit::Utils.camel_case(File.basename(path, '.rb'))
+      end
+    end
+
+    def ad_hoc_hook_names
+      @config.enabled_ad_hoc_hooks(@context)
+    end
+
     def modified_plugins
-      plugin_paths.
-        map { |path| Overcommit::HookSigner.new(path, @config, @context) }.
+      (plugin_hook_names + ad_hoc_hook_names).
+        map { |hook_name| Overcommit::HookSigner.new(hook_name, @config, @context) }.
         select(&:signature_changed?)
     end
 
@@ -57,5 +71,30 @@ def check_for_modified_plugins
 
       raise Overcommit::Exceptions::InvalidHookSignature
     end
+
+    def create_ad_hoc_hook(hook_name)
+      hook_module = Overcommit::Hook.const_get(@context.hook_class_name)
+      hook_base = hook_module.const_get('Base')
+
+      # Implement a simple class that executes the command and returns pass/fail
+      # based on the exit status
+      hook_class = Class.new(hook_base) do
+        def run # rubocop:disable Lint/NestedMethodDefinition
+          result = @context.execute_hook(command)
+
+          if result.success?
+            :pass
+          else
+            [:fail, result.stdout + result.stderr]
+          end
+        end
+      end
+
+      hook_module.const_set(hook_name, hook_class).new(@config, @context)
+    rescue LoadError, NameError => error
+      raise Overcommit::Exceptions::HookLoadError,
+            "Unable to load hook '#{hook_name}': #{error}",
+            error.backtrace
+    end
   end
 end

lib/overcommit/hook_loader/plugin_hook_loader.rb

@@ -141,7 +141,6 @@ def load_hooks
       require "overcommit/hook/#{@context.hook_type_name}/base"
 
       @hooks += HookLoader::BuiltInHookLoader.new(@config, @context, @log).load_hooks
-      @hooks += HookLoader::AdHocHookLoader.new(@config, @context, @log).load_hooks
 
       # Load plugin hooks after so they can subclass existing hooks
       @hooks += HookLoader::PluginHookLoader.new(@config, @context, @log).load_hooks

lib/overcommit/hook_runner.rb

@@ -1,21 +1,51 @@
 module Overcommit
   # Calculates, stores, and retrieves stored signatures of hook plugins.
   class HookSigner
-    attr_reader :hook_path, :hook_name
+    attr_reader :hook_name
 
     # We don't want to include the skip setting as it is set by Overcommit
     # itself
     IGNORED_CONFIG_KEYS = %w[skip]
 
-    # @param hook_path [String] path to the actual hook definition
+    # @param hook_name [String] name of the hook
     # @param config [Overcommit::Configuration]
     # @param context [Overcommit::HookContext]
-    def initialize(hook_path, config, context)
-      @hook_path = hook_path
+    def initialize(hook_name, config, context)
+      @hook_name = hook_name
       @config = config
       @context = context
+    end
+
+    # Returns the path of the file that should be incorporated into this hooks
+    # signature.
+    #
+    # @return [String]
+    def hook_path
+      @hook_path ||= begin
+        plugin_path = File.join(@config.plugin_directory,
+                                @context.hook_type_name,
+                                "#{Overcommit::Utils.snake_case(@hook_name)}.rb")
+
+        if File.exist?(plugin_path)
+          plugin_path
+        else
+          # Otherwise this is an ad hoc hook using an existing hook script
+          hook_config = @config.for_hook(@hook_name, @context.hook_class_name)
 
-      @hook_name = Overcommit::Utils.camel_case(File.basename(@hook_path, '.rb'))
+          command = Array(hook_config['command'] ||
+                          hook_config['required_executable'])
+
+          unless !@config.verify_plugin_signatures? ||
+                 command.first.start_with?(".#{File::SEPARATOR}")
+            raise Overcommit::Exceptions::InvalidHookDefinition,
+                  'Hook must specify a `required_executable` or `command` that ' \
+                  'is under source control (i.e. is a path relative to the root ' \
+                  'of the repository) so that it can be signed'
+          end
+
+          File.join(Overcommit::Utils.repo_root, command.first)
+        end
+      end
     end
 
     # Return whether the signature for this hook has changed since it was last
@@ -54,7 +84,7 @@ def signature
     end
 
     def hook_contents
-      File.open(@hook_path, 'r').read
+      File.read(hook_path)
     end
 
     def stored_signature

lib/overcommit/hook_signer.rb

@@ -76,7 +76,8 @@ rescue Overcommit::Exceptions::HookContextLoadError => error
   puts error
   puts 'Are you running an old version of Overcommit?'
   exit 69 # EX_UNAVAILABLE
-rescue Overcommit::Exceptions::HookLoadError => error
+rescue Overcommit::Exceptions::HookLoadError,
+       Overcommit::Exceptions::InvalidHookDefinition => error
   puts error.message
   puts error.backtrace
   exit 78 # EX_CONFIG