Commit a12d25d8 authored by Stan Hu's avatar Stan Hu

Validate Wiki attachments are valid temporary files

A malicious attacker could craft a request to read arbitrary files on
the system. This change adds a Grape validation to ensure that the
tempfile parameter delivered by the Rack multipart uploader is a
Tempfile type to prevent users from being able to specify arbitrary
filenames.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/53072
parent a310a638
---
title: Validate Wiki attachments are valid temporary files
merge_request:
author:
type: security
# frozen_string_literal: true
# This module overrides the Grape type validator defined in
# https://github.com/ruby-grape/grape/blob/master/lib/grape/validations/types/file.rb
module API
module Validations
module Types
class SafeFile < ::Grape::Validations::Types::File
def value_coerced?(value)
super && value[:tempfile].is_a?(Tempfile)
end
end
end
end
end
...@@ -6,7 +6,7 @@ module API ...@@ -6,7 +6,7 @@ module API
def commit_params(attrs) def commit_params(attrs)
{ {
file_name: attrs[:file][:filename], file_name: attrs[:file][:filename],
file_content: File.read(attrs[:file][:tempfile]), file_content: attrs[:file][:tempfile].read,
branch_name: attrs[:branch] branch_name: attrs[:branch]
} }
end end
...@@ -100,7 +100,7 @@ module API ...@@ -100,7 +100,7 @@ module API
success Entities::WikiAttachment success Entities::WikiAttachment
end end
params do params do
requires :file, type: File, desc: 'The attachment file to be uploaded' requires :file, type: ::API::Validations::Types::SafeFile, desc: 'The attachment file to be uploaded'
optional :branch, type: String, desc: 'The name of the branch' optional :branch, type: String, desc: 'The name of the branch'
end end
post ":id/wikis/attachments", requirements: API::PROJECT_ENDPOINT_REQUIREMENTS do post ":id/wikis/attachments", requirements: API::PROJECT_ENDPOINT_REQUIREMENTS do
......
...@@ -158,6 +158,16 @@ describe API::Wikis do ...@@ -158,6 +158,16 @@ describe API::Wikis do
expect(json_response.size).to eq(1) expect(json_response.size).to eq(1)
expect(json_response['error']).to eq('file is missing') expect(json_response['error']).to eq('file is missing')
end end
it 'responds with validation error on invalid temp file' do
payload[:file] = { tempfile: '/etc/hosts' }
post(api(url, user), payload)
expect(response).to have_gitlab_http_status(400)
expect(json_response.size).to eq(1)
expect(json_response['error']).to eq('file is invalid')
end
end end
describe 'GET /projects/:id/wikis' do describe 'GET /projects/:id/wikis' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment