Commit 5332995c authored by James Lopez's avatar James Lopez

Resolve reflected XSS in Ouath authorize window

parent 34d84fd2
...@@ -9,7 +9,7 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController ...@@ -9,7 +9,7 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
before_action :verify_user_oauth_applications_enabled, except: :index before_action :verify_user_oauth_applications_enabled, except: :index
before_action :authenticate_user! before_action :authenticate_user!
before_action :add_gon_variables 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? helper_method :can?
......
---
title: Resolve reflected XSS in Ouath authorize window
merge_request:
author:
type: security
...@@ -48,6 +48,13 @@ Doorkeeper.configure do ...@@ -48,6 +48,13 @@ Doorkeeper.configure do
# #
force_ssl_in_redirect_uri false 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) # 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 # Optional parameter confirmation: true (default false) if you want to enforce ownership of
# a registered application # a registered application
......
# 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
...@@ -11,7 +11,7 @@ ...@@ -11,7 +11,7 @@
# #
# It's strongly recommended that you check this file into your version control system. # It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20181013005024) do ActiveRecord::Schema.define(version: 20181026091631) do
# These are extensions that must be enabled in order to support this database # These are extensions that must be enabled in order to support this database
enable_extension "plpgsql" enable_extension "plpgsql"
......
...@@ -40,6 +40,23 @@ describe Oauth::ApplicationsController do ...@@ -40,6 +40,23 @@ describe Oauth::ApplicationsController do
expect(response).to have_gitlab_http_status(302) expect(response).to have_gitlab_http_status(302)
expect(response).to redirect_to(profile_path) expect(response).to redirect_to(profile_path)
end 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 end
......
# 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
...@@ -25,7 +25,7 @@ describe API::Applications, :api do ...@@ -25,7 +25,7 @@ describe API::Applications, :api do
it 'does not allow creating an application with the wrong redirect_uri format' do it 'does not allow creating an application with the wrong redirect_uri format' do
expect 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 } end.not_to change { Doorkeeper::Application.count }
expect(response).to have_gitlab_http_status(400) expect(response).to have_gitlab_http_status(400)
...@@ -33,6 +33,16 @@ describe API::Applications, :api do ...@@ -33,6 +33,16 @@ describe API::Applications, :api do
expect(json_response['message']['redirect_uri'][0]).to eq('must be an absolute URI.') expect(json_response['message']['redirect_uri'][0]).to eq('must be an absolute URI.')
end 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 it 'does not allow creating an application without a name' do
expect do expect do
post api('/applications', admin_user), redirect_uri: 'http://application.url', scopes: '' post api('/applications', admin_user), redirect_uri: 'http://application.url', scopes: ''
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment