Merge branch 'security-fix-uri-xss-applications-11-3' into 'security-11-3'

[11.3] Reflected XSS in OAuth Authorize window due to redirect_uri allowing arbitrary protocols

See merge request gitlab/gitlabhq!2581

7 files changed, 121 insertions, 2 deletions

diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb
index a1fe02dc852..37eac46a30a 100644
--- a/app/controllers/oauth/applications_controller.rb
+++ b/app/controllers/oauth/applications_controller.rb
@@ -7,7 +7,7 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
   before_action :verify_user_oauth_applications_enabled
   before_action :authenticate_user!
   before_action :add_gon_variables
-  before_action :load_scopes, only: [:index, :create, :edit]
+  before_action :load_scopes, only: [:index, :create, :edit, :update]
 
   helper_method :can?
 
diff --git a/changelogs/unreleased/security-fix-uri-xss-applications.yml b/changelogs/unreleased/security-fix-uri-xss-applications.yml
new file mode 100644
index 00000000000..0eaa1b1c4a3
--- /dev/null
+++ b/changelogs/unreleased/security-fix-uri-xss-applications.yml
@@ -0,0 +1,5 @@
+---
+title: Resolve reflected XSS in Ouath authorize window
+merge_request:
+author:
+type: security
diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb
index f321b4ea763..6be5c00daaa 100644
--- a/config/initializers/doorkeeper.rb
+++ b/config/initializers/doorkeeper.rb
@@ -48,6 +48,13 @@ Doorkeeper.configure do
   #
   force_ssl_in_redirect_uri false
 
+  # Specify what redirect URI's you want to block during Application creation.
+  # Any redirect URI is whitelisted by default.
+  #
+  # You can use this option in order to forbid URI's with 'javascript' scheme
+  # for example.
+  forbid_redirect_uri { |uri| %w[data vbscript javascript].include?(uri.scheme.to_s.downcase) }
+
   # Provide support for an owner to be assigned to each registered application (disabled by default)
   # Optional parameter confirmation: true (default false) if you want to enforce ownership of
   # a registered application
diff --git a/db/post_migrate/20181026091631_migrate_forbidden_redirect_uris.rb b/db/post_migrate/20181026091631_migrate_forbidden_redirect_uris.rb
new file mode 100644
index 00000000000..ff5510e8eb7
--- /dev/null
+++ b/db/post_migrate/20181026091631_migrate_forbidden_redirect_uris.rb
@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+class MigrateForbiddenRedirectUris < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  FORBIDDEN_SCHEMES = %w[data:// vbscript:// javascript://]
+  NEW_URI = 'http://forbidden-scheme-has-been-overwritten'
+
+  disable_ddl_transaction!
+
+  def up
+    update_forbidden_uris(:oauth_applications)
+    update_forbidden_uris(:oauth_access_grants)
+  end
+
+  def down
+    # noop
+  end
+
+  private
+
+  def update_forbidden_uris(table_name)
+    update_column_in_batches(table_name, :redirect_uri, NEW_URI) do |table, query|
+      where_clause = FORBIDDEN_SCHEMES.map do |scheme|
+        table[:redirect_uri].matches("#{scheme}%")
+      end.inject(&:or)
+
+      query.where(where_clause)
+    end
+  end
+end
diff --git a/spec/controllers/oauth/applications_controller_spec.rb b/spec/controllers/oauth/applications_controller_spec.rb
index 1195f44f37d..af87c607de9 100644
--- a/spec/controllers/oauth/applications_controller_spec.rb
+++ b/spec/controllers/oauth/applications_controller_spec.rb
@@ -23,6 +23,23 @@ describe Oauth::ApplicationsController do
         expect(response).to have_gitlab_http_status(302)
         expect(response).to redirect_to(profile_path)
       end
+
+      context 'redirect_uri' do
+        render_views
+
+        it 'shows an error for a forbidden URI' do
+          invalid_uri_params = {
+            doorkeeper_application: {
+              name: 'foo',
+              redirect_uri: 'javascript://alert()'
+            }
+          }
+
+          post :create, invalid_uri_params
+
+          expect(response.body).to include 'Redirect URI is forbidden by the server'
+        end
+      end
     end
   end
 end
diff --git a/spec/migrations/migrate_forbidden_redirect_uris_spec.rb b/spec/migrations/migrate_forbidden_redirect_uris_spec.rb
new file mode 100644
index 00000000000..0bc13a3974a
--- /dev/null
+++ b/spec/migrations/migrate_forbidden_redirect_uris_spec.rb
@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20181026091631_migrate_forbidden_redirect_uris.rb')
+
+describe MigrateForbiddenRedirectUris, :migration do
+  let(:oauth_application) { table(:oauth_applications) }
+  let(:oauth_access_grant) { table(:oauth_access_grants) }
+
+  let!(:control_app) { oauth_application.create(random_params) }
+  let!(:control_access_grant) { oauth_application.create(random_params) }
+  let!(:forbidden_js_app) { oauth_application.create(random_params.merge(redirect_uri: 'javascript://alert()')) }
+  let!(:forbidden_vb_app) { oauth_application.create(random_params.merge(redirect_uri: 'VBSCRIPT://alert()')) }
+  let!(:forbidden_access_grant) { oauth_application.create(random_params.merge(redirect_uri: 'vbscript://alert()')) }
+
+  context 'oauth application' do
+    it 'migrates forbidden javascript URI' do
+      expect { migrate! }.to change { forbidden_js_app.reload.redirect_uri }.to('http://forbidden-scheme-has-been-overwritten')
+    end
+
+    it 'migrates forbidden VBScript URI' do
+      expect { migrate! }.to change { forbidden_vb_app.reload.redirect_uri }.to('http://forbidden-scheme-has-been-overwritten')
+    end
+
+    it 'does not migrate a valid URI' do
+      expect { migrate! }.not_to change { control_app.reload.redirect_uri }
+    end
+  end
+
+  context 'access grant' do
+    it 'migrates forbidden VBScript URI' do
+      expect { migrate! }.to change { forbidden_access_grant.reload.redirect_uri }.to('http://forbidden-scheme-has-been-overwritten')
+    end
+
+    it 'does not migrate a valid URI' do
+      expect { migrate! }.not_to change { control_access_grant.reload.redirect_uri }
+    end
+  end
+
+  def random_params
+    {
+      name: 'test',
+      secret: 'test',
+      uid: Doorkeeper::OAuth::Helpers::UniqueToken.generate,
+      redirect_uri: 'http://valid.com'
+    }
+  end
+end
diff --git a/spec/requests/api/applications_spec.rb b/spec/requests/api/applications_spec.rb
index f56bc932f40..0f0bf126f8c 100644
--- a/spec/requests/api/applications_spec.rb
+++ b/spec/requests/api/applications_spec.rb
@@ -24,7 +24,7 @@ describe API::Applications, :api do
 
       it 'does not allow creating an application with the wrong redirect_uri format' do
         expect do
-          post api('/applications', admin_user), name: 'application_name', redirect_uri: 'wrong_url_format', scopes: ''
+          post api('/applications', admin_user), name: 'application_name', redirect_uri: 'http://', scopes: ''
         end.not_to change { Doorkeeper::Application.count }
 
         expect(response).to have_http_status 400
@@ -32,6 +32,16 @@ describe API::Applications, :api do
         expect(json_response['message']['redirect_uri'][0]).to eq('must be an absolute URI.')
       end
 
+      it 'does not allow creating an application with a forbidden URI format' do
+        expect do
+          post api('/applications', admin_user), name: 'application_name', redirect_uri: 'javascript://alert()', scopes: ''
+        end.not_to change { Doorkeeper::Application.count }
+
+        expect(response).to have_gitlab_http_status(400)
+        expect(json_response).to be_a Hash
+        expect(json_response['message']['redirect_uri'][0]).to eq('is forbidden by the server.')
+      end
+
       it 'does not allow creating an application without a name' do
         expect do
           post api('/applications', admin_user), redirect_uri: 'http://application.url', scopes: ''