Merge branch 'security-354059-detect-host-keys-blind-ssrf-14-9' into '14-9-stable-ee'

Fix blind SSRF when looking up SSH host keys for mirroring

See merge request gitlab-org/security/gitlab!2309

app/models/ssh_host_key.rb

@@ -46,11 +46,11 @@ class SshHostKey
       .select(&:valid?)
   end
-  attr_reader :project, :url, :compare_host_keys
+  attr_reader :project, :url, :ip, :compare_host_keys
   def initialize(project:, url:, compare_host_keys: nil)
     @project = project
-    @url = normalize_url(url)
+    @url, @ip = normalize_url(url)
     @compare_host_keys = compare_host_keys
   end
@@ -90,9 +90,11 @@ class SshHostKey
   end
   def calculate_reactive_cache
+    input = [ip, url.hostname].compact.join(' ')
+
     known_hosts, errors, status =
       Open3.popen3({}, *%W[ssh-keyscan -T 5 -p #{url.port} -f-]) do |stdin, stdout, stderr, wait_thr|
-        stdin.puts(url.host)
+        stdin.puts(input)
         stdin.close
         [
@@ -127,11 +129,31 @@ class SshHostKey
   end
   def normalize_url(url)
-    full_url = ::Addressable::URI.parse(url)
-    raise ArgumentError, "Invalid URL" unless full_url&.scheme == 'ssh'
-    Addressable::URI.parse("ssh://#{full_url.host}:#{full_url.inferred_port}")
-  rescue Addressable::URI::InvalidURIError
+    url, real_hostname = Gitlab::UrlBlocker.validate!(
+      url,
+      schemes: %w[ssh],
+      allow_localhost: allow_local_requests?,
+      allow_local_network: allow_local_requests?,
+      dns_rebind_protection: Gitlab::CurrentSettings.dns_rebinding_protection_enabled?
+    )
+
+    # When DNS rebinding protection is required, the hostname is replaced by the
+    # resolved IP. However, `url` is used in `id`, so we can't change it. Track
+    # the resolved IP separately instead.
+    if real_hostname
+      ip = url.hostname
+      url.hostname = real_hostname
+    end
+
+    # Ensure ssh://foo and ssh://foo:22 share the same cache
+    url.port = url.inferred_port
+    [url, ip]
+  rescue Gitlab::UrlBlocker::BlockedUrlError
     raise ArgumentError, "Invalid URL"
   end
+
+  def allow_local_requests?
+    Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services?
+  end
 end

spec/controllers/projects/mirrors_controller_spec.rb

@@ -177,6 +177,7 @@ RSpec.describe Projects::MirrorsController do
         INVALID
         git@example.com:foo/bar.git
         ssh://git@example.com:foo/bar.git
+        ssh://127.0.0.1/foo/bar.git
       ].each do |url|
         it "returns an error with a 400 response for URL #{url.inspect}" do
           do_get(project, url)

spec/models/ssh_host_key_spec.rb

@@ -4,7 +4,9 @@ require 'spec_helper'
 RSpec.describe SshHostKey do
   using RSpec::Parameterized::TableSyntax
+
   include ReactiveCachingHelpers
+  include StubRequests
   let(:key1) do
     'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQC3UpyF2iLqy1d63M6k3jH1vuEnq/NWtE+o' \
@@ -35,6 +37,7 @@ RSpec.describe SshHostKey do
   let(:extra) { known_hosts + "foo\nbar\n" }
   let(:reversed) { known_hosts.lines.reverse.join }
+  let(:url) { 'ssh://example.com:2222' }
   let(:compare_host_keys) { nil }
   def stub_ssh_keyscan(args, status: true, stdout: "", stderr: "")
@@ -50,7 +53,7 @@ RSpec.describe SshHostKey do
   let(:project) { build(:project) }
-  subject(:ssh_host_key) { described_class.new(project: project, url: 'ssh://example.com:2222', compare_host_keys: compare_host_keys) }
+  subject(:ssh_host_key) { described_class.new(project: project, url: url, compare_host_keys: compare_host_keys) }
   describe '.primary_key' do
     it 'returns a symbol' do
@@ -191,5 +194,45 @@ RSpec.describe SshHostKey do
         is_expected.to eq(error: 'Failed to detect SSH host keys')
       end
     end
+
+    context 'DNS rebinding protection enabled' do
+      before do
+        stub_application_setting(dns_rebinding_protection_enabled: true)
+      end
+
+      it 'sends an address as well as hostname to ssh-keyscan' do
+        stub_dns(url, ip_address: '1.2.3.4')
+
+        stdin = stub_ssh_keyscan(%w[-T 5 -p 2222 -f-])
+
+        cache
+
+        expect(stdin.string).to eq("1.2.3.4 example.com\n")
+      end
+    end
+  end
+
+  describe 'URL validation' do
+    let(:url) { 'ssh://127.0.0.1' }
+
+    context 'when local requests are not allowed' do
+      before do
+        stub_application_setting(allow_local_requests_from_web_hooks_and_services: false)
+      end
+
+      it 'forbids scanning localhost' do
+        expect { ssh_host_key }.to raise_error(/Invalid URL/)
+      end
+    end
+
+    context 'when local requests are allowed' do
+      before do
+        stub_application_setting(allow_local_requests_from_web_hooks_and_services: true)
+      end
+
+      it 'permits scanning localhost' do
+        expect(ssh_host_key.url.to_s).to eq('ssh://127.0.0.1:22')
+      end
+    end
   end
 end