Merge branch 'fj-15329-services-callbacks-ssrf' into 'security-10-6'

Server Side Request Forgery in Services and Web Hooks

See merge request gitlab/gitlabhq!2337

5 files changed, 75 insertions, 20 deletions

diff --git a/lib/gitlab/http.rb b/lib/gitlab/http.rb
new file mode 100644
index 00000000000..96558872a37
--- /dev/null
+++ b/lib/gitlab/http.rb
@@ -0,0 +1,11 @@
+# This class is used as a proxy for all outbounding http connection
+# coming from callbacks, services and hooks. The direct use of the HTTParty
+# is discouraged because it can lead to several security problems, like SSRF
+# calling internal IP or services.
+module Gitlab
+  class HTTP
+    include HTTParty # rubocop:disable Gitlab/HTTParty
+
+    connection_adapter ProxyHTTPConnectionAdapter
+  end
+end
diff --git a/lib/gitlab/proxy_http_connection_adapter.rb b/lib/gitlab/proxy_http_connection_adapter.rb
new file mode 100644
index 00000000000..c70d6f4cd84
--- /dev/null
+++ b/lib/gitlab/proxy_http_connection_adapter.rb
@@ -0,0 +1,34 @@
+# This class is part of the Gitlab::HTTP wrapper. Depending on the value
+# of the global setting allow_local_requests_from_hooks_and_services this adapter
+# will allow/block connection to internal IPs and/or urls.
+#
+# This functionality can be overriden by providing the setting the option
+# allow_local_requests = true in the request. For example:
+# Gitlab::HTTP.get('http://www.gitlab.com', allow_local_requests: true)
+#
+# This option will take precedence over the global setting.
+module Gitlab
+  class ProxyHTTPConnectionAdapter < HTTParty::ConnectionAdapter
+    def connection
+      if !allow_local_requests? && blocked_url?
+        raise URI::InvalidURIError
+      end
+
+      super
+    end
+
+    private
+
+    def blocked_url?
+      Gitlab::UrlBlocker.blocked_url?(uri, allow_private_networks: false)
+    end
+
+    def allow_local_requests?
+      options.fetch(:allow_local_requests, allow_settings_local_requests?)
+    end
+
+    def allow_settings_local_requests?
+      Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services?
+    end
+  end
+end
diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb
index 13150ddab67..0f9f939e204 100644
--- a/lib/gitlab/url_blocker.rb
+++ b/lib/gitlab/url_blocker.rb
@@ -3,11 +3,7 @@ require 'resolv'
 module Gitlab
   class UrlBlocker
     class << self
-      # Used to specify what hosts and port numbers should be prohibited for project
-      # imports.
-      VALID_PORTS = [22, 80, 443].freeze
-
-      def blocked_url?(url)
+      def blocked_url?(url, allow_private_networks: true, valid_ports: [])
         return false if url.nil?
 
         blocked_ips = ["127.0.0.1", "::1", "0.0.0.0"]
@@ -18,12 +14,15 @@ module Gitlab
           # Allow imports from the GitLab instance itself but only from the configured ports
           return false if internal?(uri)
 
-          return true if blocked_port?(uri.port)
+          return true if blocked_port?(uri.port, valid_ports)
           return true if blocked_user_or_hostname?(uri.user)
           return true if blocked_user_or_hostname?(uri.hostname)
 
-          server_ips = Addrinfo.getaddrinfo(uri.hostname, 80, nil, :STREAM).map(&:ip_address)
+          addrs_info = Addrinfo.getaddrinfo(uri.hostname, 80, nil, :STREAM)
+          server_ips = addrs_info.map(&:ip_address)
+
           return true if (blocked_ips & server_ips).any?
+          return true if !allow_private_networks && private_network?(addrs_info)
         rescue Addressable::URI::InvalidURIError
           return true
         rescue SocketError
@@ -35,10 +34,10 @@ module Gitlab
 
       private
 
-      def blocked_port?(port)
-        return false if port.blank?
+      def blocked_port?(port, valid_ports)
+        return false if port.blank? || valid_ports.blank?
 
-        port < 1024 && !VALID_PORTS.include?(port)
+        port < 1024 && !valid_ports.include?(port)
       end
 
       def blocked_user_or_hostname?(value)
@@ -61,6 +60,10 @@ module Gitlab
           (uri.port.blank? || uri.port == config.gitlab_shell.ssh_port)
       end
 
+      def private_network?(addrs_info)
+        addrs_info.any? { |addr| addr.ipv4_private? || addr.ipv6_sitelocal? }
+      end
+
       def config
         Gitlab.config
       end
diff --git a/lib/mattermost/session.rb b/lib/mattermost/session.rb
index 65ccdb3c347..a514ec15c2a 100644
--- a/lib/mattermost/session.rb
+++ b/lib/mattermost/session.rb
@@ -22,16 +22,14 @@ module Mattermost
   # going.
   class Session
     include Doorkeeper::Helpers::Controller
-    include HTTParty
 
     LEASE_TIMEOUT = 60
 
-    base_uri Settings.mattermost.host
-
-    attr_accessor :current_resource_owner, :token
+    attr_accessor :current_resource_owner, :token, :base_uri
 
     def initialize(current_user)
       @current_resource_owner = current_user
+      @base_uri = Settings.mattermost.host
     end
 
     def with_session
@@ -73,13 +71,13 @@ module Mattermost
 
     def get(path, options = {})
       handle_exceptions do
-        self.class.get(path, options.merge(headers: @headers))
+        Gitlab::HTTP.get(path, build_options(options))
       end
     end
 
     def post(path, options = {})
       handle_exceptions do
-        self.class.post(path, options.merge(headers: @headers))
+        Gitlab::HTTP.post(path, build_options(options))
       end
     end
 
@@ -91,6 +89,14 @@ module Mattermost
 
     private
 
+    def build_options(options)
+      options.tap do |hash|
+        hash[:headers] = @headers
+        hash[:allow_local_requests] = true
+        hash[:base_uri] = base_uri if base_uri.presence
+      end
+    end
+
     def create
       raise Mattermost::NoSessionError unless oauth_uri
       raise Mattermost::NoSessionError unless token_uri
@@ -165,14 +171,14 @@ module Mattermost
 
     def handle_exceptions
       yield
-    rescue HTTParty::Error => e
+    rescue Gitlab::HTTP::Error => e
       raise Mattermost::ConnectionError.new(e.message)
     rescue Errno::ECONNREFUSED => e
       raise Mattermost::ConnectionError.new(e.message)
     end
 
     def parse_cookie(response)
-      cookie_hash = CookieHash.new
+      cookie_hash = Gitlab::HTTP::CookieHash.new
       response.get_fields('Set-Cookie').each { |c| cookie_hash.add_cookies(c) }
       cookie_hash
     end
diff --git a/lib/microsoft_teams/notifier.rb b/lib/microsoft_teams/notifier.rb
index 3bef68a1bcb..c08d3e933a8 100644
--- a/lib/microsoft_teams/notifier.rb
+++ b/lib/microsoft_teams/notifier.rb
@@ -9,14 +9,15 @@ module MicrosoftTeams
       result = false
 
       begin
-        response = HTTParty.post(
+        response = Gitlab::HTTP.post(
           @webhook.to_str,
           headers: @header,
+          allow_local_requests: true,
           body: body(options)
         )
 
         result = true if response
-      rescue HTTParty::Error, StandardError => error
+      rescue Gitlab::HTTP::Error, StandardError => error
         Rails.logger.info("#{self.class.name}: Error while connecting to #{@webhook}: #{error.message}")
       end