Merge branch 'rs-alphanumeric-ssh-params' into 'security-9-4'jej/security-release-2017-08-10

Ensure user and hostnames begin with an alnum character in UrlBlocker

See merge request !2138

3 files changed, 47 insertions, 0 deletions

diff --git a/changelogs/unreleased/rs-alphanumeric-ssh-params.yml b/changelogs/unreleased/rs-alphanumeric-ssh-params.yml
new file mode 100644
index 00000000000..426b01cafad
--- /dev/null
+++ b/changelogs/unreleased/rs-alphanumeric-ssh-params.yml
@@ -0,0 +1,5 @@
+---
+title: Disallow Git URLs that include a username or hostname beginning with a non-alphanumeric
+  character
+merge_request:
+author:
diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb
index 7e14a566696..fee1a127fd7 100644
--- a/lib/gitlab/url_blocker.rb
+++ b/lib/gitlab/url_blocker.rb
@@ -19,6 +19,8 @@ module Gitlab
           return false if internal?(uri)
 
           return true if blocked_port?(uri.port)
+          return true if blocked_user_or_hostname?(uri.user)
+          return true if blocked_user_or_hostname?(uri.hostname)
 
           server_ips = Resolv.getaddresses(uri.hostname)
           return true if (blocked_ips & server_ips).any?
@@ -37,6 +39,12 @@ module Gitlab
         port < 1024 && !VALID_PORTS.include?(port)
       end
 
+      def blocked_user_or_hostname?(value)
+        return false if value.blank?
+
+        value !~ /\A\p{Alnum}/
+      end
+
       def internal?(uri)
         internal_web?(uri) || internal_shell?(uri)
       end
diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb
index f5b4882815f..f18823b61ef 100644
--- a/spec/lib/gitlab/url_blocker_spec.rb
+++ b/spec/lib/gitlab/url_blocker_spec.rb
@@ -20,6 +20,34 @@ describe Gitlab::UrlBlocker do
       expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git')).to be true
     end
 
+    it 'returns true for a non-alphanumeric hostname' do
+      stub_resolv
+
+      aggregate_failures do
+        expect(described_class).to be_blocked_url('ssh://-oProxyCommand=whoami/a')
+
+        # The leading character here is a Unicode "soft hyphen"
+        expect(described_class).to be_blocked_url('ssh://­oProxyCommand=whoami/a')
+
+        # Unicode alphanumerics are allowed
+        expect(described_class).not_to be_blocked_url('ssh://ğitlab.com/a')
+      end
+    end
+
+    it 'returns true for a non-alphanumeric username' do
+      stub_resolv
+
+      aggregate_failures do
+        expect(described_class).to be_blocked_url('ssh://-oProxyCommand=whoami@example.com/a')
+
+        # The leading character here is a Unicode "soft hyphen"
+        expect(described_class).to be_blocked_url('ssh://­oProxyCommand=whoami@example.com/a')
+
+        # Unicode alphanumerics are allowed
+        expect(described_class).not_to be_blocked_url('ssh://ğitlab@example.com/a')
+      end
+    end
+
     it 'returns true for invalid URL' do
       expect(described_class.blocked_url?('http://:8080')).to be true
     end
@@ -28,4 +56,10 @@ describe Gitlab::UrlBlocker do
       expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git')).to be false
     end
   end
+
+  # Resolv does not support resolving UTF-8 domain names
+  # See https://bugs.ruby-lang.org/issues/4270
+  def stub_resolv
+    allow(Resolv).to receive(:getaddresses).and_return([])
+  end
 end