How it was found

I was digging through the gitlab-foss repository and noticed an interested pattern that seems to be adopted in a few places: the use of Forwardable with meta-programming over delegators, explicit attr_reader methods or method_missing. Heads up: the arbitrary file read vulnerability I demonstrate in this report isn't currently exploitable. I was somewhat hesitant to submit this, but I think it'd be a good refactor nonetheless. Before diving into the vulnerability, I'd like to start by describing Ruby's Forwardable module behavior in combination with def_delegators.

Before Ruby 2.5.1, delegators could be implemented using the delegate or method_missing methods. It would look something like this:

class HelloWorld
  def initialize(attributes)
    @options = OpenStruct.new(attributes)
  end

  def say_it
    "Hello world"
  end

  def method_missing(method, *args)
    @options.send(method, *args)
  end
end

When a method would be called on a HelloWorld instance that wouldn't exist, it would pass it along to the @options instance variable.

HelloWorld.new({}).say_it
# => "Hello world"

HelloWorld.new(hello: "world").hello
# => "world"

HelloWorld.new(say_it: "Not hello world").say_it
# => "Hello world"

Because the say_it method is already defined on the class, its behavior won't be overridden when passing say_it to the initializer.

This class can be refactored to use the Forwardable method and def_delegators:

class HelloWorld
  extend Forwardable

  def initialize(attributes)
    @options = OpenStruct.new(attributes)

    self.class.instance_eval do
      def_delegators :@options, *attributes.keys
    end
  end

  def say_it
    "Hello world"
  end
end

At first glance, this seems like it has the same behavior as the first code example; but there's one crucial difference: because the delegators are meta-programmed after the class was loaded, it can overwrite existing methods:

HelloWorld.new({}).say_it
# => "Hello world"

HelloWorld.new(hello: "world").hello
# => "world"

HelloWorld.new(say_it: "Not hello world").say_it
# => "Not hello world"
#        ^------------------ The method is overwritten

As can be seen in the above example, the say_it method is overwritten when passing it to the initializer.

Going back to GitLab's main Ruby repository, there are a number of places where the Forwardable module is used. One place in particular stands out: Gitlab::ImportExport::AfterExportStrategies::BaseAfterExportStrategy. This class is a base class used for:

• Gitlab::ImportExport::AfterExportStrategies::MoveFileStrategy
• Gitlab::ImportExport::AfterExportStrategies::DownloadNotificationStrategy
• Gitlab::ImportExport::AfterExportStrategies::WebUploadStrategy

# frozen_string_literal: true

module Gitlab
  module ImportExport
    module AfterExportStrategies
      class BaseAfterExportStrategy
        extend Gitlab::ImportExport::CommandLineUtil
        include ActiveModel::Validations
        extend Forwardable

        # ...

        def initialize(attributes = {})
          @options = OpenStruct.new(attributes)

          self.class.instance_eval do
            def_delegators :@options, *attributes.keys
          end
        end

        # ...

        def archive_path
          project.import_export_shared.archive_path
        end

        # ...
      end
    end
  end
end

The MoveFileStrategy and WebUploadStrategy classes overwrite the initializer method or declare its arguments, so these don't meta-program the arguments on the class or limit what can be delegated. My worry, and the potential security vulnerabilities, is that if a new strategy would be declared that inherits from the BaseAfterExportStrategy without overwriting the initializer, it may give attackers the ability to change the behavior of existing methods.

As an example, let's say the DownloadNotificationStrategy class would be initialized with a user-inputted hash: if the user would specify the archive_path argument, it could overwrite the method and point it to a different archive on the local system. Same for the WebUploadStrategy: if the initialize method would be removed today, specs would still pass, but suddenly a security vulnerability would be present if the user could give it arbitrary arguments (same thing, overwrite the archive_path).

I know that this isn't the security vulnerabilities you typically receive from me, but after reading the code, I felt it was the right thing to do to warn you about the potential security vulnerabilities that could be introduced in the future.

This is based on the master branch as of May 13, 2020.

Recommendation

Given that there are only three classes inheriting from the base class, I'd rewrite the code like this to avoid trouble in the future (untested). Direct download: F828467.

diff --git a/lib/gitlab/import_export/after_export_strategies/base_after_export_strategy.rb b/lib/gitlab/import_export/after_export_strategies/base_after_export_strategy.rb
index b30258123d4..b52073978ee 100644
--- a/lib/gitlab/import_export/after_export_strategies/base_after_export_strategy.rb
+++ b/lib/gitlab/import_export/after_export_strategies/base_after_export_strategy.rb
@@ -6,7 +6,6 @@ module Gitlab
       class BaseAfterExportStrategy
         extend Gitlab::ImportExport::CommandLineUtil
         include ActiveModel::Validations
-        extend Forwardable
 
         StrategyError = Class.new(StandardError)
 
@@ -16,14 +15,6 @@ module Gitlab
 
         public
 
-        def initialize(attributes = {})
-          @options = OpenStruct.new(attributes)
-
-          self.class.instance_eval do
-            def_delegators :@options, *attributes.keys
-          end
-        end
-
         def execute(current_user, project)
           @project = project
 
@@ -67,10 +58,6 @@ module Gitlab
           project.import_export_shared.lock_files_path
         end
 
-        def archive_path
-          project.import_export_shared.archive_path
-        end
-
         def locks_present?
           project.import_export_shared.locks_present?
         end
diff --git a/lib/gitlab/import_export/after_export_strategies/download_notification_strategy.rb b/lib/gitlab/import_export/after_export_strategies/download_notification_strategy.rb
index 39a6090ad87..da0a593691c 100644
--- a/lib/gitlab/import_export/after_export_strategies/download_notification_strategy.rb
+++ b/lib/gitlab/import_export/after_export_strategies/download_notification_strategy.rb
@@ -10,6 +10,10 @@ module Gitlab
           false
         end
 
+        def archive_path
+          project.import_export_shared.archive_path
+        end
+
         private
 
         def strategy_execute
diff --git a/lib/gitlab/import_export/after_export_strategies/move_file_strategy.rb b/lib/gitlab/import_export/after_export_strategies/move_file_strategy.rb
index 2e3136936f8..8a58f0911e3 100644
--- a/lib/gitlab/import_export/after_export_strategies/move_file_strategy.rb
+++ b/lib/gitlab/import_export/after_export_strategies/move_file_strategy.rb
@@ -4,6 +4,8 @@ module Gitlab
   module ImportExport
     module AfterExportStrategies
       class MoveFileStrategy < BaseAfterExportStrategy
+        attr_reader :archive_path
+
         def initialize(archive_path:)
           @archive_path = archive_path
         end
diff --git a/lib/gitlab/import_export/after_export_strategies/web_upload_strategy.rb b/lib/gitlab/import_export/after_export_strategies/web_upload_strategy.rb
index e2dba831661..80b12a76d26 100644
--- a/lib/gitlab/import_export/after_export_strategies/web_upload_strategy.rb
+++ b/lib/gitlab/import_export/after_export_strategies/web_upload_strategy.rb
@@ -16,8 +16,11 @@ module Gitlab
           end
         end
 
+        attr_reader :url, :http_method
+
         def initialize(url:, http_method: PUT_METHOD)
-          super
+          @url = url
+          @http_method = http_method
         end
 
         protected
@@ -32,6 +35,10 @@ module Gitlab
           end
         end
 
+        def archive_path
+          project.import_export_shared.archive_path
+        end
+
         private
 
         def send_file

Impact

Allowing an attacker to pass a hash to the initializer of a class inheriting from BaseAfterExportStrategy may lead to arbitrary file read, or potentially even to remote code execution.