From 53796c72e2ec50eec77f0bd79ec0c8426cee4d58 Mon Sep 17 00:00:00 2001 From: waffles Date: Fri, 9 Dec 2016 14:09:31 -0800 Subject: [PATCH] Sandbox the component updater's patcher utility process. The code will now open files in the browser process before passing the handles across IPC to the utility process. The utility process in turn invokes courgette/bsdiff, which memory maps the files and operates on them as before. There is a behavioral difference when using the courgette or courgette_mini tools: the output file will now be created/overwritten at the start of the operation, and in the case of a failure, will be deleted. Previously, the output file was created late in the operation operation and several failure modes would leave it unmodified. BUG=660325 Review-Url: https://codereview.chromium.org/2534873005 Cr-Commit-Position: refs/heads/master@{#437669} --- ...ponent_patcher_operation_out_of_process.cc | 18 +++++- chrome/common/chrome_utility_messages.h | 12 ++-- .../utility/chrome_content_utility_client.cc | 39 ++++++------- .../utility/chrome_content_utility_client.h | 12 ++-- courgette/courgette.h | 10 ++++ courgette/ensemble_apply.cc | 55 ++++++++++++------- courgette/third_party/bsdiff/bsdiff.h | 6 ++ courgette/third_party/bsdiff/bsdiff_apply.cc | 44 +++++++++++---- 8 files changed, 128 insertions(+), 68 deletions(-) diff --git a/chrome/browser/component_updater/component_patcher_operation_out_of_process.cc b/chrome/browser/component_updater/component_patcher_operation_out_of_process.cc index ba3de60e239d1b..27d2795f0d8682 100644 --- a/chrome/browser/component_updater/component_patcher_operation_out_of_process.cc +++ b/chrome/browser/component_updater/component_patcher_operation_out_of_process.cc @@ -4,6 +4,7 @@ #include "chrome/browser/component_updater/component_patcher_operation_out_of_process.h" +#include #include #include "base/bind.h" @@ -61,7 +62,6 @@ void PatchHost::StartProcess(std::unique_ptr message) { this, base::ThreadTaskRunnerHandle::Get().get()); host->SetName(l10n_util::GetStringUTF16( IDS_UTILITY_PROCESS_COMPONENT_PATCHER_NAME)); - host->DisableSandbox(); host->Send(message.release()); } @@ -102,13 +102,25 @@ void ChromeOutOfProcessPatcher::Patch( const base::FilePath& output_abs_path, base::Callback callback) { host_ = new PatchHost(callback, task_runner); + IPC::PlatformFileForTransit input = IPC::TakePlatformFileForTransit( + base::File( + input_abs_path, base::File::FLAG_OPEN | base::File::FLAG_READ)); + IPC::PlatformFileForTransit patch = IPC::TakePlatformFileForTransit( + base::File( + patch_abs_path, base::File::FLAG_OPEN | base::File::FLAG_READ)); + IPC::PlatformFileForTransit output = IPC::TakePlatformFileForTransit( + base::File( + output_abs_path, + base::File::FLAG_CREATE | + base::File::FLAG_WRITE | + base::File::FLAG_EXCLUSIVE_WRITE)); std::unique_ptr patch_message; if (operation == update_client::kBsdiff) { patch_message.reset(new ChromeUtilityMsg_PatchFileBsdiff( - input_abs_path, patch_abs_path, output_abs_path)); + input, patch, output)); } else if (operation == update_client::kCourgette) { patch_message.reset(new ChromeUtilityMsg_PatchFileCourgette( - input_abs_path, patch_abs_path, output_abs_path)); + input, patch, output)); } else { NOTREACHED(); } diff --git a/chrome/common/chrome_utility_messages.h b/chrome/common/chrome_utility_messages.h index 99af34bc5d0bdd..4bc2f0a985c1b9 100644 --- a/chrome/common/chrome_utility_messages.h +++ b/chrome/common/chrome_utility_messages.h @@ -144,17 +144,17 @@ IPC_STRUCT_END() // and place the output in |output_file|. The patch should use the bsdiff // algorithm (Courgette's version). IPC_MESSAGE_CONTROL3(ChromeUtilityMsg_PatchFileBsdiff, - base::FilePath /* input_file */, - base::FilePath /* patch_file */, - base::FilePath /* output_file */) + IPC::PlatformFileForTransit /* input_file */, + IPC::PlatformFileForTransit /* patch_file */, + IPC::PlatformFileForTransit /* output_file */) // Tell the utility process to patch the given |input_file| using |patch_file| // and place the output in |output_file|. The patch should use the Courgette // algorithm. IPC_MESSAGE_CONTROL3(ChromeUtilityMsg_PatchFileCourgette, - base::FilePath /* input_file */, - base::FilePath /* patch_file */, - base::FilePath /* output_file */) + IPC::PlatformFileForTransit /* input_file */, + IPC::PlatformFileForTransit /* patch_file */, + IPC::PlatformFileForTransit /* output_file */) #if defined(OS_CHROMEOS) // Tell the utility process to create a zip file on the given list of files. diff --git a/chrome/utility/chrome_content_utility_client.cc b/chrome/utility/chrome_content_utility_client.cc index 0d2d215b4679ba..f364d63992ea5f 100644 --- a/chrome/utility/chrome_content_utility_client.cc +++ b/chrome/utility/chrome_content_utility_client.cc @@ -265,33 +265,26 @@ void ChromeContentUtilityClient::OnCreateZipFile( #endif // defined(OS_CHROMEOS) void ChromeContentUtilityClient::OnPatchFileBsdiff( - const base::FilePath& input_file, - const base::FilePath& patch_file, - const base::FilePath& output_file) { - if (input_file.empty() || patch_file.empty() || output_file.empty()) { - Send(new ChromeUtilityHostMsg_PatchFile_Finished(-1)); - } else { - const int patch_status = bsdiff::ApplyBinaryPatch(input_file, - patch_file, - output_file); - Send(new ChromeUtilityHostMsg_PatchFile_Finished(patch_status)); - } + const IPC::PlatformFileForTransit& input_file, + const IPC::PlatformFileForTransit& patch_file, + const IPC::PlatformFileForTransit& output_file) { + const int patch_status = bsdiff::ApplyBinaryPatch( + IPC::PlatformFileForTransitToFile(input_file), + IPC::PlatformFileForTransitToFile(patch_file), + IPC::PlatformFileForTransitToFile(output_file)); + Send(new ChromeUtilityHostMsg_PatchFile_Finished(patch_status)); ReleaseProcessIfNeeded(); } void ChromeContentUtilityClient::OnPatchFileCourgette( - const base::FilePath& input_file, - const base::FilePath& patch_file, - const base::FilePath& output_file) { - if (input_file.empty() || patch_file.empty() || output_file.empty()) { - Send(new ChromeUtilityHostMsg_PatchFile_Finished(-1)); - } else { - const int patch_status = courgette::ApplyEnsemblePatch( - input_file.value().c_str(), - patch_file.value().c_str(), - output_file.value().c_str()); - Send(new ChromeUtilityHostMsg_PatchFile_Finished(patch_status)); - } + const IPC::PlatformFileForTransit& input_file, + const IPC::PlatformFileForTransit& patch_file, + const IPC::PlatformFileForTransit& output_file) { + const int patch_status = courgette::ApplyEnsemblePatch( + IPC::PlatformFileForTransitToFile(input_file), + IPC::PlatformFileForTransitToFile(patch_file), + IPC::PlatformFileForTransitToFile(output_file)); + Send(new ChromeUtilityHostMsg_PatchFile_Finished(patch_status)); ReleaseProcessIfNeeded(); } diff --git a/chrome/utility/chrome_content_utility_client.h b/chrome/utility/chrome_content_utility_client.h index c1c8eb716de6cb..447948970e893d 100644 --- a/chrome/utility/chrome_content_utility_client.h +++ b/chrome/utility/chrome_content_utility_client.h @@ -50,12 +50,12 @@ class ChromeContentUtilityClient : public content::ContentUtilityClient { const base::FileDescriptor& dest_fd); #endif // defined(OS_CHROMEOS) - void OnPatchFileBsdiff(const base::FilePath& input_file, - const base::FilePath& patch_file, - const base::FilePath& output_file); - void OnPatchFileCourgette(const base::FilePath& input_file, - const base::FilePath& patch_file, - const base::FilePath& output_file); + void OnPatchFileBsdiff(const IPC::PlatformFileForTransit& input_file, + const IPC::PlatformFileForTransit& patch_file, + const IPC::PlatformFileForTransit& output_file); + void OnPatchFileCourgette(const IPC::PlatformFileForTransit& input_file, + const IPC::PlatformFileForTransit& patch_file, + const IPC::PlatformFileForTransit& output_file); void OnStartupPing(); #if defined(FULL_SAFE_BROWSING) void OnAnalyzeZipFileForDownloadProtection( diff --git a/courgette/courgette.h b/courgette/courgette.h index 2dacc7878ca31c..533756659a1126 100644 --- a/courgette/courgette.h +++ b/courgette/courgette.h @@ -7,6 +7,7 @@ #include // Required to define size_t on GCC +#include "base/files/file.h" #include "base/files/file_path.h" namespace courgette { @@ -73,6 +74,15 @@ class EncodedProgram; Status ApplyEnsemblePatch(SourceStream* old, SourceStream* patch, SinkStream* output); +// Applies the patch in |patch_file| to the bytes in |old_file| and writes the +// transformed ensemble to |new_file|. +// Returns C_OK unless something went wrong. +// This function first validates that the patch file has a proper header, so the +// function can be used to 'try' a patch. +Status ApplyEnsemblePatch(base::File old_file, + base::File patch_file, + base::File new_file); + // Applies the patch in |patch_file_name| to the bytes in |old_file_name| and // writes the transformed ensemble to |new_file_name|. // Returns C_OK unless something went wrong. diff --git a/courgette/ensemble_apply.cc b/courgette/ensemble_apply.cc index 8ed9cbd9f5d0b4..9f4ec54bfabc23 100644 --- a/courgette/ensemble_apply.cc +++ b/courgette/ensemble_apply.cc @@ -12,6 +12,7 @@ #include #include +#include "base/files/file.h" #include "base/files/file_util.h" #include "base/files/memory_mapped_file.h" #include "base/logging.h" @@ -371,33 +372,31 @@ Status ApplyEnsemblePatch(SourceStream* base, return C_OK; } -Status ApplyEnsemblePatch(const base::FilePath::CharType* old_file_name, - const base::FilePath::CharType* patch_file_name, - const base::FilePath::CharType* new_file_name) { - base::FilePath patch_file_path(patch_file_name); - base::MemoryMappedFile patch_file; - if (!patch_file.Initialize(patch_file_path)) +Status ApplyEnsemblePatch(base::File old_file, + base::File patch_file, + base::File new_file) { + base::MemoryMappedFile patch_file_mem; + if (!patch_file_mem.Initialize(std::move(patch_file))) return C_READ_OPEN_ERROR; // 'Dry-run' the first step of the patch process to validate format of header. SourceStream patch_header_stream; - patch_header_stream.Init(patch_file.data(), patch_file.length()); + patch_header_stream.Init(patch_file_mem.data(), patch_file_mem.length()); EnsemblePatchApplication patch_process; Status status = patch_process.ReadHeader(&patch_header_stream); if (status != C_OK) return status; // Read the old_file. - base::FilePath old_file_path(old_file_name); - base::MemoryMappedFile old_file; - if (!old_file.Initialize(old_file_path)) + base::MemoryMappedFile old_file_mem; + if (!old_file_mem.Initialize(std::move(old_file))) return C_READ_ERROR; // Apply patch on streams. SourceStream old_source_stream; SourceStream patch_source_stream; - old_source_stream.Init(old_file.data(), old_file.length()); - patch_source_stream.Init(patch_file.data(), patch_file.length()); + old_source_stream.Init(old_file_mem.data(), old_file_mem.length()); + patch_source_stream.Init(patch_file_mem.data(), patch_file_mem.length()); SinkStream new_sink_stream; status = ApplyEnsemblePatch(&old_source_stream, &patch_source_stream, &new_sink_stream); @@ -405,12 +404,10 @@ Status ApplyEnsemblePatch(const base::FilePath::CharType* old_file_name, return status; // Write the patched data to |new_file_name|. - base::FilePath new_file_path(new_file_name); - int written = - base::WriteFile( - new_file_path, - reinterpret_cast(new_sink_stream.Buffer()), - static_cast(new_sink_stream.Length())); + int written = new_file.Write( + 0, + reinterpret_cast(new_sink_stream.Buffer()), + static_cast(new_sink_stream.Length())); if (written == -1) return C_WRITE_OPEN_ERROR; if (static_cast(written) != new_sink_stream.Length()) @@ -419,4 +416,24 @@ Status ApplyEnsemblePatch(const base::FilePath::CharType* old_file_name, return C_OK; } -} // namespace +Status ApplyEnsemblePatch(const base::FilePath::CharType* old_file_name, + const base::FilePath::CharType* patch_file_name, + const base::FilePath::CharType* new_file_name) { + Status result = ApplyEnsemblePatch( + base::File( + base::FilePath(old_file_name), + base::File::FLAG_OPEN | base::File::FLAG_READ), + base::File( + base::FilePath(patch_file_name), + base::File::FLAG_OPEN | base::File::FLAG_READ), + base::File( + base::FilePath(new_file_name), + base::File::FLAG_CREATE_ALWAYS | + base::File::FLAG_WRITE | + base::File::FLAG_EXCLUSIVE_WRITE)); + if (result != C_OK) + base::DeleteFile(base::FilePath(new_file_name), false); + return result; +} + +} // namespace courgette diff --git a/courgette/third_party/bsdiff/bsdiff.h b/courgette/third_party/bsdiff/bsdiff.h index a5da4ec9dd130d..c1d26c728aa804 100644 --- a/courgette/third_party/bsdiff/bsdiff.h +++ b/courgette/third_party/bsdiff/bsdiff.h @@ -44,6 +44,7 @@ #include +#include "base/files/file.h" #include "base/files/file_util.h" namespace courgette { @@ -76,6 +77,11 @@ BSDiffStatus ApplyBinaryPatch(courgette::SourceStream* old_stream, courgette::SourceStream* patch_stream, courgette::SinkStream* new_stream); +// As above, but simply takes base::Files. +BSDiffStatus ApplyBinaryPatch(base::File old_stream, + base::File patch_stream, + base::File new_stream); + // As above, but simply takes the file paths. BSDiffStatus ApplyBinaryPatch(const base::FilePath& old_stream, const base::FilePath& patch_stream, diff --git a/courgette/third_party/bsdiff/bsdiff_apply.cc b/courgette/third_party/bsdiff/bsdiff_apply.cc index fca9616a8ab0bf..8d43c5d5175856 100644 --- a/courgette/third_party/bsdiff/bsdiff_apply.cc +++ b/courgette/third_party/bsdiff/bsdiff_apply.cc @@ -40,6 +40,7 @@ #include #include +#include "base/files/file_util.h" #include "base/files/memory_mapped_file.h" #include "courgette/crc.h" #include "courgette/streams.h" @@ -189,24 +190,24 @@ BSDiffStatus ApplyBinaryPatch(SourceStream* old_stream, return OK; } -BSDiffStatus ApplyBinaryPatch(const base::FilePath& old_file_path, - const base::FilePath& patch_file_path, - const base::FilePath& new_file_path) { +BSDiffStatus ApplyBinaryPatch(base::File old_file, + base::File patch_file, + base::File new_file) { // Set up the old stream. - base::MemoryMappedFile old_file; - if (!old_file.Initialize(old_file_path)) { + base::MemoryMappedFile old_file_mem; + if (!old_file_mem.Initialize(std::move(old_file))) { return READ_ERROR; } SourceStream old_file_stream; - old_file_stream.Init(old_file.data(), old_file.length()); + old_file_stream.Init(old_file_mem.data(), old_file_mem.length()); // Set up the patch stream. - base::MemoryMappedFile patch_file; - if (!patch_file.Initialize(patch_file_path)) { + base::MemoryMappedFile patch_file_mem; + if (!patch_file_mem.Initialize(std::move(patch_file))) { return READ_ERROR; } SourceStream patch_file_stream; - patch_file_stream.Init(patch_file.data(), patch_file.length()); + patch_file_stream.Init(patch_file_mem.data(), patch_file_mem.length()); // Set up the new stream and apply the patch. SinkStream new_sink_stream; @@ -217,12 +218,33 @@ BSDiffStatus ApplyBinaryPatch(const base::FilePath& old_file_path, } // Write the stream to disk. - int written = base::WriteFile( - new_file_path, reinterpret_cast(new_sink_stream.Buffer()), + int written = new_file.Write( + 0, + reinterpret_cast(new_sink_stream.Buffer()), static_cast(new_sink_stream.Length())); if (written != static_cast(new_sink_stream.Length())) return WRITE_ERROR; return OK; } +BSDiffStatus ApplyBinaryPatch(const base::FilePath& old_file_path, + const base::FilePath& patch_file_path, + const base::FilePath& new_file_path) { + BSDiffStatus result = ApplyBinaryPatch( + base::File( + old_file_path, + base::File::FLAG_OPEN | base::File::FLAG_READ), + base::File( + patch_file_path, + base::File::FLAG_OPEN | base::File::FLAG_READ), + base::File( + new_file_path, + base::File::FLAG_CREATE_ALWAYS | + base::File::FLAG_WRITE | + base::File::FLAG_EXCLUSIVE_WRITE)); + if (result != OK) + base::DeleteFile(new_file_path, false); + return result; +} + } // namespace bsdiff