Skip to content
Snippets Groups Projects
Commit adaefba2 authored by GitLab Release Tools Bot's avatar GitLab Release Tools Bot
Browse files

Merge branch 'security-fix-dynamic-child-pipeline-zip-extraction' into 'master'

parents ed95f1ff d1f52556
No related branches found
No related tags found
No related merge requests found
......@@ -9,6 +9,7 @@ class ArtifactFileReader
Error = Class.new(StandardError)
MAX_ARCHIVE_SIZE = 5.megabytes
TMP_ARTIFACT_EXTRACTION_DIR = "extracted_artifacts_job_%{id}"
def initialize(job)
@job = job
......@@ -45,20 +46,20 @@ def validate!
end
def read_zip_file!(file_path)
job.artifacts_file.use_open_file do |file|
zip_file = Zip::File.new(file, false, true)
entry = zip_file.find_entry(file_path)
dir_name = format(TMP_ARTIFACT_EXTRACTION_DIR, id: job.id.to_i)
unless entry
raise Error, "Path `#{file_path}` does not exist inside the `#{job.name}` artifacts archive!"
job.artifacts_file.use_open_file(unlink_early: false) do |tmp_open_file|
Dir.mktmpdir(dir_name) do |tmp_dir|
SafeZip::Extract.new(tmp_open_file.file_path).extract(files: [file_path], to: tmp_dir)
File.read(File.join(tmp_dir, file_path))
end
if entry.name_is_directory?
raise Error, "Path `#{file_path}` was expected to be a file but it was a directory!"
end
zip_file.read(entry)
end
rescue SafeZip::Extract::NoMatchingError
raise Error, "Path `#{file_path}` does not exist inside the `#{job.name}` artifacts archive!"
rescue SafeZip::Extract::EntrySizeError
raise Error, "Path `#{file_path}` has invalid size in the zip!"
rescue Errno::EISDIR
raise Error, "Path `#{file_path}` was expected to be a file but it was a directory!"
end
def max_archive_size_in_mb
......
......@@ -25,8 +25,8 @@ def exist?
end
def extract
# do not extract if file is not part of target directory
return false unless matching_target_directory
# do not extract if file is not part of target directory or target file
return false unless matching_target_directory || matching_target_file
# do not overwrite existing file
raise SafeZip::Extract::AlreadyExistsError, "File already exists #{zip_entry.name}" if exist?
......@@ -44,6 +44,8 @@ def extract
end
rescue SafeZip::Extract::Error
raise
rescue Zip::EntrySizeError => e
raise SafeZip::Extract::EntrySizeError, e.message
rescue StandardError => e
raise SafeZip::Extract::ExtractError, e.message
end
......@@ -84,6 +86,10 @@ def matching_target_directory
params.matching_target_directory(path)
end
def matching_target_file
params.matching_target_file(path)
end
def read_symlink
zip_archive.read(zip_entry)
end
......
......@@ -6,6 +6,7 @@ class Extract
PermissionDeniedError = Class.new(Error)
SymlinkSourceDoesNotExistError = Class.new(Error)
UnsupportedEntryError = Class.new(Error)
EntrySizeError = Class.new(Error)
AlreadyExistsError = Class.new(Error)
NoMatchingError = Class.new(Error)
ExtractError = Class.new(Error)
......
......@@ -4,11 +4,13 @@ module SafeZip
class ExtractParams
include Gitlab::Utils::StrongMemoize
attr_reader :directories, :extract_path
attr_reader :directories, :files, :extract_path
def initialize(directories:, to:)
def initialize(to:, directories: [], files: [])
@directories = directories
@files = files
@extract_path = ::File.realpath(to)
validate!
end
def matching_target_directory(path)
......@@ -32,5 +34,23 @@ def directories_wildcard
end
end
end
def matching_target_file(path)
target_files.include?(path)
end
private
def target_files
strong_memoize(:target_files) do
files.map do |file|
::File.join(extract_path, file)
end
end
end
def validate!
raise ArgumentError, 'Either directories or files are required' if directories.empty? && files.empty?
end
end
end
File added
No preview for this file type
......@@ -10,71 +10,117 @@
subject { described_class.new(job).read(path) }
context 'when job has artifacts and metadata' do
let!(:artifacts) { create(:ci_job_artifact, :archive, job: job) }
let!(:metadata) { create(:ci_job_artifact, :metadata, job: job) }
shared_examples 'extracting job artifact archive' do
it 'returns the content at the path' do
is_expected.to be_present
expect(YAML.safe_load(subject).keys).to contain_exactly('rspec', 'time', 'custom')
end
it 'returns the content at the path' do
is_expected.to be_present
expect(YAML.safe_load(subject).keys).to contain_exactly('rspec', 'time', 'custom')
end
context 'when path does not exist' do
let(:path) { 'file/does/not/exist.txt' }
let(:expected_error) do
"Path `#{path}` does not exist inside the `#{job.name}` artifacts archive!"
end
context 'when path does not exist' do
let(:path) { 'file/does/not/exist.txt' }
let(:expected_error) do
"Path `#{path}` does not exist inside the `#{job.name}` artifacts archive!"
it 'raises an error' do
expect { subject }.to raise_error(described_class::Error, expected_error)
end
end
it 'raises an error' do
expect { subject }.to raise_error(described_class::Error, expected_error)
context 'when path points to a directory' do
let(:path) { 'other_artifacts_0.1.2' }
let(:expected_error) do
"Path `#{path}` was expected to be a file but it was a directory!"
end
it 'raises an error' do
expect { subject }.to raise_error(described_class::Error, expected_error)
end
end
end
context 'when path points to a directory' do
let(:path) { 'other_artifacts_0.1.2' }
let(:expected_error) do
"Path `#{path}` was expected to be a file but it was a directory!"
context 'when path is nested' do
# path exists in ci_build_artifacts.zip
let(:path) { 'other_artifacts_0.1.2/doc_sample.txt' }
it 'returns the content at the nested path' do
is_expected.to be_present
end
end
it 'raises an error' do
expect { subject }.to raise_error(described_class::Error, expected_error)
context 'when artifact archive size is greater than the limit' do
let(:expected_error) do
"Artifacts archive for job `#{job.name}` is too large: max 1 KB"
end
before do
stub_const("#{described_class}::MAX_ARCHIVE_SIZE", 1.kilobyte)
end
it 'raises an error' do
expect { subject }.to raise_error(described_class::Error, expected_error)
end
end
end
context 'when path is nested' do
# path exists in ci_build_artifacts.zip
let(:path) { 'other_artifacts_0.1.2/doc_sample.txt' }
context 'when metadata entry shows size greater than the limit' do
let(:expected_error) do
"Artifacts archive for job `#{job.name}` is too large: max 5 MB"
end
it 'returns the content at the nested path' do
is_expected.to be_present
before do
expect_next_instance_of(Gitlab::Ci::Build::Artifacts::Metadata::Entry) do |entry|
expect(entry).to receive(:total_size).and_return(10.megabytes)
end
end
it 'raises an error' do
expect { subject }.to raise_error(described_class::Error, expected_error)
end
end
end
context 'when artifact archive size is greater than the limit' do
let(:expected_error) do
"Artifacts archive for job `#{job.name}` is too large: max 1 KB"
end
context 'when job artifact is on local storage' do
let!(:artifacts) { create(:ci_job_artifact, :archive, job: job) }
let!(:metadata) { create(:ci_job_artifact, :metadata, job: job) }
it_behaves_like 'extracting job artifact archive'
end
context 'when job artifact is on remote storage' do
before do
stub_const("#{described_class}::MAX_ARCHIVE_SIZE", 1.kilobyte)
stub_artifacts_object_storage
stub_request(:get, %r{https://artifacts.+ci_build_artifacts\.zip})
.to_return(
status: 200,
body: File.open(Rails.root.join('spec/fixtures/ci_build_artifacts.zip')),
headers: {}
)
stub_request(:get, %r{https://artifacts.+ci_build_artifacts_metadata})
.to_return(
status: 200,
body: File.open(Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz')),
headers: {}
)
end
it 'raises an error' do
expect { subject }.to raise_error(described_class::Error, expected_error)
end
let!(:artifacts) { create(:ci_job_artifact, :archive, :remote_store, job: job) }
let!(:metadata) { create(:ci_job_artifact, :metadata, :remote_store, job: job) }
it_behaves_like 'extracting job artifact archive'
end
context 'when metadata entry shows size greater than the limit' do
let(:expected_error) do
"Artifacts archive for job `#{job.name}` is too large: max 5 MB"
end
context 'when extracting job artifact raises entry size error' do
let!(:artifacts) { create(:ci_job_artifact, :archive, job: job) }
let!(:metadata) { create(:ci_job_artifact, :metadata, job: job) }
before do
expect_next_instance_of(Gitlab::Ci::Build::Artifacts::Metadata::Entry) do |entry|
expect(entry).to receive(:total_size).and_return(10.megabytes)
allow_next_instance_of(SafeZip::Extract, anything) do |extractor|
allow(extractor).to receive(:extract).and_raise(SafeZip::Extract::EntrySizeError)
end
end
it 'raises an error' do
expected_error = "Path `#{path}` has invalid size in the zip!"
expect { subject }.to raise_error(described_class::Error, expected_error)
end
end
......
......@@ -5,12 +5,13 @@
RSpec.describe SafeZip::Entry do
let(:target_path) { Dir.mktmpdir('safe-zip') }
let(:directories) { %w(public folder/with/subfolder) }
let(:params) { SafeZip::ExtractParams.new(directories: directories, to: target_path) }
let(:files) { %w(public/index.html public/assets/image.png) }
let(:params) { SafeZip::ExtractParams.new(directories: directories, files: files, to: target_path) }
let(:entry) { described_class.new(zip_archive, zip_entry, params) }
let(:entry_name) { 'public/folder/index.html' }
let(:entry_path_dir) { File.join(target_path, File.dirname(entry_name)) }
let(:entry_path) { File.join(target_path, entry_name) }
let(:entry_path) { File.join(File.realpath(target_path), entry_name) }
let(:zip_archive) { double }
let(:zip_entry) do
......@@ -28,7 +29,7 @@
describe '#path_dir' do
subject { entry.path_dir }
it { is_expected.to eq(target_path + '/public/folder') }
it { is_expected.to eq(File.realpath(target_path) + '/public/folder') }
end
describe '#exist?' do
......@@ -51,6 +52,9 @@
subject { entry.extract }
context 'when entry does not match the filtered directories' do
let(:directories) { %w(public folder/with/subfolder) }
let(:files) { [] }
using RSpec::Parameterized::TableSyntax
where(:entry_name) do
......@@ -70,7 +74,30 @@
end
end
context 'when entry does exist' do
context 'when entry does not match the filtered files' do
let(:directories) { [] }
let(:files) { %w(public/index.html public/assets/image.png) }
using RSpec::Parameterized::TableSyntax
where(:entry_name) do
[
'assets/folder/index.html',
'public/../folder/index.html',
'public/../../../../../index.html',
'../../../../../public/index.html',
'/etc/passwd'
]
end
with_them do
it 'does not extract file' do
is_expected.to be_falsey
end
end
end
context 'when there is an existing extracted entry' do
before do
create_entry
end
......
......@@ -4,8 +4,10 @@
RSpec.describe SafeZip::ExtractParams do
let(:target_path) { Dir.mktmpdir("safe-zip") }
let(:params) { described_class.new(directories: directories, to: target_path) }
let(:real_target_path) { File.realpath(target_path) }
let(:params) { described_class.new(directories: directories, files: files, to: target_path) }
let(:directories) { %w(public folder/with/subfolder) }
let(:files) { %w(public/index.html public/assets/image.png) }
after do
FileUtils.remove_entry_secure(target_path)
......@@ -14,13 +16,13 @@
describe '#extract_path' do
subject { params.extract_path }
it { is_expected.to eq(target_path) }
it { is_expected.to eq(real_target_path) }
end
describe '#matching_target_directory' do
using RSpec::Parameterized::TableSyntax
subject { params.matching_target_directory(target_path + path) }
subject { params.matching_target_directory(real_target_path + path) }
where(:path, :result) do
'/public/index.html' | '/public/'
......@@ -30,7 +32,7 @@
end
with_them do
it { is_expected.to eq(result ? target_path + result : nil) }
it { is_expected.to eq(result ? real_target_path + result : nil) }
end
end
......@@ -38,7 +40,7 @@
subject { params.target_directories }
it 'starts with target_path' do
is_expected.to all(start_with(target_path + '/'))
is_expected.to all(start_with(real_target_path + '/'))
end
it 'ends with / for all paths' do
......@@ -53,4 +55,27 @@
is_expected.to all(end_with('/*'))
end
end
describe '#matching_target_file' do
using RSpec::Parameterized::TableSyntax
subject { params.matching_target_file(real_target_path + path) }
where(:path, :result) do
'/public/index.html' | true
'/non/existing/path' | false
'/public/' | false
'/folder/with/index.html' | false
end
with_them do
it { is_expected.to eq(result) }
end
end
context 'when directories and files are empty' do
it 'is invalid' do
expect { described_class.new(to: target_path) }.to raise_error(ArgumentError, /directories or files are required/)
end
end
end
......@@ -5,6 +5,7 @@
RSpec.describe SafeZip::Extract do
let(:target_path) { Dir.mktmpdir('safe-zip') }
let(:directories) { %w(public) }
let(:files) { %w(public/index.html) }
let(:object) { described_class.new(archive) }
let(:archive) { Rails.root.join('spec', 'fixtures', 'safe_zip', archive_name) }
......@@ -13,20 +14,36 @@
end
describe '#extract' do
subject { object.extract(directories: directories, to: target_path) }
subject { object.extract(directories: directories, files: files, to: target_path) }
shared_examples 'extracts archive' do
it 'does extract archive' do
subject
context 'when specifying directories' do
subject { object.extract(directories: directories, to: target_path) }
expect(File.exist?(File.join(target_path, 'public', 'index.html'))).to eq(true)
expect(File.exist?(File.join(target_path, 'source'))).to eq(false)
it 'does extract archive' do
subject
expect(File.exist?(File.join(target_path, 'public', 'index.html'))).to eq(true)
expect(File.exist?(File.join(target_path, 'public', 'assets', 'image.png'))).to eq(true)
expect(File.exist?(File.join(target_path, 'source'))).to eq(false)
end
end
context 'when specifying files' do
subject { object.extract(files: files, to: target_path) }
it 'does extract archive' do
subject
expect(File.exist?(File.join(target_path, 'public', 'index.html'))).to eq(true)
expect(File.exist?(File.join(target_path, 'public', 'assets', 'image.png'))).to eq(false)
end
end
end
shared_examples 'fails to extract archive' do
it 'does not extract archive' do
expect { subject }.to raise_error(SafeZip::Extract::Error)
expect { subject }.to raise_error(SafeZip::Extract::Error, including(error_message))
end
end
......@@ -38,9 +55,18 @@
end
end
%w(invalid-symlink-does-not-exist.zip invalid-symlinks-outside.zip).each do |name|
context "when using #{name} archive" do
context 'when zip files are invalid' do
using RSpec::Parameterized::TableSyntax
where(:name, :message) do
'invalid-symlink-does-not-exist.zip' | 'does not exist'
'invalid-symlinks-outside.zip' | 'Symlink cannot be created'
'invalid-unexpected-large.zip' | 'larger when inflated'
end
with_them do
let(:archive_name) { name }
let(:error_message) { message }
it_behaves_like 'fails to extract archive'
end
......@@ -49,6 +75,19 @@
context 'when no matching directories are found' do
let(:archive_name) { 'valid-simple.zip' }
let(:directories) { %w(non/existing) }
let(:error_message) { 'No entries extracted' }
subject { object.extract(directories: directories, to: target_path) }
it_behaves_like 'fails to extract archive'
end
context 'when no matching files are found' do
let(:archive_name) { 'valid-simple.zip' }
let(:files) { %w(non/existing) }
let(:error_message) { 'No entries extracted' }
subject { object.extract(files: files, to: target_path) }
it_behaves_like 'fails to extract archive'
end
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment