Merge branch 'security-stored-xss-for-environments' into 'master'

[master] Stored XSS for Environments

Closes #2727

See merge request gitlab/gitlabhq!2594

changelogs/unreleased/security-stored-xss-for-environments.yml

@@ -0,0 +1,5 @@
+---
+title: Fix stored XSS for Environments
+merge_request:
+author:
+type: security

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

lib/gitlab/url_blocker.rb

@@ -111,12 +111,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
 

spec/factories/ci/builds.rb

@@ -308,7 +308,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

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

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