Merge branch 'security-11-4-stored-xss-for-environments' into 'security-11-4'

[11.4] Stored XSS for Environments

See merge request gitlab/gitlabhq!2615

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 50768d2693c..cab00a4dbf5 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 85ba7d4097d..d9203b70e96 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -278,7 +278,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