Merge remote-tracking branch 'dev/master'

CHANGELOG.md

@@ -2,6 +2,17 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 11.0.1 (2018-06-21)
+
+### Security (5 changes)
+
+- Fix XSS vulnerability for table of content generation.
+- Update sanitize gem to 4.6.5 to fix HTML injection vulnerability.
+- HTML escape branch name in project graphs page.
+- HTML escape the name of the user in ProjectsHelper#link_to_member.
+- Don't show events from internal projects for anonymous users in public feed.
+
+
 ## 11.0.0 (2018-06-22)
 
 ### Security (3 changes)
@@ -242,6 +253,17 @@ entry.
 - Workhorse to send raw diff and patch for commits.
 
 
+## 10.8.5 (2018-06-21)
+
+### Security (5 changes)
+
+- Fix XSS vulnerability for table of content generation.
+- Update sanitize gem to 4.6.5 to fix HTML injection vulnerability.
+- HTML escape branch name in project graphs page.
+- HTML escape the name of the user in ProjectsHelper#link_to_member.
+- Don't show events from internal projects for anonymous users in public feed.
+
+
 ## 10.8.4 (2018-06-06)
 
 - No changes.
@@ -460,6 +482,22 @@ entry.
 - Gitaly handles repository forks by default.
 
 
+## 10.7.6 (2018-06-21)
+
+### Security (6 changes)
+
+- Fix XSS vulnerability for table of content generation.
+- Update sanitize gem to 4.6.5 to fix HTML injection vulnerability.
+- HTML escape branch name in project graphs page.
+- HTML escape the name of the user in ProjectsHelper#link_to_member.
+- Don't show events from internal projects for anonymous users in public feed.
+- XSS fix to use safe_params instead of params in url_for helpers.
+
+### Other (1 change)
+
+- Replacing gollum libraries for gitlab custom libs. !18343
+
+
 ## 10.7.5 (2018-05-28)
 
 ### Security (3 changes)

Gemfile

@@ -230,7 +230,7 @@ gem 'ruby-fogbugz', '~> 0.2.1'
 gem 'kubeclient', '~> 3.1.0'
 
 # Sanitize user input
-gem 'sanitize', '~> 2.0'
+gem 'sanitize', '~> 4.6.5'
 gem 'babosa', '~> 1.0.2'
 
 # Sanitizes SVG input

Gemfile.lock

@@ -295,13 +295,13 @@ GEM
       flowdock (~> 0.7)
       gitlab-grit (>= 2.4.1)
       multi_json
-    gitlab-gollum-lib (4.2.7.4)
+    gitlab-gollum-lib (4.2.7.5)
       gemojione (~> 3.2)
       github-markup (~> 1.6)
       gollum-grit_adapter (~> 1.0)
       nokogiri (>= 1.6.1, < 2.0)
       rouge (~> 3.1)
-      sanitize (~> 2.1)
+      sanitize (~> 4.6.4)
       stringex (~> 2.6)
     gitlab-gollum-rugged_adapter (0.4.4.1)
       mime-types (>= 1.15)
@@ -514,6 +514,8 @@ GEM
     netrc (0.11.0)
     nokogiri (1.8.2)
       mini_portile2 (~> 2.3.0)
+    nokogumbo (1.5.0)
+      nokogiri
     numerizer (0.1.1)
     oauth (0.5.4)
     oauth2 (1.4.0)
@@ -804,8 +806,10 @@ GEM
       et-orbi (~> 1.0)
     rugged (0.27.2)
     safe_yaml (1.0.4)
-    sanitize (2.1.0)
+    sanitize (4.6.5)
+      crass (~> 1.0.2)
       nokogiri (>= 1.4.4)
+      nokogumbo (~> 1.4)
     sass (3.5.5)
       sass-listen (~> 4.0.0)
     sass-listen (4.0.0)
@@ -1151,7 +1155,7 @@ DEPENDENCIES
   ruby_parser (~> 3.8)
   rufus-scheduler (~> 3.4)
   rugged (~> 0.27)
-  sanitize (~> 2.0)
+  sanitize (~> 4.6.5)
   sass-rails (~> 5.0.6)
   scss_lint (~> 0.56.0)
   seed-fu (~> 2.3.7)

Gemfile.rails5.lock

@@ -298,13 +298,13 @@ GEM
       flowdock (~> 0.7)
       gitlab-grit (>= 2.4.1)
       multi_json
-    gitlab-gollum-lib (4.2.7.4)
+    gitlab-gollum-lib (4.2.7.5)
       gemojione (~> 3.2)
       github-markup (~> 1.6)
       gollum-grit_adapter (~> 1.0)
       nokogiri (>= 1.6.1, < 2.0)
       rouge (~> 3.1)
-      sanitize (~> 2.1)
+      sanitize (~> 4.6.4)
       stringex (~> 2.6)
     gitlab-gollum-rugged_adapter (0.4.4.1)
       mime-types (>= 1.15)
@@ -518,6 +518,8 @@ GEM
     nio4r (2.3.1)
     nokogiri (1.8.2)
       mini_portile2 (~> 2.3.0)
+    nokogumbo (1.5.0)
+      nokogiri
     numerizer (0.1.1)
     oauth (0.5.4)
     oauth2 (1.4.0)
@@ -813,8 +815,10 @@ GEM
       et-orbi (~> 1.0)
     rugged (0.27.1)
     safe_yaml (1.0.4)
-    sanitize (2.1.0)
+    sanitize (4.6.5)
+      crass (~> 1.0.2)
       nokogiri (>= 1.4.4)
+      nokogumbo (~> 1.4)
     sass (3.5.5)
       sass-listen (~> 4.0.0)
     sass-listen (4.0.0)
@@ -1162,7 +1166,7 @@ DEPENDENCIES
   ruby_parser (~> 3.8)
   rufus-scheduler (~> 3.4)
   rugged (~> 0.27)
-  sanitize (~> 2.0)
+  sanitize (~> 4.6.5)
   sass-rails (~> 5.0.6)
   scss_lint (~> 0.56.0)
   seed-fu (~> 2.3.7)

app/finders/user_recent_events_finder.rb

@@ -56,7 +56,7 @@ class UserRecentEventsFinder
 
     visible = target_user
       .project_interactions
-      .where(visibility_level: [Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PUBLIC])
+      .where(visibility_level: Gitlab::VisibilityLevel.levels_for_user(current_user))
       .select(:id)
 
     Gitlab::SQL::Union.new([authorized, visible]).to_sql

app/helpers/projects_helper.rb

@@ -40,7 +40,8 @@ module ProjectsHelper
       name_tag_options[:class] << 'has-tooltip'
     end
 
-    content_tag(:span, sanitize(username), name_tag_options)
+    # NOTE: ActionView::Helpers::TagHelper#content_tag HTML escapes username
+    content_tag(:span, username, name_tag_options)
   end
 
   def link_to_member(project, author, opts = {}, &block)

app/views/projects/graphs/charts.html.haml

@@ -30,7 +30,7 @@
           #{@commits_graph.start_date.strftime('%b %d')}
         - end_time = capture do
           #{@commits_graph.end_date.strftime('%b %d')}
-        = (_("Commit statistics for %{ref} %{start_time} - %{end_time}") % { ref: "<strong>#{@ref}</strong>", start_time: start_time, end_time: end_time }).html_safe
+        = (_("Commit statistics for %{ref} %{start_time} - %{end_time}") % { ref: "<strong>#{h @ref}</strong>", start_time: start_time, end_time: end_time }).html_safe
 
     .col-md-6
       .tree-ref-container

changelogs/unreleased/security-2682-fix-xss-for-markdown-toc.yml

+---
+title: Fix XSS vulnerability for table of content generation
+merge_request:
+author:
+type: security

changelogs/unreleased/security-fj-bumping-sanitize-gem.yml

+---
+title: Update sanitize gem to 4.6.5 to fix HTML injection vulnerability
+merge_request:
+author:
+type: security

changelogs/unreleased/security-html_escape_branch_name.yml

+---
+title: HTML escape branch name in project graphs page
+merge_request:
+author:
+type: security

changelogs/unreleased/security-html_escape_usernames.yml

+---
+title: HTML escape the name of the user in ProjectsHelper#link_to_member
+merge_request:
+author:
+type: security

changelogs/unreleased/security-rd-do-not-show-internal-info-in-public-feed.yml

+---
+title: Don't show events from internal projects for anonymous users in public feed
+merge_request:
+author:
+type: security

lib/banzai/filter/sanitization_filter.rb

@@ -25,10 +25,11 @@ module Banzai
         # Only push these customizations once
         return if customized?(whitelist[:transformers])
 
-        # Allow table alignment; we whitelist specific style properties in a
+        # Allow table alignment; we whitelist specific text-align values in a
         # transformer below
         whitelist[:attributes]['th'] = %w(style)
         whitelist[:attributes]['td'] = %w(style)
+        whitelist[:css] = { properties: ['text-align'] }
 
         # Allow span elements
         whitelist[:elements].push('span')

lib/banzai/filter/table_of_contents_filter.rb

@@ -92,7 +92,7 @@ module Banzai
         def text
           return '' unless node
 
-          @text ||= node.text
+          @text ||= EscapeUtils.escape_html(node.text)
         end
 
         private

spec/features/projects/graph_spec.rb

@@ -3,6 +3,7 @@ require 'spec_helper'
 describe 'Project Graph', :js do
   let(:user) { create :user }
   let(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:branch_name) { 'master' }
 
   before do
     project.add_master(user)
@@ -12,7 +13,7 @@ describe 'Project Graph', :js do
 
   shared_examples 'page should have commits graphs' do
     it 'renders commits' do
-      expect(page).to have_content('Commit statistics for master')
+      expect(page).to have_content("Commit statistics for #{branch_name}")
       expect(page).to have_content('Commits per day of month')
     end
   end
@@ -57,6 +58,23 @@ describe 'Project Graph', :js do
     it_behaves_like 'page should have languages graphs'
   end
 
+  context 'chart graph with HTML escaped branch name' do
+    let(:branch_name) { '<h1>evil</h1>' }
+
+    before do
+      project.repository.create_branch(branch_name, 'master')
+
+      visit charts_project_graph_path(project, branch_name)
+    end
+
+    it_behaves_like 'page should have commits graphs'
+
+    it 'HTML escapes branch name' do
+      expect(page.body).to include("Commit statistics for <strong>#{ERB::Util.html_escape(branch_name)}</strong>")
+      expect(page.body).not_to include(branch_name)
+    end
+  end
+
   context 'when CI enabled' do
     before do
       project.enable_ci

spec/finders/user_recent_events_finder_spec.rb

 require 'spec_helper'
 
 describe UserRecentEventsFinder do
-  let(:user) { create(:user) }
-  let(:project) { create(:project) }
-  let(:project_owner) { project.creator }
-  let!(:event) { create(:event, project: project, author: project_owner) }
+  let(:current_user)     { create(:user) }
+  let(:project_owner)    { create(:user) }
+  let(:private_project)  { create(:project, :private, creator: project_owner) }
+  let(:internal_project) { create(:project, :internal, creator: project_owner) }
+  let(:public_project)   { create(:project, :public, creator: project_owner) }
+  let!(:private_event)   { create(:event, project: private_project, author: project_owner) }
+  let!(:internal_event)  { create(:event, project: internal_project, author: project_owner) }
+  let!(:public_event)    { create(:event, project: public_project, author: project_owner) }
 
-  subject(:finder) { described_class.new(user, project_owner) }
+  subject(:finder) { described_class.new(current_user, project_owner) }
 
   describe '#execute' do
-    it 'does not include the event when a user does not have access to the project' do
-      expect(finder.execute).to be_empty
+    context 'current user does not have access to projects' do
+      it 'returns public and internal events' do
+        records = finder.execute
+
+        expect(records).to include(public_event, internal_event)
+        expect(records).not_to include(private_event)
+      end
     end
 
-    context 'when the user has access to a project' do
+    context 'when current user has access to the projects' do
       before do
-        project.add_developer(user)
+        private_project.add_developer(current_user)
+        internal_project.add_developer(current_user)
+        public_project.add_developer(current_user)
       end
 
-      it 'includes the event' do
-        expect(finder.execute).to include(event)
+      it 'returns all the events' do
+        expect(finder.execute).to include(private_event, internal_event, public_event)
       end
 
-      it 'does not include the event if the user cannot read cross project' do
-        expect(Ability).to receive(:allowed?).with(user, :read_cross_project) { false }
+      it 'does not include the events if the user cannot read cross project' do
+        expect(Ability).to receive(:allowed?).with(current_user, :read_cross_project) { false }
         expect(finder.execute).to be_empty
       end
     end
+
+    context 'when current user is anonymous' do
+      let(:current_user) { nil }
+
+      it 'returns public events only' do
+        expect(finder.execute).to eq([public_event])
+      end
+    end
   end
 end

spec/helpers/projects_helper_spec.rb

@@ -248,7 +248,7 @@ describe ProjectsHelper do
   describe '#link_to_member' do
     let(:group)   { build_stubbed(:group) }
     let(:project) { build_stubbed(:project, group: group) }
-    let(:user)    { build_stubbed(:user) }
+    let(:user)    { build_stubbed(:user, name: '<h1>Administrator</h1>') }
 
     describe 'using the default options' do
       it 'returns an HTML link to the user' do
@@ -256,6 +256,13 @@ describe ProjectsHelper do
 
         expect(link).to match(%r{/#{user.username}})
       end
+
+      it 'HTML escapes the name of the user' do
+        link = helper.link_to_member(project, user)
+
+        expect(link).to include(ERB::Util.html_escape(user.name))
+        expect(link).not_to include(user.name)
+      end
     end
   end
 

spec/lib/banzai/filter/sanitization_filter_spec.rb

@@ -93,6 +93,16 @@ describe Banzai::Filter::SanitizationFilter do
       expect(doc.at_css('td')['style']).to eq 'text-align: center'
     end
 
+    it 'disallows `text-align` property in `style` attribute on other elements' do
+      html = <<~HTML
+        <div style="text-align: center">Text</div>
+      HTML
+
+      doc = filter(html)
+
+      expect(doc.at_css('div')['style']).to be_nil
+    end
+
     it 'allows `span` elements' do
       exp = act = %q{<span>Hello</span>}
       expect(filter(act).to_html).to eq exp
@@ -224,7 +234,7 @@ describe Banzai::Filter::SanitizationFilter do
 
       'protocol-based JS injection: spaces and entities' => {
         input:  '<a href=" &#14;  javascript:alert(\'XSS\');">foo</a>',
-        output: '<a href="">foo</a>'
+        output: '<a href>foo</a>'
       },
 
       'protocol whitespace' => {

spec/lib/banzai/filter/table_of_contents_filter_spec.rb

@@ -139,5 +139,14 @@ describe Banzai::Filter::TableOfContentsFilter do
         expect(items[5].ancestors).to include(items[4])
       end
     end
+
+    context 'header text contains escaped content' do
+      let(:content) { '<img src="x" onerror="alert(42)">' }
+      let(:results) { result(header(1, content)) }
+
+      it 'outputs escaped content' do
+        expect(doc.inner_html).to include(content)
+      end
+    end
   end
 end