Add latest changes from gitlab-org/security/gitlab@12-7-stable-ee

.gitlab/issue_templates/Security developer workflow.md

@@ -9,10 +9,11 @@ Set the title to: `Description of the original issue`
 ## Prior to starting the security release work
 - [ ] Read the [security process for developers] if you are not familiar with it.
-- [ ] Link this issue in the Security Release issue on GitLab.com. You can find this issue in the topic of the `#releases` channel.
-- [ ] Add a link to the confidential `gitlab-org/gitlab` issue describing the vulnerability next to **Original issue** in the [links table](#links).
-- [ ] Add a link to the confidential `gitlab-org/gitlab` Security release issue next to **Security release issue** in the [links table](#links).
+- [ ] Mark this [issue as related] to the Security Release tracking issue. You can find it on the topic of the `#releases` Slack channel.
 - [ ] Run `scripts/security-harness` in your local repository to prevent accidentally pushing to any remote besides `gitlab.com/gitlab-org/security`.
+- Fill out the [Links section](#links):
+  - [ ] Next to **Issue on GitLab**, add a link to the `gitlab-org/gitlab` issue that describes the security vulnerability.
+  - [ ] Next to **Security Release tracking issue**, add a link to the security release issue that will include this security issue.
 ## Development
@@ -29,7 +30,8 @@ After your merge request has being approved according to our [approval guideline
    * You can use the script `bin/secpick` instead of the following steps, to help you cherry-picking. See the [secpick documentation]
 - [ ] Create each MR targeting the stable branch `X-Y-stable`, using the [Security Release merge request template].
    * Every merge request will have its own set of TODOs, so make sure to complete those.
-- [ ] Make sure all MRs are linked in the [Links section](#links)
+- [ ] On the "Related merge requests" section, ensure all MRs are linked to this issue.
+   * This section should only list the merge requests created for this issue: One targeting `master` and the 3 backports.
 ## Documentation and final details
@@ -46,12 +48,8 @@ After your merge request has being approved according to our [approval guideline
 | Description | Link |
 | -------- | -------- |
-| Original issue   | #TODO  |
-| Security release issue   | #TODO  |
-| `master` MR | !TODO   |
-| `Backport X.Y` MR | !TODO   |
-| `Backport X.Y` MR | !TODO   |
-| `Backport X.Y` MR | !TODO   |
+| Issue on [GitLab](https://gitlab.com/gitlab-org/gitlab/issues) | #TODO  |
+| Security Release tracking issue | #TODO  |
 ### Details
@@ -68,5 +66,6 @@ After your merge request has being approved according to our [approval guideline
 [security Release merge request template]: https://gitlab.com/gitlab-org/security/gitlab/blob/master/.gitlab/merge_request_templates/Security%20Release.md
 [code review process]: https://docs.gitlab.com/ee/development/code_review.html
 [approval guidelines]: https://docs.gitlab.com/ee/development/code_review.html#approval-guidelines
+[issue as related]: https://docs.gitlab.com/ee/user/project/issues/related_issues.html#adding-a-related-issue
 /label ~security

.gitlab/merge_request_templates/Security Release.md

@@ -8,11 +8,11 @@ See [the general developer security release guidelines](https://gitlab.com/gitla
 ## Related issues
-<!-- Mention the issue(s) this MR is related to -->
+<!-- Mention the GitLab Security issue this MR is related to -->
 ## Developer checklist
-- [ ] Link this MR in the `links` section of the related issue on [GitLab Security].
+- [ ] **Make sure this merge request mentions the [GitLab Security] issue it belongs to (i.e. `Related to <issue_id>`).**
 - [ ] Merge request targets `master`, or `X-Y-stable` for backports.
 - [ ] Milestone is set for the version this merge request applies to. A closed milestone can be assigned via [quick actions].
 - [ ] Title of this merge request is the same as for all backports.

app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue

 <script>
 import { GlLoadingIcon } from '@gitlab/ui';
+import { escape } from 'lodash';
 import simplePoll from '../../../lib/utils/simple_poll';
 import eventHub from '../../event_hub';
 import statusIcon from '../mr_widget_status_icon.vue';
@@ -44,11 +45,10 @@ export default {
     fastForwardMergeText() {
       return sprintf(
         __(
-          `Fast-forward merge is not possible. Rebase the source branch onto %{startTag}${this.mr.targetBranch}%{endTag} to allow this merge request to be merged.`,
+          'Fast-forward merge is not possible. Rebase the source branch onto %{targetBranch} to allow this merge request to be merged.',
         ),
         {
-          startTag: '<span class="label-branch">',
-          endTag: '</span>',
+          targetBranch: `<span class="label-branch">${escape(this.mr.targetBranch)}</span>`,
         },
         false,
       );

app/models/badge.rb

@@ -32,7 +32,9 @@ class Badge < ApplicationRecord
   end
   def rendered_image_url(project = nil)
-    build_rendered_url(image_url, project)
+    Gitlab::AssetProxy.proxy_url(
+      build_rendered_url(image_url, project)
+    )
   end
   private

app/views/shared/issuable/form/_branch_chooser.html.haml

@@ -8,7 +8,9 @@
 .form-group.row.d-flex.gl-pl-3.gl-pr-3.branch-selector
   .align-self-center
-    %span= s_('From %{source_title} into').html_safe % { source_title: "<code>#{source_title}</code>".html_safe }
+    %span
+      = _('From <code>%{source_title}</code> into').html_safe % { source_title: source_title }
+
     - if issuable.new_record?
       %code= target_title
       &nbsp;

changelogs/unreleased/security-49-xss-branch-names.yml

+---
+title: Fix for XSS in branch names
+merge_request:
+author:
+type: security

changelogs/unreleased/security-badge-camo.yml

+---
+title: Run project badge images through the asset proxy
+merge_request:
+author:
+type: security

lib/gitlab/asset_proxy.rb

+# frozen_string_literal: true
+
+# This is based on https://github.com/jch/html-pipeline/blob/v2.12.2/lib/html/pipeline/camo_filter.rb
+# and Banzai::Filter::AssetProxyFilter which we use to proxy images in Markdown
+
+module Gitlab
+  module AssetProxy
+    class << self
+      def proxy_url(url)
+        return url unless Gitlab.config.asset_proxy.enabled
+        return url if asset_host_whitelisted?(url)
+
+        "#{Gitlab.config.asset_proxy.url}/#{asset_url_hash(url)}/#{hexencode(url)}"
+      end
+
+      private
+
+      def asset_host_whitelisted?(url)
+        parsed_url = URI.parse(url)
+
+        Gitlab.config.asset_proxy.domain_regexp&.match?(parsed_url.host)
+      end
+
+      def asset_url_hash(url)
+        OpenSSL::HMAC.hexdigest('sha1', Gitlab.config.asset_proxy.secret_key, url)
+      end
+
+      def hexencode(str)
+        str.unpack1('H*')
+      end
+    end
+  end
+end

locale/gitlab.pot

@@ -7882,6 +7882,9 @@ msgstr ""
 msgid "Failure"
 msgstr ""
+msgid "Fast-forward merge is not possible. Rebase the source branch onto %{targetBranch} to allow this merge request to be merged."
+msgstr ""
+
 msgid "Fast-forward merge is not possible. Rebase the source branch onto the target branch or merge target branch into source branch to allow this merge request to be merged."
 msgstr ""
@@ -8328,7 +8331,7 @@ msgstr ""
 msgid "From %{providerTitle}"
 msgstr ""
-msgid "From %{source_title} into"
+msgid "From <code>%{source_title}</code> into"
 msgstr ""
 msgid "From Bitbucket"

spec/features/merge_request/user_creates_merge_request_spec.rb

@@ -5,9 +5,9 @@ require "spec_helper"
 describe "User creates a merge request", :js do
   include ProjectForksHelper
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:user) { create(:user) }
   let(:title) { "Some feature" }
-  let(:project) { create(:project, :repository) }
-  let(:user) { create(:user) }
   before do
     project.add_maintainer(user)
@@ -38,6 +38,26 @@ describe "User creates a merge request", :js do
     end
   end
+  context "XSS branch name exists" do
+    before do
+      project.repository.create_branch("<img/src='x'/onerror=alert('oops')>", "master")
+    end
+
+    it "doesn't execute the dodgy branch name" do
+      visit(project_new_merge_request_path(project))
+
+      find(".js-source-branch").click
+      click_link("<img/src='x'/onerror=alert('oops')>")
+
+      find(".js-target-branch").click
+      click_link("feature")
+
+      click_button("Compare branches")
+
+      expect { page.driver.browser.switch_to.alert }.to raise_error(Selenium::WebDriver::Error::NoSuchAlertError)
+    end
+  end
+
   context "to a forked project" do
     let(:forked_project) { fork_project(project, user, namespace: user.namespace, repository: true) }

spec/lib/gitlab/asset_proxy_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::AssetProxy do
+  context 'when asset proxy is disabled' do
+    before do
+      stub_asset_proxy_setting(enabled: false)
+    end
+
+    it 'returns the original URL' do
+      url = 'http://example.com/test.png'
+
+      expect(described_class.proxy_url(url)).to eq(url)
+    end
+  end
+
+  context 'when asset proxy is enabled' do
+    before do
+      stub_asset_proxy_setting(whitelist: %w(gitlab.com *.mydomain.com))
+      stub_asset_proxy_setting(
+        enabled: true,
+        url: 'https://assets.example.com',
+        secret_key: 'shared-secret',
+        domain_regexp: Banzai::Filter::AssetProxyFilter.compile_whitelist(Gitlab.config.asset_proxy.whitelist)
+      )
+    end
+
+    it 'returns a proxied URL' do
+      url = 'http://example.com/test.png'
+      proxied_url = 'https://assets.example.com/08df250eeeef1a8cf2c761475ac74c5065105612/687474703a2f2f6578616d706c652e636f6d2f746573742e706e67'
+
+      expect(described_class.proxy_url(url)).to eq(proxied_url)
+    end
+
+    context 'whitelisted domain' do
+      it 'returns original URL for single domain whitelist' do
+        url = 'http://gitlab.com/test.png'
+
+        expect(described_class.proxy_url(url)).to eq(url)
+      end
+
+      it 'returns original URL for wildcard subdomain whitelist' do
+        url = 'http://test.mydomain.com/test.png'
+
+        expect(described_class.proxy_url(url)).to eq(url)
+      end
+    end
+  end
+end

spec/models/badge_spec.rb

@@ -91,6 +91,22 @@ describe Badge do
       let(:method) { :image_url }
       it_behaves_like 'rendered_links'
+
+      context 'when asset proxy is enabled' do
+        let(:placeholder_url) { 'http://www.example.com/image' }
+
+        before do
+          stub_asset_proxy_setting(
+            enabled: true,
+            url: 'https://assets.example.com',
+            secret_key: 'shared-secret'
+          )
+        end
+
+        it 'returns a proxied URL' do
+          expect(badge.rendered_image_url).to start_with('https://assets.example.com')
+        end
+      end
     end
   end
 end