From 86b964745038eebff6218e97735e6344b6f3e42a Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 17 Aug 2018 22:42:37 -0400 Subject: [PATCH] utils: Use JSON to marshal child errors Replaces our serialization of child process errors via Marshal with JSON, preventing unintentional or malicious code execution outside of the build sandbox. Additionally, adds tests for the new behavior. --- Library/Homebrew/build.rb | 15 +++++- Library/Homebrew/dev-cmd/test.rb | 3 -- Library/Homebrew/exceptions.rb | 41 ++++++++++++++-- Library/Homebrew/formula_installer.rb | 8 +++- Library/Homebrew/global.rb | 2 + Library/Homebrew/postinstall.rb | 2 +- Library/Homebrew/test.rb | 2 +- .../Homebrew/test/formula_installer_spec.rb | 20 +++++++- .../test/support/fixtures/failball.rb | 20 ++++++++ Library/Homebrew/test/support/lib/config.rb | 6 ++- Library/Homebrew/test/utils/fork_spec.rb | 21 +++++++++ Library/Homebrew/utils/fork.rb | 47 +++++++++++++++++-- 12 files changed, 169 insertions(+), 18 deletions(-) create mode 100644 Library/Homebrew/test/support/fixtures/failball.rb create mode 100644 Library/Homebrew/test/utils/fork_spec.rb diff --git a/Library/Homebrew/build.rb b/Library/Homebrew/build.rb index fe4225384e..3d72481687 100644 --- a/Library/Homebrew/build.rb +++ b/Library/Homebrew/build.rb @@ -190,7 +190,20 @@ begin build = Build.new(formula, options) build.install rescue Exception => e # rubocop:disable Lint/RescueException - Marshal.dump(e, error_pipe) + error_hash = JSON.parse e.to_json + + # Special case: need to recreate BuildErrors in full + # for proper analytics reporting and error messages. + # BuildErrors are specific to build processses and not other + # children, which is why we create the necessary state here + # and not in Utils.safe_fork. + if error_hash["json_class"] == "BuildError" + error_hash["cmd"] = e.cmd + error_hash["args"] = e.args + error_hash["env"] = e.env + end + + error_pipe.puts error_hash.to_json error_pipe.close exit! 1 end diff --git a/Library/Homebrew/dev-cmd/test.rb b/Library/Homebrew/dev-cmd/test.rb index d21a62cde5..8e8ceb9824 100644 --- a/Library/Homebrew/dev-cmd/test.rb +++ b/Library/Homebrew/dev-cmd/test.rb @@ -93,9 +93,6 @@ module Homebrew exec(*args) end end - rescue ::Test::Unit::AssertionFailedError => e - ofail "#{f.full_name}: failed" - puts e.message rescue Exception => e # rubocop:disable Lint/RescueException ofail "#{f.full_name}: failed" puts e, e.backtrace diff --git a/Library/Homebrew/exceptions.rb b/Library/Homebrew/exceptions.rb index dc3f5f9e36..51d85d19ca 100644 --- a/Library/Homebrew/exceptions.rb +++ b/Library/Homebrew/exceptions.rb @@ -352,14 +352,16 @@ class FormulaAmbiguousPythonError < RuntimeError end class BuildError < RuntimeError - attr_reader :formula, :env - attr_accessor :options + attr_reader :cmd, :args, :env + attr_accessor :formula, :options def initialize(formula, cmd, args, env) @formula = formula + @cmd = cmd + @args = args @env = env - args = args.map { |arg| arg.to_s.gsub " ", "\\ " }.join(" ") - super "Failed executing: #{cmd} #{args}" + pretty_args = args.map { |arg| arg.to_s.gsub " ", "\\ " }.join(" ") + super "Failed executing: #{cmd} #{pretty_args}" end def issues @@ -526,12 +528,22 @@ end # raised by safe_system in utils.rb class ErrorDuringExecution < RuntimeError + attr_reader :cmd attr_reader :status + attr_reader :output def initialize(cmd, status:, output: nil) + @cmd = cmd @status = status + @output = output - s = "Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{status.exitstatus}." + exitstatus = if status.respond_to?(:exitstatus) + status.exitstatus + else + status + end + + s = "Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{exitstatus}." unless [*output].empty? format_output_line = lambda do |type, line| @@ -596,3 +608,22 @@ class BottleFormulaUnavailableError < RuntimeError EOS end end + +# Raised when a child process sends us an exception over its error pipe. +class ChildProcessError < RuntimeError + attr_reader :inner + attr_reader :inner_class + + def initialize(inner) + @inner = inner + @inner_class = Object.const_get inner["json_class"] + + super <<~EOS + An exception occured within a build process: + #{inner_class}: #{inner["m"]} + EOS + + # Clobber our real (but irrelevant) backtrace with that of the inner exception. + set_backtrace inner["b"] + end +end diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb index cb03e5c9cc..8eeea7c160 100644 --- a/Library/Homebrew/formula_installer.rb +++ b/Library/Homebrew/formula_installer.rb @@ -744,14 +744,18 @@ class FormulaInstaller raise "Empty installation" end rescue Exception => e # rubocop:disable Lint/RescueException - e.options = display_options(formula) if e.is_a?(BuildError) + if e.is_a? BuildError + e.formula = formula + e.options = options + end + ignore_interrupts do # any exceptions must leave us with nothing installed formula.update_head_version formula.prefix.rmtree if formula.prefix.directory? formula.rack.rmdir_if_possible end - raise + raise e end def link(keg) diff --git a/Library/Homebrew/global.rb b/Library/Homebrew/global.rb index ce38bddd59..78f7bd30ba 100644 --- a/Library/Homebrew/global.rb +++ b/Library/Homebrew/global.rb @@ -1,5 +1,7 @@ require "pathname" require "English" +require "json" +require "json/add/core" HOMEBREW_LIBRARY_PATH = Pathname.new(__FILE__).realpath.parent diff --git a/Library/Homebrew/postinstall.rb b/Library/Homebrew/postinstall.rb index 53a5b7e751..94c56e48fc 100644 --- a/Library/Homebrew/postinstall.rb +++ b/Library/Homebrew/postinstall.rb @@ -15,7 +15,7 @@ begin formula.extend(Debrew::Formula) if ARGV.debug? formula.run_post_install rescue Exception => e # rubocop:disable Lint/RescueException - Marshal.dump(e, error_pipe) + error_pipe.puts e.to_json error_pipe.close exit! 1 end diff --git a/Library/Homebrew/test.rb b/Library/Homebrew/test.rb index 3d5e62a884..fbe6fa64e7 100644 --- a/Library/Homebrew/test.rb +++ b/Library/Homebrew/test.rb @@ -28,7 +28,7 @@ begin raise "test returned false" if formula.run_test == false end rescue Exception => e # rubocop:disable Lint/RescueException - Marshal.dump(e, error_pipe) + error_pipe.puts e.to_json error_pipe.close exit! 1 end diff --git a/Library/Homebrew/test/formula_installer_spec.rb b/Library/Homebrew/test/formula_installer_spec.rb index 932738742b..de6008c6b4 100644 --- a/Library/Homebrew/test/formula_installer_spec.rb +++ b/Library/Homebrew/test/formula_installer_spec.rb @@ -4,6 +4,7 @@ require "keg" require "tab" require "test/support/fixtures/testball" require "test/support/fixtures/testball_bottle" +require "test/support/fixtures/failball" describe FormulaInstaller do define_negated_matcher :need_bottle, :be_bottle_unneeded @@ -28,7 +29,7 @@ describe FormulaInstaller do Tab.clear_cache expect(Tab.for_keg(keg)).not_to be_poured_from_bottle - yield formula + yield formula if block_given? ensure Tab.clear_cache keg.unlink @@ -132,4 +133,21 @@ describe FormulaInstaller do fi.check_install_sanity }.to raise_error(CannotInstallFormulaError) end + + specify "install fails with BuildError when a system() call fails" do + ENV["HOMEBREW_TEST_NO_EXIT_CLEANUP"] = "1" + ENV["FAILBALL_BUILD_ERROR"] = "1" + + expect { + temporary_install(Failball.new) + }.to raise_error(BuildError) + end + + specify "install fails with a RuntimeError when #install raises" do + ENV["HOMEBREW_TEST_NO_EXIT_CLEANUP"] = "1" + + expect { + temporary_install(Failball.new) + }.to raise_error(RuntimeError) + end end diff --git a/Library/Homebrew/test/support/fixtures/failball.rb b/Library/Homebrew/test/support/fixtures/failball.rb new file mode 100644 index 0000000000..125452feca --- /dev/null +++ b/Library/Homebrew/test/support/fixtures/failball.rb @@ -0,0 +1,20 @@ +class Failball < Formula + def initialize(name = "failball", path = Pathname.new(__FILE__).expand_path, spec = :stable, alias_path: nil) + self.class.instance_eval do + stable.url "file://#{TEST_FIXTURE_DIR}/tarballs/testball-0.1.tbz" + stable.sha256 TESTBALL_SHA256 + end + super + end + + def install + prefix.install "bin" + prefix.install "libexec" + + # This should get marshalled into a BuildError. + system "/usr/bin/false" if ENV["FAILBALL_BUILD_ERROR"] + + # This should get marshalled into a RuntimeError. + raise "something that isn't a build error happened!" + end +end diff --git a/Library/Homebrew/test/support/lib/config.rb b/Library/Homebrew/test/support/lib/config.rb index 5f0b69313c..68030ad9a5 100644 --- a/Library/Homebrew/test/support/lib/config.rb +++ b/Library/Homebrew/test/support/lib/config.rb @@ -8,7 +8,11 @@ HOMEBREW_BREW_FILE = Pathname.new(ENV["HOMEBREW_BREW_FILE"]) TEST_TMPDIR = ENV.fetch("HOMEBREW_TEST_TMPDIR") do |k| dir = Dir.mktmpdir("homebrew-tests-", ENV["HOMEBREW_TEMP"] || "/tmp") - at_exit { FileUtils.remove_entry(dir) } + at_exit do + # Child processes inherit this at_exit handler, but we don't want them + # to clean TEST_TMPDIR up prematurely (i.e., when they exit early for a test). + FileUtils.remove_entry(dir) unless ENV["HOMEBREW_TEST_NO_EXIT_CLEANUP"] + end ENV[k] = dir end diff --git a/Library/Homebrew/test/utils/fork_spec.rb b/Library/Homebrew/test/utils/fork_spec.rb new file mode 100644 index 0000000000..0af9b8f3ae --- /dev/null +++ b/Library/Homebrew/test/utils/fork_spec.rb @@ -0,0 +1,21 @@ +require "utils/fork" + +describe Utils do + describe "#safe_fork" do + it "raises a RuntimeError on an error that isn't ErrorDuringExecution" do + expect { + described_class.safe_fork do + raise "this is an exception in the child" + end + }.to raise_error(RuntimeError) + end + + it "raises an ErrorDuringExecution on one in the child" do + expect { + described_class.safe_fork do + safe_system "/usr/bin/false" + end + }.to raise_error(ErrorDuringExecution) + end + end +end diff --git a/Library/Homebrew/utils/fork.rb b/Library/Homebrew/utils/fork.rb index 5087ca716c..d0ecbb4ded 100644 --- a/Library/Homebrew/utils/fork.rb +++ b/Library/Homebrew/utils/fork.rb @@ -2,6 +2,26 @@ require "fcntl" require "socket" module Utils + def self.rewrite_child_error(child_error) + error = if child_error.inner_class == ErrorDuringExecution + ErrorDuringExecution.new(child_error.inner["cmd"], + status: child_error.inner["status"], + output: child_error.inner["output"]) + elsif child_error.inner_class == BuildError + # We fill `BuildError#formula` and `BuildError#options` in later, + # when we rescue this in `FormulaInstaller#build`. + BuildError.new(nil, child_error.inner["cmd"], + child_error.inner["args"], child_error.inner["env"]) + else + # Everything other error in the child just becomes a RuntimeError. + RuntimeError.new(child_error.message) + end + + error.set_backtrace child_error.backtrace + + error + end + def self.safe_fork(&_block) Dir.mktmpdir("homebrew", HOMEBREW_TEMP) do |tmpdir| UNIXServer.open("#{tmpdir}/socket") do |server| @@ -15,7 +35,18 @@ module Utils write.fcntl(Fcntl::F_SETFD, Fcntl::FD_CLOEXEC) yield rescue Exception => e # rubocop:disable Lint/RescueException - Marshal.dump(e, write) + error_hash = JSON.parse e.to_json + + # Special case: We need to recreate ErrorDuringExecutions + # for proper error messages and because other code expects + # to rescue them further down. + if e.is_a?(ErrorDuringExecution) + error_hash["cmd"] = e.cmd + error_hash["status"] = e.status.exitstatus + error_hash["output"] = e.output + end + + write.puts error_hash.to_json write.close exit! else @@ -33,10 +64,20 @@ module Utils socket.close end write.close - data = read.read + # Each line on the error pipe contains a JSON-serialized exception. + # We read the first, since only one is necessary for a failure. + data = read.gets read.close Process.wait(pid) unless socket.nil? - raise Marshal.load(data) unless data.nil? || data.empty? # rubocop:disable Security/MarshalLoad + + if data && !data.empty? + error_hash = JSON.parse(data) unless data.nil? || data.empty? + + e = ChildProcessError.new(error_hash) + + raise rewrite_child_error(e) + end + raise Interrupt if $CHILD_STATUS.exitstatus == 130 raise "Forked child process failed: #{$CHILD_STATUS}" unless $CHILD_STATUS.success? end