Merge branch 'security-wiki-svg-attachment' into 'security-11-4'

[11.4] Stored XSS in latest IE

See merge request gitlab/gitlabhq!2674

app/controllers/concerns/sends_blob.rb

@@ -8,7 +8,7 @@ module SendsBlob
     include SendFileUpload
   end
-  def send_blob(blob, params = {})
+  def send_blob(repository, blob, params = {})
     if blob
       headers['X-Content-Type-Options'] = 'nosniff'

app/controllers/projects/avatars_controller.rb

@@ -8,7 +8,7 @@ class Projects::AvatarsController < Projects::ApplicationController
   def show
     @blob = @repository.blob_at_branch(@repository.root_ref, @project.avatar_in_git)
-    send_blob(@blob)
+    send_blob(@repository, @blob)
   end
   def destroy

app/controllers/projects/raw_controller.rb

@@ -12,6 +12,6 @@ class Projects::RawController < Projects::ApplicationController
   def show
     @blob = @repository.blob_at(@commit.id, @path)
-    send_blob(@blob, inline: (params[:inline] != 'false'))
+    send_blob(@repository, @blob, inline: (params[:inline] != 'false'))
   end
 end

app/controllers/projects/wikis_controller.rb

@@ -2,6 +2,7 @@
 class Projects::WikisController < Projects::ApplicationController
   include PreviewMarkdown
+  include SendsBlob
   include Gitlab::Utils::StrongMemoize
   before_action :authorize_read_wiki!
@@ -26,16 +27,8 @@ class Projects::WikisController < Projects::ApplicationController
       set_encoding_error unless valid_encoding?
       render 'show'
-    elsif file = @project_wiki.find_file(params[:id], params[:version_id])
-      response.headers['Content-Security-Policy'] = "default-src 'none'"
-      response.headers['X-Content-Security-Policy'] = "default-src 'none'"
-
-      send_data(
-        file.raw_data,
-        type: file.mime_type,
-        disposition: 'inline',
-        filename: file.name
-      )
+    elsif file_blob
+      send_blob(@project_wiki.repository, file_blob)
     elsif can?(current_user, :create_wiki, @project) && view_param == 'create'
       @page = build_page(title: params[:id])
@@ -164,4 +157,14 @@ class Projects::WikisController < Projects::ApplicationController
   def set_encoding_error
     flash.now[:notice] = "The content of this page is not encoded in UTF-8. Edits can only be made via the Git repository."
   end
+
+  def file_blob
+    strong_memoize(:file_blob) do
+      commit = @project_wiki.repository.commit(@project_wiki.default_branch)
+
+      next unless commit
+
+      @project_wiki.repository.blob_at(commit.id, params[:id])
+    end
+  end
 end

app/helpers/blob_helper.rb

@@ -150,7 +150,9 @@ module BlobHelper
   # example of Javascript) we tell the browser of the victim not to
   # execute untrusted data.
   def safe_content_type(blob)
-    if blob.text?
+    if blob.extension == 'svg'
+      blob.mime_type
+    elsif blob.text?
       'text/plain; charset=utf-8'
     elsif blob.image?
       blob.content_type
@@ -159,6 +161,12 @@ module BlobHelper
     end
   end
+  def content_disposition(blob, inline)
+    return 'attachment' if blob.extension == 'svg'
+
+    inline ? 'inline' : 'attachment'
+  end
+
   def ref_project
     @ref_project ||= @target_project || @project
   end

app/helpers/workhorse_helper.rb

@@ -6,7 +6,7 @@ module WorkhorseHelper
   # Send a Git blob through Workhorse
   def send_git_blob(repository, blob, inline: true)
     headers.store(*Gitlab::Workhorse.send_git_blob(repository, blob))
-    headers['Content-Disposition'] = inline ? 'inline' : 'attachment'
+    headers['Content-Disposition'] = content_disposition(blob, inline)
     headers['Content-Type'] = safe_content_type(blob)
     render plain: ""
   end

changelogs/unreleased/security-wiki-svg-attachment.yml

+---
+title: Setting svg disposition as attachment in wikis
+merge_request:
+author:
+type: security

spec/controllers/projects/wikis_controller_spec.rb

@@ -22,20 +22,18 @@ describe Projects::WikisController do
     subject { get :show, namespace_id: project.namespace, project_id: project, id: wiki_title }
-    context 'when page content encoding is invalid' do
-      it 'limits the retrieved pages for the sidebar' do
-        expect(controller).to receive(:load_wiki).and_return(project_wiki)
-        # empty? call
-        expect(project_wiki).to receive(:pages).with(limit: 1).and_call_original
-        # Sidebar entries
-        expect(project_wiki).to receive(:pages).with(limit: 15).and_call_original
-        subject
-        expect(response).to have_http_status(:ok)
-        expect(response.body).to include(wiki_title)
-      end
+    it 'limits the retrieved pages for the sidebar' do
+      expect(controller).to receive(:load_wiki).and_return(project_wiki)
+      # empty? call
+      expect(project_wiki).to receive(:pages).with(limit: 1).and_call_original
+      # Sidebar entries
+      expect(project_wiki).to receive(:pages).with(limit: 15).and_call_original
+      subject
+      expect(response).to have_http_status(:ok)
+      expect(response.body).to include(wiki_title)
     end
     context 'when page content encoding is invalid' do
@@ -48,6 +46,42 @@ describe Projects::WikisController do
         expect(flash[:notice]).to eq 'The content of this page is not encoded in UTF-8. Edits can only be made via the Git repository.'
       end
     end
+
+    context 'when page is a file' do
+      include WikiHelpers
+
+      let(:path) { upload_file_to_wiki(project, user, file_name) }
+
+      before do
+        subject
+      end
+
+      subject { get :show, namespace_id: project.namespace, project_id: project, id: path }
+
+      context 'when file is an image' do
+        let(:file_name) { 'dk.png' }
+
+        it 'renders the content inline' do
+          expect(response.headers['Content-Disposition']).to match(/^inline/)
+        end
+
+        context 'when file is a svg' do
+          let(:file_name) { 'unsanitized.svg' }
+
+          it 'renders the content as an attachment' do
+            expect(response.headers['Content-Disposition']).to match(/^attachment/)
+          end
+        end
+      end
+
+      context 'when file is a pdf' do
+        let(:file_name) { 'git-cheat-sheet.pdf' }
+
+        it 'sets the content type to application/octet-stream' do
+          expect(response.headers['Content-Type']).to eq 'application/octet-stream'
+        end
+      end
+    end
   end
   describe 'POST #preview_markdown' do

spec/features/projects/wiki/user_views_wiki_empty_spec.rb

@@ -54,7 +54,7 @@ describe 'User views empty wiki' do
     it_behaves_like 'empty wiki and non-accessible issues'
   end
-  context 'when user is logged in and a memeber' do
+  context 'when user is logged in and a member' do
     let(:project) { create(:project, :public, :wiki_repo) }
     before do

spec/features/projects/wiki/user_views_wiki_page_spec.rb

@@ -2,12 +2,15 @@ require 'spec_helper'
 describe 'User views a wiki page' do
   shared_examples 'wiki page user view' do
+    include WikiHelpers
+
     let(:user) { create(:user) }
     let(:project) { create(:project, :wiki_repo, namespace: user.namespace) }
+    let(:path) { 'image.png' }
     let(:wiki_page) do
       create(:wiki_page,
         wiki: project.wiki,
-        attrs: { title: 'home', content: 'Look at this [image](image.jpg)\n\n ![alt text](image.jpg)' })
+        attrs: { title: 'home', content: "Look at this [image](#{path})\n\n ![alt text](#{path})" })
     end
     before do
@@ -82,33 +85,26 @@ describe 'User views a wiki page' do
         expect(find('.wiki-pages')).to have_content(wiki_page.title.capitalize)
       end
-      it 'shows a file stored in a page' do
-        raw_file = Gitlab::GitalyClient::WikiFile.new(
-          mime_type: 'image/jpeg',
-          name: 'images/image.jpg',
-          path: 'images/image.jpg',
-          raw_data: ''
-        )
-        wiki_file = Gitlab::Git::WikiFile.new(raw_file)
-
-        allow(wiki_file).to receive(:mime_type).and_return('image/jpeg')
-        allow_any_instance_of(ProjectWiki).to receive(:find_file).with('image.jpg', nil).and_return(wiki_file)
-        expect(page).to have_xpath("//img[@data-src='#{project.wiki.wiki_base_path}/image.jpg']")
-        expect(page).to have_link('image', href: "#{project.wiki.wiki_base_path}/image.jpg")
-        click_on('image')
-        expect(current_path).to match('wikis/image.jpg')
-        expect(page).not_to have_xpath('/html') # Page should render the image which means there is no html involved
+      context 'shows a file stored in a page' do
+        let(:path) { upload_file_to_wiki(project, user, 'dk.png') }
+        it do
+          expect(page).to have_xpath("//img[@data-src='#{project.wiki.wiki_base_path}/#{path}']")
+          expect(page).to have_link('image', href: "#{project.wiki.wiki_base_path}/#{path}")
+          click_on('image')
+          expect(current_path).to match("wikis/#{path}")
+          expect(page).not_to have_xpath('/html') # Page should render the image which means there is no html involved
+        end
       end
       it 'shows the creation page if file does not exist' do
-        expect(page).to have_link('image', href: "#{project.wiki.wiki_base_path}/image.jpg")
+        expect(page).to have_link('image', href: "#{project.wiki.wiki_base_path}/#{path}")
         click_on('image')
-        expect(current_path).to match('wikis/image.jpg')
+        expect(current_path).to match("wikis/#{path}")
         expect(page).to have_content('New Wiki Page')
         expect(page).to have_content('Create page')
       end

spec/support/helpers/wiki_helpers.rb

+module WikiHelpers
+  extend self
+
+  def upload_file_to_wiki(project, user, file_name)
+    opts = {
+      file_name: file_name,
+      file_content: File.read(expand_fixture_path(file_name))
+     }
+
+    ::Wikis::CreateAttachmentService.new(project, user, opts)
+                                    .execute[:result][:file_path]
+  end
+end