Merge branch 'latest-security-to-master-ee-21-03-18' into 'master'

Introduce latest security changes to `master` EE

See merge request gitlab-org/gitlab-ee!5062

.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

@@ -246,7 +246,8 @@ module ApplicationSettingsHelper
       :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

@@ -331,7 +331,8 @@ class ApplicationSetting < ActiveRecord::Base
       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

@@ -41,6 +41,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 @@ class AssemblaService < Service
     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 @@ class BambooService < CiService
     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 @@ class BuildkiteService < CiService
   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 @@ class CampfireService < Service
   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 @@ class CampfireService < Service
         }
       }
     }
-    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 @@ class DroneCiService < CiService
   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 @@ class ExternalWikiService < Service
   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

@@ -81,13 +81,13 @@ class IssueTrackerService < Service
     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 @@ class MockCiService < CiService
   #
   #
   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 @@ class PivotaltrackerService < Service
           '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 @@ class PushoverService < Service
       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 @@ class TeamcityService < CiService
 
     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 @@ class TeamcityService < CiService
   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 @@ module Projects
 
     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 WebHookService
     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 @@ class WebHookService
   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

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

@@ -185,6 +185,7 @@ ActiveRecord::Schema.define(version: 20180314174825) do
     t.string "external_authorization_service_default_label"
     t.boolean "pages_domain_verification_enabled", default: true, null: false
     t.float "external_authorization_service_timeout", default: 0.5, null: false
+    t.boolean "allow_local_requests_from_hooks_and_services", default: false, null: false
   end
 
   create_table "approvals", force: :cascade do |t|

ee/app/controllers/oauth/jira/authorizations_controller.rb

@@ -31,7 +31,7 @@ class Oauth::Jira::AuthorizationsController < ApplicationController
                     .slice(:code, :client_id, :client_secret)
                     .merge(grant_type: 'authorization_code', redirect_uri: oauth_jira_callback_url)
 
-    auth_response = HTTParty.post(oauth_token_url, body: auth_params)
+    auth_response = Gitlab::HTTP.post(oauth_token_url, body: auth_params, allow_local_requests: true)
     token_type, scope, token = auth_response['token_type'], auth_response['scope'], auth_response['access_token']
 
     render text: "access_token=#{token}&scope=#{scope}&token_type=#{token_type}"

ee/app/models/project_services/jenkins_deprecated_service.rb

@@ -89,14 +89,14 @@ class JenkinsDeprecatedService < CiService
     parsed_url = URI.parse(build_page(sha, ref))
 
     if parsed_url.userinfo.blank?
-      response = HTTParty.get(build_page(sha, ref), verify: false)
+      response = Gitlab::HTTP.get(build_page(sha, ref), verify: false, allow_local_requests: true)
     else
       get_url = build_page(sha, ref).gsub("#{parsed_url.userinfo}@", "")
       auth = {
         username: CGI.unescape(parsed_url.user),
         password: CGI.unescape(parsed_url.password)
       }
-      response = HTTParty.get(get_url, verify: false, basic_auth: auth)
+      response = Gitlab::HTTP.get(get_url, verify: false, basic_auth: auth, allow_local_requests: true)
     end
 
     if response.code == 200

ee/app/models/project_services/jenkins_service.rb

 class JenkinsService < CiService
-  include HTTParty
   prop_accessor :jenkins_url, :project_name, :username, :password
 
   before_update :reset_password

ee/app/services/fetch_subscription_plans_service.rb

@@ -12,7 +12,10 @@ class FetchSubscriptionPlansService
   private
 
   def send_request
-    response = HTTParty.get(URL, query: { plan: @plan }, headers: { 'Accept' => 'application/json' })
+    response = Gitlab::HTTP.get(URL,
+                                allow_local_requests: true,
+                                query: { plan: @plan },
+                                headers: { 'Accept' => 'application/json' })
 
     JSON.parse(response.body).map { |plan| Hashie::Mash.new(plan) }
   rescue => e

ee/app/services/geo/base_notify.rb

 module Geo
   class BaseNotify
-    include HTTParty
-
-    # HTTParty timeout
-    default_timeout Gitlab.config.gitlab.webhook_timeout
-
     def notify(notify_url, content)
-      response = self.class.post(notify_url,
-                                 body: content,
-                                 headers: {
-                                   'Content-Type' => 'application/json',
-                                   'PRIVATE-TOKEN' => private_token
-                                 })
+      response = Gitlab::HTTP.post(notify_url,
+                                   timeout: Gitlab.config.gitlab.webhook_timeout,
+                                   body: content,
+                                   allow_local_requests: true,
+                                   headers: {
+                                     'Content-Type' => 'application/json',
+                                     'PRIVATE-TOKEN' => private_token
+                                   })
 
       [(response.code >= 200 && response.code < 300), ActionView::Base.full_sanitizer.sanitize(response.to_s)]
-    rescue HTTParty::Error, Errno::ECONNREFUSED => e
+    rescue Gitlab::HTTP::Error, Errno::ECONNREFUSED => e
       [false, ActionView::Base.full_sanitizer.sanitize(e.message)]
     end
 

ee/app/services/geo/node_status_fetch_service.rb

 module Geo
   class NodeStatusFetchService
-    include HTTParty
-
     def call(geo_node)
       return GeoNodeStatus.current_node_status if geo_node.current?
 
@@ -9,7 +7,7 @@ module Geo
       data = data.merge(success: false, health_status: 'Offline')
 
       begin
-        response = self.class.get(geo_node.status_url, headers: headers, timeout: timeout)
+        response = Gitlab::HTTP.get(geo_node.status_url, allow_local_requests: true, headers: headers, timeout: timeout)
         data[:success] = response.success?
 
         if response.success?
@@ -33,7 +31,7 @@ module Geo
       rescue OpenSSL::Cipher::CipherError
         data[:health] = 'Error decrypting the Geo secret from the database. Check that the primary uses the correct db_key_base.'
         data[:health_status] = 'Unhealthy'
-      rescue HTTParty::Error, Timeout::Error, SocketError, SystemCallError, OpenSSL::SSL::SSLError => e
+      rescue Gitlab::HTTP::Error, Timeout::Error, SocketError, SystemCallError, OpenSSL::SSL::SSLError => e
         data[:health] = e.message
       end
 

ee/app/services/projects/slack_application_install_service.rb

@@ -52,7 +52,7 @@ module Projects
     end
 
     def exchange_slack_token
-      HTTParty.get(SLACK_EXCHANGE_TOKEN_URL, query: {
+      Gitlab::HTTP.get(SLACK_EXCHANGE_TOKEN_URL, query: {
         client_id: Gitlab::CurrentSettings.slack_app_id,
         client_secret: Gitlab::CurrentSettings.slack_app_secret,
         redirect_uri: slack_auth_project_settings_slack_url(project),

ee/lib/gitlab/chat/responder/slack.rb

@@ -16,7 +16,7 @@ module Gitlab
         #
         # output - The output to send back to Slack, as a Hash.
         def send_response(output)
-          HTTParty.post(
+          Gitlab::HTTP.post(
             pipeline.chat_data.response_url,
             {
               headers: { Accept: 'application/json' },

ee/lib/gitlab/ci/external/file/remote.rb

@@ -11,8 +11,8 @@ module Gitlab
 
             @content = strong_memoize(:content) do
               begin
-                HTTParty.get(location)
-              rescue HTTParty::Error, Timeout::Error, SocketError
+                Gitlab::HTTP.get(location, allow_local_requests: true)
+              rescue Gitlab::HTTP::Error, Timeout::Error, SocketError
                 nil
               end
             end

ee/lib/gitlab/geo/transfer.rb

@@ -49,7 +49,7 @@ module Gitlab
         true
       end
 
-      # Use HTTParty for now but switch to curb if performance becomes
+      # Use Gitlab::HTTP for now but switch to curb if performance becomes
       # an issue
       def download_file(url, req_headers)
         file_size = -1
@@ -58,7 +58,7 @@ module Gitlab
         return unless temp_file
 
         begin
-          response = HTTParty.get(url, headers: req_headers, stream_body: true) do |fragment|
+          response = Gitlab::HTTP.get(url, allow_local_requests: true, headers: req_headers, stream_body: true) do |fragment|
             temp_file.write(fragment)
           end
 
@@ -78,7 +78,7 @@ module Gitlab
 
           file_size = File.stat(filename).size
           log_info("Successful downloaded", filename: filename, file_size_bytes: file_size)
-        rescue StandardError, HTTParty::Error => e
+        rescue StandardError, Gitlab::HTTP::Error => e
           log_error("Error downloading file", error: e, filename: filename, url: url)
         ensure
           temp_file.close

ee/spec/controllers/oauth/jira/authorizations_controller_spec.rb

@@ -37,7 +37,7 @@ describe Oauth::Jira::AuthorizationsController do
                                'grant_type' => 'authorization_code',
                                'redirect_uri' => 'http://test.host/-/jira/login/oauth/callback' }
 
-      expect(HTTParty).to receive(:post).with(oauth_token_url, body: expected_auth_params) do
+      expect(Gitlab::HTTP).to receive(:post).with(oauth_token_url, allow_local_requests: true, body: expected_auth_params) do
         { 'access_token' => 'fake-123', 'scope' => 'foo', 'token_type' => 'bar' }
       end
 

ee/spec/features/billings/billing_plans_spec.rb

@@ -93,7 +93,7 @@ describe 'Billing plan pages', :feature do
   end
 
   before do
-    expect(HTTParty).to receive(:get).and_return(double(body: plans_data.to_json))
+    expect(Gitlab::HTTP).to receive(:get).and_return(double(body: plans_data.to_json))
     stub_application_setting(check_namespace_plan: true)
     allow(Gitlab).to receive(:com?) { true }
     gitlab_sign_in(user)

ee/spec/lib/gitlab/chat/responder/slack_spec.rb

@@ -19,7 +19,7 @@ describe Gitlab::Chat::Responder::Slack do
 
   describe '#send_response' do
     it 'sends a response back to Slack' do
-      expect(HTTParty).to receive(:post).with(
+      expect(Gitlab::HTTP).to receive(:post).with(
         'http://example.com',
         { headers: { Accept: 'application/json' }, body: 'hello'.to_json }
       )

ee/spec/lib/gitlab/ci/external/file/remote_spec.rb

@@ -35,7 +35,7 @@ describe Gitlab::Ci::External::File::Remote do
 
     context 'with a timeout' do
       before do
-        allow(HTTParty).to receive(:get).and_raise(Timeout::Error)
+        allow(Gitlab::HTTP).to receive(:get).and_raise(Timeout::Error)
       end
 
       it 'should be falsy' do
@@ -65,7 +65,7 @@ describe Gitlab::Ci::External::File::Remote do
 
     context 'with a timeout' do
       before do
-        allow(HTTParty).to receive(:get).and_raise(Timeout::Error)
+        allow(Gitlab::HTTP).to receive(:get).and_raise(Timeout::Error)
       end
 
       it 'should be falsy' do

ee/spec/lib/gitlab/geo/transfer_spec.rb

@@ -32,7 +32,7 @@ describe Gitlab::Geo::Transfer do
     it 'when the HTTP response is successful' do
       expect(FileUtils).to receive(:mv).with(anything, lfs_object.file.path).and_call_original
       response = double(success?: true)
-      expect(HTTParty).to receive(:get).and_yield(content.to_s).and_return(response)
+      expect(Gitlab::HTTP).to receive(:get).and_yield(content.to_s).and_return(response)
 
       expect(subject.download_from_primary).to eq(size)
       stat = File.stat(lfs_object.file.path)
@@ -44,7 +44,7 @@ describe Gitlab::Geo::Transfer do
     it 'when the HTTP response is unsuccessful' do
       expect(FileUtils).not_to receive(:mv).with(anything, lfs_object.file.path).and_call_original
       response = double(success?: false, code: 404, msg: 'No such file')
-      expect(HTTParty).to receive(:get).and_return(response)
+      expect(Gitlab::HTTP).to receive(:get).and_return(response)
 
       expect(subject.download_from_primary).to eq(-1)
     end

ee/spec/services/fetch_subscription_plans_service_spec.rb

@@ -9,8 +9,8 @@ describe FetchSubscriptionPlansService do
       it 'returns parsed JSON' do
         json_mock = double(body: [{ 'foo' => 'bar' }].to_json)
 
-        expect(HTTParty).to receive(:get)
-                              .with(endpoint_url, query: { plan: 'bronze' }, headers: { 'Accept' => 'application/json' })
+        expect(Gitlab::HTTP).to receive(:get)
+                              .with(endpoint_url, allow_local_requests: true, query: { plan: 'bronze' }, headers: { 'Accept' => 'application/json' })
                               .and_return(json_mock)
 
         is_expected.to eq([Hashie::Mash.new('foo' => 'bar')])
@@ -19,7 +19,7 @@ describe FetchSubscriptionPlansService do
 
     context 'when failing to fetch plans data' do
       before do
-        expect(HTTParty).to receive(:get).and_raise(HTTParty::Error.new('Error message'))
+        expect(Gitlab::HTTP).to receive(:get).and_raise(Gitlab::HTTP::Error.new('Error message'))
       end
 
       it 'logs failure' do

ee/spec/services/geo/metrics_update_service_spec.rb

@@ -72,7 +72,7 @@ describe Geo::MetricsUpdateService, :geo do
   describe '#execute' do
     before do
       request = double(success?: true, parsed_response: data.stringify_keys, code: 200)
-      allow(Geo::NodeStatusFetchService).to receive(:get).and_return(request)
+      allow(Gitlab::HTTP).to receive(:get).and_return(request)
     end
 
     context 'when current node is nil' do
@@ -81,7 +81,7 @@ describe Geo::MetricsUpdateService, :geo do
       end
 
       it 'skips fetching the status' do
-        expect(Geo::NodeStatusFetchService).to receive(:get).never
+        expect(Gitlab::HTTP).to receive(:get).never
 
         subject.execute
       end

ee/spec/services/geo/node_status_fetch_service_spec.rb

@@ -14,7 +14,7 @@ describe Geo::NodeStatusFetchService, :geo do
                        code: 401,
                        message: 'Unauthorized',
                        parsed_response: { 'message' => 'Test' } )
-      allow(described_class).to receive(:get).and_return(request)
+      allow(Gitlab::HTTP).to receive(:get).and_return(request)
 
       status = subject.call(secondary)
 
@@ -71,7 +71,7 @@ describe Geo::NodeStatusFetchService, :geo do
                cursor_last_event_id: 1,
                cursor_last_event_timestamp: Time.now.to_i }
       request = double(success?: true, parsed_response: data.stringify_keys, code: 200)
-      allow(described_class).to receive(:get).and_return(request)
+      allow(Gitlab::HTTP).to receive(:get).and_return(request)
 
       status = subject.call(secondary)
 
@@ -84,7 +84,7 @@ describe Geo::NodeStatusFetchService, :geo do
                        code: 401,
                        message: 'Unauthorized',
                        parsed_response: '<html><h1>You are not allowed</h1></html>')
-      allow(described_class).to receive(:get).and_return(request)
+      allow(Gitlab::HTTP).to receive(:get).and_return(request)
 
       status = subject.call(secondary)
 
@@ -94,7 +94,7 @@ describe Geo::NodeStatusFetchService, :geo do
 
     it 'alerts on bad SSL certficate' do
       message = 'bad certificate'
-      allow(described_class).to receive(:get).and_raise(OpenSSL::SSL::SSLError.new(message))
+      allow(Gitlab::HTTP).to receive(:get).and_raise(OpenSSL::SSL::SSLError.new(message))
 
       status = subject.call(secondary)
 
@@ -102,7 +102,7 @@ describe Geo::NodeStatusFetchService, :geo do
     end
 
     it 'handles connection refused' do
-      allow(described_class).to receive(:get).and_raise(Errno::ECONNREFUSED.new('bad connection'))
+      allow(Gitlab::HTTP).to receive(:get).and_raise(Errno::ECONNREFUSED.new('bad connection'))
 
       status = subject.call(secondary)
 
@@ -126,7 +126,7 @@ describe Geo::NodeStatusFetchService, :geo do
     end
 
     it 'returns the status from database if it could not fetch it' do
-      allow(described_class).to receive(:get).and_raise(Errno::ECONNREFUSED.new('bad connection'))
+      allow(Gitlab::HTTP).to receive(:get).and_raise(Errno::ECONNREFUSED.new('bad connection'))
       db_status = create(:geo_node_status, :healthy, geo_node: secondary)
 
       status = subject.call(secondary)

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 @@ 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

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,24 +71,32 @@ 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
 
     def delete(path, options = {})
       handle_exceptions do
-        self.class.delete(path, options.merge(headers: @headers))
+        Gitlab::HTTP.delete(path, build_options(options))
       end
     end
 
     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

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
 

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

rubocop/rubocop.rb

 # rubocop:disable Naming/FileName
 require_relative 'cop/gitlab/module_with_instance_variables'
 require_relative 'cop/gitlab/predicate_memoization'
+require_relative 'cop/gitlab/httparty'
 require_relative 'cop/include_sidekiq_worker'
 require_relative 'cop/line_break_around_conditional_block'
 require_relative 'cop/migration/add_column'

spec/lib/gitlab/http_spec.rb

+require 'spec_helper'
+
+describe Gitlab::HTTP do
+  describe 'allow_local_requests_from_hooks_and_services is' do
+    before do
+      WebMock.stub_request(:get, /.*/).to_return(status: 200, body: 'Success')
+    end
+
+    context 'disabled' do
+      before do
+        allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false)
+      end
+
+      it 'deny requests to localhost' do
+        expect { described_class.get('http://localhost:3003') }.to raise_error(URI::InvalidURIError)
+      end
+
+      it 'deny requests to private network' do
+        expect { described_class.get('http://192.168.1.2:3003') }.to raise_error(URI::InvalidURIError)
+      end
+
+      context 'if allow_local_requests set to true' do
+        it 'override the global value and allow requests to localhost or private network' do
+          expect { described_class.get('http://localhost:3003', allow_local_requests: true) }.not_to raise_error
+        end
+      end
+    end
+
+    context 'enabled' do
+      before do
+        allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true)
+      end
+
+      it 'allow requests to localhost' do
+        expect { described_class.get('http://localhost:3003') }.not_to raise_error
+      end
+
+      it 'allow requests to private network' do
+        expect { described_class.get('http://192.168.1.2:3003') }.not_to raise_error
+      end
+
+      context 'if allow_local_requests set to false' do
+        it 'override the global value and ban requests to localhost or private network' do
+          expect { described_class.get('http://localhost:3003', allow_local_requests: false) }.to raise_error(URI::InvalidURIError)
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/url_blocker_spec.rb

@@ -2,6 +2,8 @@ require 'spec_helper'
 
 describe Gitlab::UrlBlocker do
   describe '#blocked_url?' do
+    let(:valid_ports) { Project::VALID_IMPORT_PORTS }
+
     it 'allows imports from configured web host and port' do
       import_url = "http://#{Gitlab.config.gitlab.host}:#{Gitlab.config.gitlab.port}/t.git"
       expect(described_class.blocked_url?(import_url)).to be false
@@ -17,7 +19,7 @@ describe Gitlab::UrlBlocker do
     end
 
     it 'returns true for bad port' do
-      expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git')).to be true
+      expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git', valid_ports: valid_ports)).to be true
     end
 
     it 'returns true for alternative version of 127.0.0.1 (0177.1)' do
@@ -71,6 +73,47 @@ describe Gitlab::UrlBlocker do
     it 'returns false for legitimate URL' do
       expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git')).to be false
     end
+
+    context 'when allow_private_networks is' do
+      let(:private_networks) { ['192.168.1.2', '10.0.0.2', '172.16.0.2'] }
+      let(:fake_domain) { 'www.fakedomain.fake' }
+
+      context 'true (default)' do
+        it 'does not block urls from private networks' do
+          private_networks.each do |ip|
+            stub_domain_resolv(fake_domain, ip)
+
+            expect(described_class).not_to be_blocked_url("http://#{fake_domain}")
+
+            unstub_domain_resolv
+
+            expect(described_class).not_to be_blocked_url("http://#{ip}")
+          end
+        end
+      end
+
+      context 'false' do
+        it 'blocks urls from private networks' do
+          private_networks.each do |ip|
+            stub_domain_resolv(fake_domain, ip)
+
+            expect(described_class).to be_blocked_url("http://#{fake_domain}", allow_private_networks: false)
+
+            unstub_domain_resolv
+
+            expect(described_class).to be_blocked_url("http://#{ip}", allow_private_networks: false)
+          end
+        end
+      end
+
+      def stub_domain_resolv(domain, ip)
+        allow(Addrinfo).to receive(:getaddrinfo).with(domain, any_args).and_return([double(ip_address: ip, ipv4_private?: true)])
+      end
+
+      def unstub_domain_resolv
+        allow(Addrinfo).to receive(:getaddrinfo).and_call_original
+      end
+    end
   end
 
   # Resolv does not support resolving UTF-8 domain names

spec/lib/mattermost/command_spec.rb

@@ -4,10 +4,11 @@ describe Mattermost::Command do
   let(:params) { { 'token' => 'token', team_id: 'abc' } }
 
   before do
-    Mattermost::Session.base_uri('http://mattermost.example.com')
+    session = Mattermost::Session.new(nil)
+    session.base_uri = 'http://mattermost.example.com'
 
     allow_any_instance_of(Mattermost::Client).to receive(:with_session)
-      .and_yield(Mattermost::Session.new(nil))
+      .and_yield(session)
   end
 
   describe '#create' do

spec/lib/mattermost/session_spec.rb

@@ -15,7 +15,7 @@ describe Mattermost::Session, type: :request do
   it { is_expected.to respond_to(:strategy) }
 
   before do
-    described_class.base_uri(mattermost_url)
+    subject.base_uri = mattermost_url
   end
 
   describe '#with session' do

spec/lib/mattermost/team_spec.rb

@@ -2,10 +2,11 @@ require 'spec_helper'
 
 describe Mattermost::Team do
   before do
-    Mattermost::Session.base_uri('http://mattermost.example.com')
+    session = Mattermost::Session.new(nil)
+    session.base_uri = 'http://mattermost.example.com'
 
     allow_any_instance_of(Mattermost::Client).to receive(:with_session)
-      .and_yield(Mattermost::Session.new(nil))
+      .and_yield(session)
   end
 
   describe '#all' do

spec/models/project_services/mattermost_slash_commands_service_spec.rb

@@ -9,10 +9,11 @@ describe MattermostSlashCommandsService do
     let(:user) { create(:user) }
 
     before do
-      Mattermost::Session.base_uri("http://mattermost.example.com")
+      session = Mattermost::Session.new(nil)
+      session.base_uri = 'http://mattermost.example.com'
 
       allow_any_instance_of(Mattermost::Client).to receive(:with_session)
-        .and_yield(Mattermost::Session.new(nil))
+        .and_yield(session)
     end
 
     describe '#configure' do

spec/rubocop/cop/gitlab/httparty_spec.rb

+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../../rubocop/cop/gitlab/httparty'
+
+describe RuboCop::Cop::Gitlab::HTTParty do # rubocop:disable RSpec/FilePath
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  shared_examples('registering include offense') do |options|
+    let(:offending_lines) { options[:offending_lines] }
+
+    it 'registers an offense when the class includes HTTParty' do
+      inspect_source(source)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(offending_lines.size)
+        expect(cop.offenses.map(&:line)).to eq(offending_lines)
+      end
+    end
+  end
+
+  shared_examples('registering call offense') do |options|
+    let(:offending_lines) { options[:offending_lines] }
+
+    it 'registers an offense when the class calls HTTParty' do
+      inspect_source(source)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(offending_lines.size)
+        expect(cop.offenses.map(&:line)).to eq(offending_lines)
+      end
+    end
+  end
+
+  context 'when source is a regular module' do
+    it_behaves_like 'registering include offense', offending_lines: [2] do
+      let(:source) do
+        <<~RUBY
+          module M
+            include HTTParty
+          end
+        RUBY
+      end
+    end
+  end
+
+  context 'when source is a regular class' do
+    it_behaves_like 'registering include offense', offending_lines: [2] do
+      let(:source) do
+        <<~RUBY
+          class Foo
+            include HTTParty
+          end
+        RUBY
+      end
+    end
+  end
+
+  context 'when HTTParty is called' do
+    it_behaves_like 'registering call offense', offending_lines: [3] do
+      let(:source) do
+        <<~RUBY
+          class Foo
+            def bar
+              HTTParty.get('http://example.com')
+            end
+          end
+        RUBY
+      end
+    end
+  end
+end

spec/services/web_hook_service_spec.rb

@@ -14,6 +14,20 @@ describe WebHookService do
   end
   let(:service_instance) { described_class.new(project_hook, data, :push_hooks) }
 
+  describe '#initialize' do
+    it 'allow_local_requests is true if hook is a SystemHook' do
+      instance = described_class.new(build(:system_hook), data, :system_hook)
+      expect(instance.request_options[:allow_local_requests]).to be_truthy
+    end
+
+    it 'allow_local_requests is false if hook is not a SystemHook' do
+      %i(project_hook service_hook web_hook_log).each do |hook|
+        instance = described_class.new(build(hook), data, hook)
+        expect(instance.request_options[:allow_local_requests]).to be_falsey
+      end
+    end
+  end
+
   describe '#execute' do
     before do
       project.hooks << [project_hook]