Commit 43cc0d5a authored by James Lopez's avatar James Lopez

Fix persistent symlink in project import

- Fix permissions after untar is done
- Refactor command line util
parent 574ae4c7
---
title: Fix persistent symlink in project import
merge_request:
author:
type: security
...@@ -3,7 +3,8 @@ ...@@ -3,7 +3,8 @@
module Gitlab module Gitlab
module ImportExport module ImportExport
module CommandLineUtil module CommandLineUtil
DEFAULT_MODE = 0700 UNTAR_MASK = 'u+rwX,go+rX,go-w'
DEFAULT_DIR_MODE = 0700
def tar_czf(archive:, dir:) def tar_czf(archive:, dir:)
tar_with_options(archive: archive, dir: dir, options: 'czf') tar_with_options(archive: archive, dir: dir, options: 'czf')
...@@ -14,8 +15,8 @@ module Gitlab ...@@ -14,8 +15,8 @@ module Gitlab
end end
def mkdir_p(path) def mkdir_p(path)
FileUtils.mkdir_p(path, mode: DEFAULT_MODE) FileUtils.mkdir_p(path, mode: DEFAULT_DIR_MODE)
FileUtils.chmod(DEFAULT_MODE, path) FileUtils.chmod(DEFAULT_DIR_MODE, path)
end end
private private
...@@ -41,6 +42,7 @@ module Gitlab ...@@ -41,6 +42,7 @@ module Gitlab
def untar_with_options(archive:, dir:, options:) def untar_with_options(archive:, dir:, options:)
execute(%W(tar -#{options} #{archive} -C #{dir})) execute(%W(tar -#{options} #{archive} -C #{dir}))
execute(%W(chmod -R #{UNTAR_MASK} #{dir}))
end end
def execute(cmd) def execute(cmd)
......
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::ImportExport::CommandLineUtil do
include ExportFileHelper
let(:path) { "#{Dir.tmpdir}/symlink_test" }
let(:archive) { 'spec/fixtures/symlink_export.tar.gz' }
let(:shared) { Gitlab::ImportExport::Shared.new(nil) }
subject do
Class.new do
include Gitlab::ImportExport::CommandLineUtil
def initialize
@shared = Gitlab::ImportExport::Shared.new(nil)
end
end.new
end
before do
FileUtils.mkdir_p(path)
subject.untar_zxf(archive: archive, dir: path)
end
after do
FileUtils.rm_rf(path)
end
it 'has the right mask for project.json' do
expect(file_permissions("#{path}/project.json")).to eq(0755) # originally 777
end
it 'has the right mask for uploads' do
expect(file_permissions("#{path}/uploads")).to eq(0755) # originally 555
end
end
require 'spec_helper' require 'spec_helper'
describe Gitlab::ImportExport::FileImporter do describe Gitlab::ImportExport::FileImporter do
include ExportFileHelper
let(:shared) { Gitlab::ImportExport::Shared.new(nil) } let(:shared) { Gitlab::ImportExport::Shared.new(nil) }
let(:storage_path) { "#{Dir.tmpdir}/file_importer_spec" } let(:storage_path) { "#{Dir.tmpdir}/file_importer_spec" }
let(:valid_file) { "#{shared.export_path}/valid.json" } let(:valid_file) { "#{shared.export_path}/valid.json" }
...@@ -8,6 +10,7 @@ describe Gitlab::ImportExport::FileImporter do ...@@ -8,6 +10,7 @@ describe Gitlab::ImportExport::FileImporter do
let(:hidden_symlink_file) { "#{shared.export_path}/.hidden" } let(:hidden_symlink_file) { "#{shared.export_path}/.hidden" }
let(:subfolder_symlink_file) { "#{shared.export_path}/subfolder/invalid.json" } let(:subfolder_symlink_file) { "#{shared.export_path}/subfolder/invalid.json" }
let(:evil_symlink_file) { "#{shared.export_path}/.\nevil" } let(:evil_symlink_file) { "#{shared.export_path}/.\nevil" }
let(:custom_mode_symlink_file) { "#{shared.export_path}/symlink.mode" }
before do before do
stub_const('Gitlab::ImportExport::FileImporter::MAX_RETRIES', 0) stub_const('Gitlab::ImportExport::FileImporter::MAX_RETRIES', 0)
...@@ -45,10 +48,18 @@ describe Gitlab::ImportExport::FileImporter do ...@@ -45,10 +48,18 @@ describe Gitlab::ImportExport::FileImporter do
expect(File.exist?(subfolder_symlink_file)).to be false expect(File.exist?(subfolder_symlink_file)).to be false
end end
it 'removes symlinks without any file permissions' do
expect(File.exist?(custom_mode_symlink_file)).to be false
end
it 'does not remove a valid file' do it 'does not remove a valid file' do
expect(File.exist?(valid_file)).to be true expect(File.exist?(valid_file)).to be true
end end
it 'does not change a valid file permissions' do
expect(file_permissions(valid_file)).not_to eq(0000)
end
it 'creates the file in the right subfolder' do it 'creates the file in the right subfolder' do
expect(shared.export_path).to include('test/abcd') expect(shared.export_path).to include('test/abcd')
end end
...@@ -84,5 +95,7 @@ describe Gitlab::ImportExport::FileImporter do ...@@ -84,5 +95,7 @@ describe Gitlab::ImportExport::FileImporter do
FileUtils.ln_s(valid_file, subfolder_symlink_file) FileUtils.ln_s(valid_file, subfolder_symlink_file)
FileUtils.ln_s(valid_file, hidden_symlink_file) FileUtils.ln_s(valid_file, hidden_symlink_file)
FileUtils.ln_s(valid_file, evil_symlink_file) FileUtils.ln_s(valid_file, evil_symlink_file)
FileUtils.ln_s(valid_file, custom_mode_symlink_file)
FileUtils.chmod_R(0000, custom_mode_symlink_file)
end end
end end
...@@ -133,6 +133,6 @@ module ExportFileHelper ...@@ -133,6 +133,6 @@ module ExportFileHelper
end end
def file_permissions(file) def file_permissions(file)
File.stat(file).mode & 0777 File.lstat(file).mode & 0777
end end
end end
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