Validate URI scheme also for internal URI

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.

4 files changed, 20 insertions, 5 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/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb
index 86efe8ad114..4b1b58d68d8 100644
--- a/lib/gitlab/url_blocker.rb
+++ b/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
 
diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb
index 90754319f05..07c1fc31152 100644
--- a/spec/factories/ci/builds.rb
+++ b/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
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