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

Introduce latest security changes to `master`

See merge request gitlab-org/gitlab-ce!17905

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

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/fix-auth0-unsafe-login.yml

+---
+title: Fix GitLab Auth0 integration signing in the wrong user
+merge_request:
+author:
+type: security

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

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

changelogs/unreleased/remove-unnecessary-validate-project.yml

----
-title: 'Remove unecessary validate: true from belongs_to :project'
-merge_request:
-author:
-type: fixed

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/post_migrate/20180220150310_remove_empty_extern_uid_auth0_identities.rb

+class RemoveEmptyExternUidAuth0Identities < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class Identity < ActiveRecord::Base
+    self.table_name = 'identities'
+    include EachBatch
+  end
+
+  def up
+    broken_auth0_identities.each_batch do |identity|
+      identity.delete_all
+    end
+  end
+
+  def broken_auth0_identities
+    Identity.where(provider: 'auth0', extern_uid: [nil, ''])
+  end
+
+  def down
+  end
+end

db/schema.rb

@@ -157,6 +157,7 @@ ActiveRecord::Schema.define(version: 20180320182229) do
     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|

doc/integration/auth0.md

@@ -56,7 +56,8 @@ for initial settings.
           "name" => "auth0",
           "args" => { client_id: 'YOUR_AUTH0_CLIENT_ID',
                       client_secret: 'YOUR_AUTH0_CLIENT_SECRET',
-                      namespace: 'YOUR_AUTH0_DOMAIN'
+                      domain: 'YOUR_AUTH0_DOMAIN',
+                      scope: 'openid profile email'
                     }
         }
       ]
@@ -69,8 +70,8 @@ for initial settings.
           args: {
             client_id: 'YOUR_AUTH0_CLIENT_ID',
             client_secret: 'YOUR_AUTH0_CLIENT_SECRET',
-            namespace: 'YOUR_AUTH0_DOMAIN'
-            }
+            domain: 'YOUR_AUTH0_DOMAIN',
+            scope: 'openid profile email' }
         }
     ```

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?
-        port < 1024 && !VALID_PORTS.include?(port)
+      def blocked_port?(port, valid_ports)
+        return false if port.blank? || valid_ports.blank?
+        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/controllers/omniauth_callbacks_controller_spec.rb

@@ -3,73 +3,90 @@ require 'spec_helper'
 describe OmniauthCallbacksController do
   include LoginHelpers
-  let(:user) { create(:omniauth_user, extern_uid: 'my-uid', provider: provider) }
-  let(:provider) { :github }
+  let(:user) { create(:omniauth_user, extern_uid: extern_uid, provider: provider) }
   before do
-    mock_auth_hash(provider.to_s, 'my-uid', user.email)
+    mock_auth_hash(provider.to_s, extern_uid, user.email)
     stub_omniauth_provider(provider, context: request)
   end
-  it 'allows sign in' do
-    post provider
-    expect(request.env['warden']).to be_authenticated
-  end
-  shared_context 'sign_up' do
-    let(:user) { double(email: 'new@example.com') }
-    before do
-      stub_omniauth_setting(block_auto_created_users: false)
+  context 'github' do
+    let(:extern_uid) { 'my-uid' }
+    let(:provider) { :github }
+    it 'allows sign in' do
+      post provider
+
+      expect(request.env['warden']).to be_authenticated
+    end
+    shared_context 'sign_up' do
+      let(:user) { double(email: 'new@example.com') }
+      before do
+        stub_omniauth_setting(block_auto_created_users: false)
+      end
     end
-  end
-  context 'sign up' do
-    include_context 'sign_up'
-    it 'is allowed' do
-      post provider
-      expect(request.env['warden']).to be_authenticated
+    context 'sign up' do
+      include_context 'sign_up'
+      it 'is allowed' do
+        post provider
+        expect(request.env['warden']).to be_authenticated
+      end
     end
-  end
-  context 'when OAuth is disabled' do
-    before do
-      stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
-      settings = Gitlab::CurrentSettings.current_application_settings
-      settings.update(disabled_oauth_sign_in_sources: [provider.to_s])
-    end
-    it 'prevents login via POST' do
-      post provider
-      expect(request.env['warden']).not_to be_authenticated
-    end
-    it 'shows warning when attempting login' do
-      post provider
-      expect(response).to redirect_to new_user_session_path
-      expect(flash[:alert]).to eq('Signing in using GitHub has been disabled')
-    end
-    it 'allows linking the disabled provider' do
-      user.identities.destroy_all
-      sign_in(user)
-      expect { post provider }.to change { user.reload.identities.count }.by(1)
-    end
-    context 'sign up' do
-      include_context 'sign_up'
-      it 'is prevented' do
-        post provider
-        expect(request.env['warden']).not_to be_authenticated
+    context 'when OAuth is disabled' do
+      before do
+        stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
+        settings = Gitlab::CurrentSettings.current_application_settings
+        settings.update(disabled_oauth_sign_in_sources: [provider.to_s])
+      end
+      it 'prevents login via POST' do
+        post provider
+        expect(request.env['warden']).not_to be_authenticated
+      end
+      it 'shows warning when attempting login' do
+        post provider
+        expect(response).to redirect_to new_user_session_path
+        expect(flash[:alert]).to eq('Signing in using GitHub has been disabled')
+      end
+      it 'allows linking the disabled provider' do
+        user.identities.destroy_all
+        sign_in(user)
+        expect { post provider }.to change { user.reload.identities.count }.by(1)
+      end
+      context 'sign up' do
+        include_context 'sign_up'
+        it 'is prevented' do
+          post provider
+          expect(request.env['warden']).not_to be_authenticated
+        end
       end
     end
   end
+
+  context 'auth0' do
+    let(:extern_uid) { '' }
+    let(:provider) { :auth0 }
+
+    it 'does not allow sign in without extern_uid' do
+      post 'auth0'
+
+      expect(request.env['warden']).not_to be_authenticated
+      expect(response.status).to eq(302)
+      expect(controller).to set_flash[:alert].to('Wrong extern UID provided. Make sure Auth0 is configured correctly.')
+    end
+  end
 end

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