diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-02-28 19:59:18 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-02-28 19:59:18 +0000 |
commit | 15322f219a99c7056ad77400559fc72e833607e9 (patch) | |
tree | 6f7f9fd0ce4c51d205139ccb88504b3c75c32633 | |
parent | b7d29500f28ff59c8898cdf889a40d3da908f162 (diff) | |
download | gitlab-ce-15322f219a99c7056ad77400559fc72e833607e9.tar.gz |
Add latest changes from gitlab-org/security/gitlab@12-7-stable-ee
-rw-r--r-- | .gitlab/issue_templates/Security developer workflow.md | 19 | ||||
-rw-r--r-- | .gitlab/merge_request_templates/Security Release.md | 4 | ||||
-rw-r--r-- | app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue | 6 | ||||
-rw-r--r-- | app/models/badge.rb | 4 | ||||
-rw-r--r-- | app/views/shared/issuable/form/_branch_chooser.html.haml | 4 | ||||
-rw-r--r-- | changelogs/unreleased/security-49-xss-branch-names.yml | 5 | ||||
-rw-r--r-- | changelogs/unreleased/security-badge-camo.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/asset_proxy.rb | 33 | ||||
-rw-r--r-- | locale/gitlab.pot | 5 | ||||
-rw-r--r-- | spec/features/merge_request/user_creates_merge_request_spec.rb | 24 | ||||
-rw-r--r-- | spec/lib/gitlab/asset_proxy_spec.rb | 50 | ||||
-rw-r--r-- | spec/models/badge_spec.rb | 16 | ||||
-rw-r--r--[-rwxr-xr-x] | vendor/gitignore/C++.gitignore | 0 | ||||
-rw-r--r--[-rwxr-xr-x] | vendor/gitignore/Java.gitignore | 0 |
14 files changed, 155 insertions, 20 deletions
diff --git a/.gitlab/issue_templates/Security developer workflow.md b/.gitlab/issue_templates/Security developer workflow.md index 1b6a1f87216..56be0453b33 100644 --- a/.gitlab/issue_templates/Security developer workflow.md +++ b/.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 diff --git a/.gitlab/merge_request_templates/Security Release.md b/.gitlab/merge_request_templates/Security Release.md index cccfafe397e..02cb4c59fd1 100644 --- a/.gitlab/merge_request_templates/Security Release.md +++ b/.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. diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue index 57be97855e3..b1fb377e47a 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue @@ -1,5 +1,6 @@ <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, ); diff --git a/app/models/badge.rb b/app/models/badge.rb index eb351425e66..3400d6d407d 100644 --- a/app/models/badge.rb +++ b/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 diff --git a/app/views/shared/issuable/form/_branch_chooser.html.haml b/app/views/shared/issuable/form/_branch_chooser.html.haml index 29ac17c43b9..8d9e5ddf065 100644 --- a/app/views/shared/issuable/form/_branch_chooser.html.haml +++ b/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 diff --git a/changelogs/unreleased/security-49-xss-branch-names.yml b/changelogs/unreleased/security-49-xss-branch-names.yml new file mode 100644 index 00000000000..d6ad72aa622 --- /dev/null +++ b/changelogs/unreleased/security-49-xss-branch-names.yml @@ -0,0 +1,5 @@ +--- +title: Fix for XSS in branch names +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-badge-camo.yml b/changelogs/unreleased/security-badge-camo.yml new file mode 100644 index 00000000000..b882bffdcaa --- /dev/null +++ b/changelogs/unreleased/security-badge-camo.yml @@ -0,0 +1,5 @@ +--- +title: Run project badge images through the asset proxy +merge_request: +author: +type: security diff --git a/lib/gitlab/asset_proxy.rb b/lib/gitlab/asset_proxy.rb new file mode 100644 index 00000000000..fd7c58ba68f --- /dev/null +++ b/lib/gitlab/asset_proxy.rb @@ -0,0 +1,33 @@ +# 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 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index af51d4af448..5183f129a76 100644 --- a/locale/gitlab.pot +++ b/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" diff --git a/spec/features/merge_request/user_creates_merge_request_spec.rb b/spec/features/merge_request/user_creates_merge_request_spec.rb index 67f6d8ebe32..86ee9fa5aa5 100644 --- a/spec/features/merge_request/user_creates_merge_request_spec.rb +++ b/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) } diff --git a/spec/lib/gitlab/asset_proxy_spec.rb b/spec/lib/gitlab/asset_proxy_spec.rb new file mode 100644 index 00000000000..f5aa1819982 --- /dev/null +++ b/spec/lib/gitlab/asset_proxy_spec.rb @@ -0,0 +1,50 @@ +# 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 diff --git a/spec/models/badge_spec.rb b/spec/models/badge_spec.rb index c661f5384ea..ad40cfad9a2 100644 --- a/spec/models/badge_spec.rb +++ b/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 diff --git a/vendor/gitignore/C++.gitignore b/vendor/gitignore/C++.gitignore index 259148fa18f..259148fa18f 100755..100644 --- a/vendor/gitignore/C++.gitignore +++ b/vendor/gitignore/C++.gitignore diff --git a/vendor/gitignore/Java.gitignore b/vendor/gitignore/Java.gitignore index a1c2a238a96..a1c2a238a96 100755..100644 --- a/vendor/gitignore/Java.gitignore +++ b/vendor/gitignore/Java.gitignore |