From 5736d6606ad7c6d729baa6c4ef789a47ada4ceda Mon Sep 17 00:00:00 2001 From: Cindy Pallares Date: Wed, 28 Nov 2018 22:53:48 +0000 Subject: [PATCH] Merge branch 'security-fix-uri-xss-applications' into 'master' [master] Resolve "Reflected XSS in OAuth Authorize window due to redirect_uri allowing arbitrary protocols" See merge request gitlab/gitlabhq!2572 --- .../oauth/applications_controller.rb | 2 +- .../security-fix-uri-xss-applications.yml | 5 ++ config/initializers/doorkeeper.rb | 7 +++ ...6091631_migrate_forbidden_redirect_uris.rb | 32 +++++++++++++ .../oauth/applications_controller_spec.rb | 17 +++++++ .../migrate_forbidden_redirect_uris_spec.rb | 48 +++++++++++++++++++ spec/requests/api/applications_spec.rb | 12 ++++- 7 files changed, 121 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/security-fix-uri-xss-applications.yml create mode 100644 db/post_migrate/20181026091631_migrate_forbidden_redirect_uris.rb create mode 100644 spec/migrations/migrate_forbidden_redirect_uris_spec.rb diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb index b50f140dc80..ab4ca56bb49 100644 --- a/app/controllers/oauth/applications_controller.rb +++ b/app/controllers/oauth/applications_controller.rb @@ -9,7 +9,7 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController before_action :verify_user_oauth_applications_enabled, except: :index 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 ace8a954e92..b4219856fc0 100644 --- a/spec/controllers/oauth/applications_controller_spec.rb +++ b/spec/controllers/oauth/applications_controller_spec.rb @@ -40,6 +40,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 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 270e12bf201..6154be5c425 100644 --- a/spec/requests/api/applications_spec.rb +++ b/spec/requests/api/applications_spec.rb @@ -25,7 +25,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_gitlab_http_status(400) @@ -33,6 +33,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: '' -- GitLab