diff options
author | Alessio Caiazza <acaiazza@gitlab.com> | 2018-11-08 09:06:07 +0000 |
---|---|---|
committer | Alessio Caiazza <acaiazza@gitlab.com> | 2018-11-14 11:00:13 +0100 |
commit | 5fae9ea1e3297e70179b82074c710839a4742bd4 (patch) | |
tree | e2cedb27c97e100929e313dc7f9c6b6cef189274 | |
parent | 1c315f4c26ee0d682dd232c077a1bf38a7634b70 (diff) | |
download | gitlab-ce-5fae9ea1e3297e70179b82074c710839a4742bd4.tar.gz |
Validate URI scheme also for internal URI
This is a backport for 11.3 stable branch.
Gitlab::UrlBlocker ignores scheme when validating URI matching either
config.gitlab or config.gitlab_shell
This patch enforces matching config.gitlab.protocol for internal web and
ssh for internal shell.
A cleanup migration for stored XSS from environments table is included.
-rw-r--r-- | changelogs/unreleased/security-stored-xss-for-environments.yml | 5 | ||||
-rw-r--r-- | db/migrate/20181108091549_cleanup_environments_external_url.rb | 18 | ||||
-rw-r--r-- | db/schema.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/url_blocker.rb | 6 | ||||
-rw-r--r-- | spec/factories/ci/builds.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/url_blocker_spec.rb | 12 | ||||
-rw-r--r-- | spec/migrations/cleanup_environments_external_url_spec.rb | 28 |
7 files changed, 67 insertions, 6 deletions
diff --git a/changelogs/unreleased/security-stored-xss-for-environments.yml b/changelogs/unreleased/security-stored-xss-for-environments.yml new file mode 100644 index 00000000000..5d78ca00942 --- /dev/null +++ b/changelogs/unreleased/security-stored-xss-for-environments.yml @@ -0,0 +1,5 @@ +--- +title: Fix stored XSS for Environments +merge_request: +author: +type: security diff --git a/db/migrate/20181108091549_cleanup_environments_external_url.rb b/db/migrate/20181108091549_cleanup_environments_external_url.rb new file mode 100644 index 00000000000..8d6c20a4b15 --- /dev/null +++ b/db/migrate/20181108091549_cleanup_environments_external_url.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class CleanupEnvironmentsExternalUrl < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + update_column_in_batches(:environments, :external_url, nil) do |table, query| + query.where(table[:external_url].matches('javascript://%')) + end + end + + def down + end +end diff --git a/db/schema.rb b/db/schema.rb index e872fdcab12..15f0dd9f620 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20181014121030) do +ActiveRecord::Schema.define(version: 20181108091549) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index a2cce9151c3..e48d2e39104 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -109,12 +109,14 @@ module Gitlab end def internal_web?(uri) - uri.hostname == config.gitlab.host && + uri.scheme == config.gitlab.protocol && + uri.hostname == config.gitlab.host && (uri.port.blank? || uri.port == config.gitlab.port) end def internal_shell?(uri) - uri.hostname == config.gitlab_shell.ssh_host && + uri.scheme == 'ssh' && + uri.hostname == config.gitlab_shell.ssh_host && (uri.port.blank? || uri.port == config.gitlab_shell.ssh_port) end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 9813190925b..5aea5251980 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -257,7 +257,7 @@ FactoryBot.define do trait :with_runner_session do after(:build) do |build| - build.build_runner_session(url: 'ws://localhost') + build.build_runner_session(url: 'https://localhost') end end end diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 8df0facdab3..35b550283b5 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -10,8 +10,8 @@ describe Gitlab::UrlBlocker do expect(described_class.blocked_url?(import_url)).to be false end - it 'allows imports from configured SSH host and port' do - import_url = "http://#{Gitlab.config.gitlab_shell.ssh_host}:#{Gitlab.config.gitlab_shell.ssh_port}/t.git" + it 'allows mirroring from configured SSH host and port' do + import_url = "ssh://#{Gitlab.config.gitlab_shell.ssh_host}:#{Gitlab.config.gitlab_shell.ssh_port}/t.git" expect(described_class.blocked_url?(import_url)).to be false end @@ -29,6 +29,14 @@ describe Gitlab::UrlBlocker do expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', protocols: ['http'])).to be true end + it 'returns true for bad protocol on configured web/SSH host and ports' do + web_url = "javascript://#{Gitlab.config.gitlab.host}:#{Gitlab.config.gitlab.port}/t.git%0aalert(1)" + expect(described_class.blocked_url?(web_url)).to be true + + ssh_url = "javascript://#{Gitlab.config.gitlab_shell.ssh_host}:#{Gitlab.config.gitlab_shell.ssh_port}/t.git%0aalert(1)" + expect(described_class.blocked_url?(ssh_url)).to be true + end + it 'returns true for localhost IPs' do expect(described_class.blocked_url?('https://0.0.0.0/foo/foo.git')).to be true expect(described_class.blocked_url?('https://[::1]/foo/foo.git')).to be true diff --git a/spec/migrations/cleanup_environments_external_url_spec.rb b/spec/migrations/cleanup_environments_external_url_spec.rb new file mode 100644 index 00000000000..07ddaf3d38f --- /dev/null +++ b/spec/migrations/cleanup_environments_external_url_spec.rb @@ -0,0 +1,28 @@ +require 'spec_helper' +require Rails.root.join('db', 'migrate', '20181108091549_cleanup_environments_external_url.rb') + +describe CleanupEnvironmentsExternalUrl, :migration do + let(:environments) { table(:environments) } + let(:invalid_entries) { environments.where(environments.arel_table[:external_url].matches('javascript://%')) } + let(:namespaces) { table(:namespaces) } + let(:projects) { table(:projects) } + + before do + namespace = namespaces.create(name: 'foo', path: 'foo') + project = projects.create!(namespace_id: namespace.id) + + environments.create!(id: 1, project_id: project.id, name: 'poisoned', slug: 'poisoned', external_url: 'javascript://alert("1")') + end + + it 'clears every environment with a javascript external_url' do + expect do + subject.up + end.to change { invalid_entries.count }.from(1).to(0) + end + + it 'do not removes environments' do + expect do + subject.up + end.not_to change { environments.count } + end +end |