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

.rubocop.yml

@@ -118,6 +118,9 @@ Gitlab/ModuleWithInstanceVariables:
     - spec/support/**/*.rb
     - features/steps/**/*.rb
 
+Gitlab/HTTParty:
+  Enabled: true
+
 GitlabSecurity/PublicSend:
   Enabled: true
   Exclude:

app/helpers/application_settings_helper.rb

@@ -245,7 +245,8 @@ def visible_attributes
       :usage_ping_enabled,
       :user_default_external,
       :user_oauth_applications,
-      :version_check_enabled
+      :version_check_enabled,
+      :allow_local_requests_from_hooks_and_services
     ]
   end
 end

app/models/application_setting.rb

@@ -330,7 +330,8 @@ def self.defaults
       usage_ping_enabled: Settings.gitlab['usage_ping_enabled'],
       gitaly_timeout_fast: 10,
       gitaly_timeout_medium: 30,
-      gitaly_timeout_default: 55
+      gitaly_timeout_default: 55,
+      allow_local_requests_from_hooks_and_services: false
     }
   end
 

app/models/project.rb

@@ -38,6 +38,9 @@ class Project < ActiveRecord::Base
     attachments: 2
   }.freeze
 
+  # Valids ports to import from
+  VALID_IMPORT_PORTS = [22, 80, 443].freeze
+
   cache_markdown_field :description, pipeline: :description
 
   delegate :feature_available?, :builds_enabled?, :wiki_enabled?,

app/models/project_services/assembla_service.rb

 class AssemblaService < Service
-  include HTTParty
-
   prop_accessor :token, :subdomain
   validates :token, presence: true, if: :activated?
 
@@ -31,6 +29,6 @@ def execute(data)
     return unless supported_events.include?(data[:object_kind])
 
     url = "https://atlas.assembla.com/spaces/#{subdomain}/github_tool?secret_key=#{token}"
-    AssemblaService.post(url, body: { payload: data }.to_json, headers: { 'Content-Type' => 'application/json' })
+    Gitlab::HTTP.post(url, body: { payload: data }.to_json, headers: { 'Content-Type' => 'application/json' })
   end
 end

app/models/project_services/bamboo_service.rb

@@ -117,14 +117,14 @@ def get_path(path)
     url = build_url(path)
 
     if username.blank? && password.blank?
-      HTTParty.get(url, verify: false)
+      Gitlab::HTTP.get(url, verify: false)
     else
       url << '&os_authType=basic'
-      HTTParty.get(url, verify: false,
-                        basic_auth: {
-                          username: username,
-                          password: password
-                        })
+      Gitlab::HTTP.get(url, verify: false,
+                            basic_auth: {
+                              username: username,
+                              password: password
+                            })
     end
   end
 end

app/models/project_services/buildkite_service.rb

@@ -71,7 +71,7 @@ def fields
   end
 
   def calculate_reactive_cache(sha, ref)
-    response = HTTParty.get(commit_status_path(sha), verify: false)
+    response = Gitlab::HTTP.get(commit_status_path(sha), verify: false)
 
     status =
       if response.code == 200 && response['status']

app/models/project_services/campfire_service.rb

 class CampfireService < Service
-  include HTTParty
-
   prop_accessor :token, :subdomain, :room
   validates :token, presence: true, if: :activated?
 
@@ -31,7 +29,6 @@ def self.supported_events
   def execute(data)
     return unless supported_events.include?(data[:object_kind])
 
-    self.class.base_uri base_uri
     message = build_message(data)
     speak(self.room, message, auth)
   end
@@ -69,14 +66,14 @@ def speak(room_name, message, auth)
         }
       }
     }
-    res = self.class.post(path, auth.merge(body))
+    res = Gitlab::HTTP.post(path, base_uri: base_uri, **auth.merge(body))
     res.code == 201 ? res : nil
   end
 
   # Returns a list of rooms, or [].
   # https://github.com/basecamp/campfire-api/blob/master/sections/rooms.md#get-rooms
   def rooms(auth)
-    res = self.class.get("/rooms.json", auth)
+    res = Gitlab::HTTP.get("/rooms.json", base_uri: base_uri, **auth)
     res.code == 200 ? res["rooms"] : []
   end
 

app/models/project_services/drone_ci_service.rb

@@ -49,7 +49,7 @@ def commit_status(sha, ref)
   end
 
   def calculate_reactive_cache(sha, ref)
-    response = HTTParty.get(commit_status_path(sha, ref), verify: enable_ssl_verification)
+    response = Gitlab::HTTP.get(commit_status_path(sha, ref), verify: enable_ssl_verification)
 
     status =
       if response.code == 200 && response['status']

app/models/project_services/external_wiki_service.rb

 class ExternalWikiService < Service
-  include HTTParty
-
   prop_accessor :external_wiki_url
 
   validates :external_wiki_url, presence: true, url: true, if: :activated?
@@ -24,7 +22,7 @@ def fields
   end
 
   def execute(_data)
-    @response = HTTParty.get(properties['external_wiki_url'], verify: true) rescue nil
+    @response = Gitlab::HTTP.get(properties['external_wiki_url'], verify: true) rescue nil
     if @response != 200
       nil
     end

app/models/project_services/issue_tracker_service.rb

@@ -77,13 +77,13 @@ def execute(data)
     result = false
 
     begin
-      response = HTTParty.head(self.project_url, verify: true)
+      response = Gitlab::HTTP.head(self.project_url, verify: true)
 
       if response
         message = "#{self.type} received response #{response.code} when attempting to connect to #{self.project_url}"
         result = true
       end
-    rescue HTTParty::Error, Timeout::Error, SocketError, Errno::ECONNRESET, Errno::ECONNREFUSED, OpenSSL::SSL::SSLError => error
+    rescue Gitlab::HTTP::Error, Timeout::Error, SocketError, Errno::ECONNRESET, Errno::ECONNREFUSED, OpenSSL::SSL::SSLError => error
       message = "#{self.type} had an error when trying to connect to #{self.project_url}: #{error.message}"
     end
     Rails.logger.info(message)

app/models/project_services/mock_ci_service.rb

@@ -52,7 +52,7 @@ def build_page(sha, ref)
   #
   #
   def commit_status(sha, ref)
-    response = HTTParty.get(commit_status_path(sha), verify: false)
+    response = Gitlab::HTTP.get(commit_status_path(sha), verify: false)
     read_commit_status(response)
   rescue Errno::ECONNREFUSED
     :error

app/models/project_services/packagist_service.rb

 class PackagistService < Service
-  include HTTParty
-
   prop_accessor :username, :token, :server
 
   validates :username, presence: true, if: :activated?

app/models/project_services/pivotaltracker_service.rb

 class PivotaltrackerService < Service
-  include HTTParty
-
   API_ENDPOINT = 'https://www.pivotaltracker.com/services/v5/source_commits'.freeze
 
   prop_accessor :token, :restrict_to_branch
@@ -52,7 +50,7 @@ def execute(data)
           'message' => commit[:message]
         }
       }
-      PivotaltrackerService.post(
+      Gitlab::HTTP.post(
         API_ENDPOINT,
         body: message.to_json,
         headers: {

app/models/project_services/pushover_service.rb

 class PushoverService < Service
-  include HTTParty
-  base_uri 'https://api.pushover.net/1'
+  BASE_URI = 'https://api.pushover.net/1'.freeze
 
   prop_accessor :api_key, :user_key, :device, :priority, :sound
   validates :api_key, :user_key, :priority, presence: true, if: :activated?
@@ -99,6 +98,6 @@ def execute(data)
       pushover_data[:sound] = sound
     end
 
-    PushoverService.post('/messages.json', body: pushover_data)
+    Gitlab::HTTP.post('/messages.json', base_uri: BASE_URI, body: pushover_data)
   end
 end

app/models/project_services/teamcity_service.rb

@@ -83,7 +83,7 @@ def execute(data)
 
     branch = Gitlab::Git.ref_name(data[:ref])
 
-    HTTParty.post(
+    Gitlab::HTTP.post(
       build_url('httpAuth/app/rest/buildQueue'),
       body: "<build branchName=\"#{branch}\">"\
             "<buildType id=\"#{build_type}\"/>"\
@@ -134,10 +134,10 @@ def build_url(path)
   end
 
   def get_path(path)
-    HTTParty.get(build_url(path), verify: false,
-                                  basic_auth: {
-                                    username: username,
-                                    password: password
-                                  })
+    Gitlab::HTTP.get(build_url(path), verify: false,
+                                      basic_auth: {
+                                        username: username,
+                                        password: password
+                                      })
   end
 end

app/services/projects/import_service.rb

@@ -28,7 +28,7 @@ def execute
 
     def add_repository_to_project
       if project.external_import? && !unknown_url?
-        raise Error, 'Blocked import URL.' if Gitlab::UrlBlocker.blocked_url?(project.import_url)
+        raise Error, 'Blocked import URL.' if Gitlab::UrlBlocker.blocked_url?(project.import_url, valid_ports: Project::VALID_IMPORT_PORTS)
       end
 
       # We should skip the repository for a GitHub import or GitLab project import,

app/services/submit_usage_ping_service.rb

@@ -14,16 +14,17 @@ class SubmitUsagePingService
   def execute
     return false unless Gitlab::CurrentSettings.usage_ping_enabled?
 
-    response = HTTParty.post(
+    response = Gitlab::HTTP.post(
       URL,
       body: Gitlab::UsageData.to_json(force_refresh: true),
+      allow_local_requests: true,
       headers: { 'Content-type' => 'application/json' }
     )
 
     store_metrics(response)
 
     true
-  rescue HTTParty::Error => e
+  rescue Gitlab::HTTP::Error => e
     Rails.logger.info "Unable to contact GitLab, Inc.: #{e}"
 
     false

app/services/web_hook_service.rb

@@ -3,23 +3,20 @@ class InternalErrorResponse
     attr_reader :body, :headers, :code
 
     def initialize
-      @headers = HTTParty::Response::Headers.new({})
+      @headers = Gitlab::HTTP::Response::Headers.new({})
       @body = ''
       @code = 'internal error'
     end
   end
 
-  include HTTParty
-
-  # HTTParty timeout
-  default_timeout Gitlab.config.gitlab.webhook_timeout
-
-  attr_accessor :hook, :data, :hook_name
+  attr_accessor :hook, :data, :hook_name, :request_options
 
   def initialize(hook, data, hook_name)
     @hook = hook
     @data = data
     @hook_name = hook_name.to_s
+    @request_options = { timeout: Gitlab.config.gitlab.webhook_timeout }
+    @request_options.merge!(allow_local_requests: true) if @hook.is_a?(SystemHook)
   end
 
   def execute
@@ -73,11 +70,12 @@ def parsed_url
   end
 
   def make_request(url, basic_auth = false)
-    self.class.post(url,
+    Gitlab::HTTP.post(url,
       body: data.to_json,
       headers: build_headers(hook_name),
       verify: hook.enable_ssl_verification,
-      basic_auth: basic_auth)
+      basic_auth: basic_auth,
+      **request_options)
   end
 
   def make_request_with_auth

app/validators/importable_url_validator.rb

@@ -4,7 +4,7 @@
 # protect against Server-side Request Forgery (SSRF).
 class ImportableUrlValidator < ActiveModel::EachValidator
   def validate_each(record, attribute, value)
-    if Gitlab::UrlBlocker.blocked_url?(value)
+    if Gitlab::UrlBlocker.blocked_url?(value, valid_ports: Project::VALID_IMPORT_PORTS)
       record.errors.add(attribute, "imports are not allowed from that URL")
     end
   end

app/views/admin/application_settings/_form.html.haml

@@ -860,5 +860,14 @@
       .col-sm-10
         = f.number_field :throttle_authenticated_web_period_in_seconds, class: 'form-control'
 
+  %fieldset
+    %legend Outbound requests
+    .form-group
+      .col-sm-offset-2.col-sm-10
+        .checkbox
+          = f.label :allow_local_requests_from_hooks_and_services do
+            = f.check_box :allow_local_requests_from_hooks_and_services
+            Allow requests to the local network from hooks and services
+
   .form-actions
     = f.submit 'Save', class: 'btn btn-save'

changelogs/unreleased/fj-15329-services-callbacks-ssrf.yml

+---
+title: Fixed some SSRF vulnerabilities in services, hooks and integrations
+merge_request: 2337
+author:
+type: security

db/migrate/20180223144945_add_allow_local_requests_from_hooks_and_services_to_application_settings.rb

+class AddAllowLocalRequestsFromHooksAndServicesToApplicationSettings < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default(:application_settings, :allow_local_requests_from_hooks_and_services,
+                            :boolean,
+                            default: false,
+                            allow_null: false)
+  end
+
+  def down
+    remove_column(:application_settings, :allow_local_requests_from_hooks_and_services)
+  end
+end

db/schema.rb

@@ -157,6 +157,7 @@
     t.boolean "authorized_keys_enabled", default: true, null: false
     t.string "auto_devops_domain"
     t.boolean "pages_domain_verification_enabled", default: true, null: false
+    t.boolean "allow_local_requests_from_hooks_and_services", default: false, null: false
   end
 
   create_table "audit_events", force: :cascade do |t|

lib/gitlab/http.rb

+# 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

lib/gitlab/proxy_http_connection_adapter.rb

+# 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

lib/gitlab/url_blocker.rb

@@ -3,11 +3,7 @@
 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 @@ def blocked_url?(url)
           # 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 @@ def blocked_url?(url)
 
       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 @@ def internal_shell?(uri)
           (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

lib/mattermost/session.rb

@@ -22,16 +22,14 @@ def message
   # 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 @@ def params
 
     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 @@ def delete(path, options = {})
 
     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 @@ def lease_try_obtain
 
     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

lib/microsoft_teams/notifier.rb

@@ -9,14 +9,15 @@ def ping(options = {})
       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
 

rubocop/cop/gitlab/httparty.rb

+require_relative '../../spec_helpers'
+
+module RuboCop
+  module Cop
+    module Gitlab
+      class HTTParty < RuboCop::Cop::Cop
+        include SpecHelpers
+
+        MSG_SEND = <<~EOL.freeze
+          Avoid calling `HTTParty` directly. Instead, use the Gitlab::HTTP
+          wrapper. To allow request to localhost or the private network set
+          the option :allow_local_requests in the request call.
+        EOL
+
+        MSG_INCLUDE = <<~EOL.freeze
+          Avoid including `HTTParty` directly. Instead, use the Gitlab::HTTP
+          wrapper. To allow request to localhost or the private network set
+          the option :allow_local_requests in the request call.
+        EOL
+
+        def_node_matcher :includes_httparty?, <<~PATTERN
+          (send nil? :include (const nil? :HTTParty))
+        PATTERN
+
+        def_node_matcher :httparty_node?, <<~PATTERN
+          (send (const nil? :HTTParty)...)
+        PATTERN
+
+        def on_send(node)
+          return if in_spec?(node)
+
+          add_offense(node, location: :expression, message: MSG_SEND) if httparty_node?(node)
+          add_offense(node, location: :expression, message: MSG_INCLUDE) if includes_httparty?(node)
+        end
+
+        def autocorrect(node)
+          if includes_httparty?(node)
+            autocorrect_includes_httparty(node)
+          else
+            autocorrect_httparty_node(node)
+          end
+        end
+
+        def autocorrect_includes_httparty(node)
+          lambda do |corrector|
+            corrector.remove(node.source_range)
+          end
+        end
+
+        def autocorrect_httparty_node(node)
+          _, method_name, *arg_nodes = *node
+
+          replacement = "Gitlab::HTTP.#{method_name}(#{arg_nodes.map(&:source).join(', ')})"
+
+          lambda do |corrector|
+            corrector.replace(node.source_range, replacement)
+          end
+        end