Merge pull request #18183 from Homebrew/sandbox-fork-fix

Fix misuse of `fork` in sandbox causing crashes
This commit is contained in:
Bo Anderson 2024-08-28 15:14:59 +01:00 committed by GitHub
commit f5d39c82c3
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 198 additions and 175 deletions

View File

@ -81,21 +81,21 @@ module Homebrew
exec_args << "--HEAD" if f.head?
Utils.safe_fork do |error_pipe|
if Sandbox.available?
sandbox = Sandbox.new
f.logs.mkpath
sandbox.record_log(f.logs/"test.sandbox.log")
sandbox.allow_write_temp_and_cache
sandbox.allow_write_log(f)
sandbox.allow_write_xcode
sandbox.allow_write_path(HOMEBREW_PREFIX/"var/cache")
sandbox.allow_write_path(HOMEBREW_PREFIX/"var/homebrew/locks")
sandbox.allow_write_path(HOMEBREW_PREFIX/"var/log")
sandbox.allow_write_path(HOMEBREW_PREFIX/"var/run")
sandbox.deny_all_network_except_pipe(error_pipe) unless f.class.network_access_allowed?(:test)
sandbox.exec(*exec_args)
else
if Sandbox.available?
sandbox = Sandbox.new
f.logs.mkpath
sandbox.record_log(f.logs/"test.sandbox.log")
sandbox.allow_write_temp_and_cache
sandbox.allow_write_log(f)
sandbox.allow_write_xcode
sandbox.allow_write_path(HOMEBREW_PREFIX/"var/cache")
sandbox.allow_write_path(HOMEBREW_PREFIX/"var/homebrew/locks")
sandbox.allow_write_path(HOMEBREW_PREFIX/"var/log")
sandbox.allow_write_path(HOMEBREW_PREFIX/"var/run")
sandbox.deny_all_network unless f.class.network_access_allowed?(:test)
sandbox.run(*exec_args)
else
Utils.safe_fork do
exec(*exec_args)
end
end

View File

@ -755,21 +755,14 @@ class BottleFormulaUnavailableError < RuntimeError
end
end
# Raised when a child process sends us an exception over its error pipe.
# Raised when a `Utils.safe_fork` exits with a non-zero code.
class ChildProcessError < RuntimeError
attr_reader :inner, :inner_class
attr_reader :status
def initialize(inner)
@inner = inner
@inner_class = Object.const_get inner["json_class"]
def initialize(status)
@status = status
super <<~EOS
An exception occurred within a child process:
#{inner_class}: #{inner["m"]}
EOS
# Clobber our real (but irrelevant) backtrace with that of the inner exception.
set_backtrace inner["b"]
super "Forked child process failed: #{status}"
end
end

View File

@ -931,21 +931,21 @@ on_request: installed_on_request?, options:)
formula.specified_path,
].concat(build_argv)
Utils.safe_fork do |error_pipe|
if Sandbox.available?
sandbox = Sandbox.new
formula.logs.mkpath
sandbox.record_log(formula.logs/"build.sandbox.log")
sandbox.allow_write_path(Dir.home) if interactive?
sandbox.allow_write_temp_and_cache
sandbox.allow_write_log(formula)
sandbox.allow_cvs
sandbox.allow_fossil
sandbox.allow_write_xcode
sandbox.allow_write_cellar(formula)
sandbox.deny_all_network_except_pipe(error_pipe) unless formula.network_access_allowed?(:build)
sandbox.exec(*args)
else
if Sandbox.available?
sandbox = Sandbox.new
formula.logs.mkpath
sandbox.record_log(formula.logs/"build.sandbox.log")
sandbox.allow_write_path(Dir.home) if interactive?
sandbox.allow_write_temp_and_cache
sandbox.allow_write_log(formula)
sandbox.allow_cvs
sandbox.allow_fossil
sandbox.allow_write_xcode
sandbox.allow_write_cellar(formula)
sandbox.deny_all_network unless formula.network_access_allowed?(:build)
sandbox.run(*args)
else
Utils.safe_fork do
exec(*args)
end
end
@ -1161,22 +1161,22 @@ on_request: installed_on_request?, options:)
args << post_install_formula_path
Utils.safe_fork do |error_pipe|
if Sandbox.available?
sandbox = Sandbox.new
formula.logs.mkpath
sandbox.record_log(formula.logs/"postinstall.sandbox.log")
sandbox.allow_write_temp_and_cache
sandbox.allow_write_log(formula)
sandbox.allow_write_xcode
sandbox.deny_write_homebrew_repository
sandbox.allow_write_cellar(formula)
sandbox.deny_all_network_except_pipe(error_pipe) unless formula.network_access_allowed?(:postinstall)
Keg::KEG_LINK_DIRECTORIES.each do |dir|
sandbox.allow_write_path "#{HOMEBREW_PREFIX}/#{dir}"
end
sandbox.exec(*args)
else
if Sandbox.available?
sandbox = Sandbox.new
formula.logs.mkpath
sandbox.record_log(formula.logs/"postinstall.sandbox.log")
sandbox.allow_write_temp_and_cache
sandbox.allow_write_log(formula)
sandbox.allow_write_xcode
sandbox.deny_write_homebrew_repository
sandbox.allow_write_cellar(formula)
sandbox.deny_all_network unless formula.network_access_allowed?(:postinstall)
Keg::KEG_LINK_DIRECTORIES.each do |dir|
sandbox.allow_write_path "#{HOMEBREW_PREFIX}/#{dir}"
end
sandbox.run(*args)
else
Utils.safe_fork do
exec(*args)
end
end

View File

@ -5,12 +5,19 @@ require "erb"
require "io/console"
require "pty"
require "tempfile"
require "utils/fork"
# Helper class for running a sub-process inside of a sandboxed environment.
class Sandbox
SANDBOX_EXEC = "/usr/bin/sandbox-exec"
private_constant :SANDBOX_EXEC
# This is defined in the macOS SDK but Ruby unfortunately does not expose it.
# This value can be found by compiling a C program that prints TIOCSCTTY.
# The value is different on Linux but that's not a problem as we only support macOS in this file.
TIOCSCTTY = 0x20007461
private_constant :TIOCSCTTY
sig { returns(T::Boolean) }
def self.available?
false
@ -125,108 +132,122 @@ class Sandbox
add_rule allow: false, operation: "network*"
end
sig { params(path: T.any(String, Pathname)).void }
def deny_all_network_except_pipe(path)
deny_all_network
allow_network path:, type: :literal
end
sig { params(args: T.any(String, Pathname)).void }
def exec(*args)
seatbelt = Tempfile.new(["homebrew", ".sb"], HOMEBREW_TEMP)
seatbelt.write(@profile.dump)
seatbelt.close
@start = T.let(Time.now, T.nilable(Time))
def run(*args)
Dir.mktmpdir("homebrew-sandbox", HOMEBREW_TEMP) do |tmpdir|
allow_network path: File.join(tmpdir, "socket"), type: :literal # Make sure we have access to the error pipe.
begin
command = [SANDBOX_EXEC, "-f", seatbelt.path, *args]
# Start sandbox in a pseudoterminal to prevent access of the parent terminal.
PTY.spawn(*command) do |r, w, pid|
# Set the PTY's window size to match the parent terminal.
# Some formula tests are sensitive to the terminal size and fail if this is not set.
winch = proc do |_sig|
w.winsize = if $stdout.tty?
# We can only use IO#winsize if the IO object is a TTY.
$stdout.winsize
else
# Otherwise, default to tput, if available.
# This relies on ncurses rather than the system's ioctl.
[Utils.popen_read("tput", "lines").to_i, Utils.popen_read("tput", "cols").to_i]
end
end
seatbelt = File.new(File.join(tmpdir, "homebrew.sb"), "wx")
seatbelt.write(@profile.dump)
seatbelt.close
@start = T.let(Time.now, T.nilable(Time))
write_to_pty = proc do
# Don't hang if stdin is not able to be used - throw EIO instead.
old_ttin = trap(:TTIN, "IGNORE")
# Update the window size whenever the parent terminal's window size changes.
old_winch = trap(:WINCH, &winch)
winch.call(nil)
stdin_thread = Thread.new do
IO.copy_stream($stdin, w)
rescue Errno::EIO
# stdin is unavailable - move on.
begin
command = [SANDBOX_EXEC, "-f", seatbelt.path, *args]
# Start sandbox in a pseudoterminal to prevent access of the parent terminal.
PTY.open do |controller, worker|
# Set the PTY's window size to match the parent terminal.
# Some formula tests are sensitive to the terminal size and fail if this is not set.
winch = proc do |_sig|
controller.winsize = if $stdout.tty?
# We can only use IO#winsize if the IO object is a TTY.
$stdout.winsize
else
# Otherwise, default to tput, if available.
# This relies on ncurses rather than the system's ioctl.
[Utils.popen_read("tput", "lines").to_i, Utils.popen_read("tput", "cols").to_i]
end
end
r.each_char { |c| print(c) }
write_to_pty = proc do
# Don't hang if stdin is not able to be used - throw EIO instead.
old_ttin = trap(:TTIN, "IGNORE")
Process.wait(pid)
ensure
stdin_thread&.kill
trap(:TTIN, old_ttin)
trap(:WINCH, old_winch)
end
# Update the window size whenever the parent terminal's window size changes.
old_winch = trap(:WINCH, &winch)
winch.call(nil)
if $stdin.tty?
# If stdin is a TTY, use io.raw to set stdin to a raw, passthrough
# mode while we copy the input/output of the process spawned in the
# PTY. After we've finished copying to/from the PTY process, io.raw
# will restore the stdin TTY to its original state.
begin
# Ignore SIGTTOU as setting raw mode will hang if the process is in the background.
old_ttou = trap(:TTOU, "IGNORE")
$stdin.raw(&write_to_pty)
stdin_thread = Thread.new do
IO.copy_stream($stdin, controller)
rescue Errno::EIO
# stdin is unavailable - move on.
end
stdout_thread = Thread.new do
controller.each_char { |c| print(c) }
end
Utils.safe_fork(directory: tmpdir, yield_parent: true) do |error_pipe|
if error_pipe
# Child side
Process.setsid
controller.close
worker.ioctl(TIOCSCTTY, 0) # Make this the controlling terminal.
File.open("/dev/tty", Fcntl::O_WRONLY).close # Workaround for https://developer.apple.com/forums/thread/663632
worker.close_on_exec = true
exec(*command, in: worker, out: worker, err: worker) # And map everything to the PTY.
else
# Parent side
worker.close
end
end
rescue ChildProcessError => e
raise ErrorDuringExecution.new(command, status: e.status)
ensure
trap(:TTOU, old_ttou)
stdin_thread&.kill
stdout_thread&.kill
trap(:TTIN, old_ttin)
trap(:WINCH, old_winch)
end
else
write_to_pty.call
end
end
raise ErrorDuringExecution.new(command, status: $CHILD_STATUS) unless $CHILD_STATUS.success?
rescue
@failed = true
raise
ensure
seatbelt.unlink
sleep 0.1 # wait for a bit to let syslog catch up the latest events.
syslog_args = [
"-F", "$((Time)(local)) $(Sender)[$(PID)]: $(Message)",
"-k", "Time", "ge", @start.to_i.to_s,
"-k", "Message", "S", "deny",
"-k", "Sender", "kernel",
"-o",
"-k", "Time", "ge", @start.to_i.to_s,
"-k", "Message", "S", "deny",
"-k", "Sender", "sandboxd"
]
logs = Utils.popen_read("syslog", *syslog_args)
# These messages are confusing and non-fatal, so don't report them.
logs = logs.lines.grep_v(/^.*Python\(\d+\) deny file-write.*pyc$/).join
unless logs.empty?
if @logfile
File.open(@logfile, "w") do |log|
log.write logs
log.write "\nWe use time to filter sandbox log. Therefore, unrelated logs may be recorded.\n"
if $stdin.tty?
# If stdin is a TTY, use io.raw to set stdin to a raw, passthrough
# mode while we copy the input/output of the process spawned in the
# PTY. After we've finished copying to/from the PTY process, io.raw
# will restore the stdin TTY to its original state.
begin
# Ignore SIGTTOU as setting raw mode will hang if the process is in the background.
old_ttou = trap(:TTOU, "IGNORE")
$stdin.raw(&write_to_pty)
ensure
trap(:TTOU, old_ttou)
end
else
write_to_pty.call
end
end
rescue
@failed = true
raise
ensure
sleep 0.1 # wait for a bit to let syslog catch up the latest events.
syslog_args = [
"-F", "$((Time)(local)) $(Sender)[$(PID)]: $(Message)",
"-k", "Time", "ge", @start.to_i.to_s,
"-k", "Message", "S", "deny",
"-k", "Sender", "kernel",
"-o",
"-k", "Time", "ge", @start.to_i.to_s,
"-k", "Message", "S", "deny",
"-k", "Sender", "sandboxd"
]
logs = Utils.popen_read("syslog", *syslog_args)
if @failed && Homebrew::EnvConfig.verbose?
ohai "Sandbox Log", logs
$stdout.flush # without it, brew test-bot would fail to catch the log
# These messages are confusing and non-fatal, so don't report them.
logs = logs.lines.grep_v(/^.*Python\(\d+\) deny file-write.*pyc$/).join
unless logs.empty?
if @logfile
File.open(@logfile, "w") do |log|
log.write logs
log.write "\nWe use time to filter sandbox log. Therefore, unrelated logs may be recorded.\n"
end
end
if @failed && Homebrew::EnvConfig.verbose?
ohai "Sandbox Log", logs
$stdout.flush # without it, brew test-bot would fail to catch the log
end
end
end
end

View File

@ -16,7 +16,7 @@ RSpec.describe Sandbox, :needs_macos do
specify "#allow_write" do
sandbox.allow_write path: file
sandbox.exec "touch", file
sandbox.run "touch", file
expect(file).to exist
end
@ -65,10 +65,10 @@ RSpec.describe Sandbox, :needs_macos do
end
end
describe "#exec" do
describe "#run" do
it "fails when writing to file not specified with ##allow_write" do
expect do
sandbox.exec "touch", file
sandbox.run "touch", file
end.to raise_error(ErrorDuringExecution)
expect(file).not_to exist
@ -80,7 +80,7 @@ RSpec.describe Sandbox, :needs_macos do
allow(Utils).to receive(:popen_read).and_call_original
allow(Utils).to receive(:popen_read).with("syslog", any_args).and_return("foo")
expect { sandbox.exec "false" }
expect { sandbox.run "false" }
.to raise_error(ErrorDuringExecution)
.and output(/foo/).to_stdout
end
@ -96,7 +96,7 @@ RSpec.describe Sandbox, :needs_macos do
allow(Utils).to receive(:popen_read).and_call_original
allow(Utils).to receive(:popen_read).with("syslog", any_args).and_return(with_bogus_error)
expect { sandbox.exec "false" }
expect { sandbox.run "false" }
.to raise_error(ErrorDuringExecution)
.and output(a_string_matching(/foo/).and(matching(/bar/).and(not_matching(/Python/)))).to_stdout
end
@ -104,28 +104,28 @@ RSpec.describe Sandbox, :needs_macos do
describe "#disallow chmod on some directory" do
it "formula does a chmod to opt" do
expect { sandbox.exec "chmod", "ug-w", HOMEBREW_PREFIX }.to raise_error(ErrorDuringExecution)
expect { sandbox.run "chmod", "ug-w", HOMEBREW_PREFIX }.to raise_error(ErrorDuringExecution)
end
it "allows chmod on a path allowed to write" do
mktmpdir do |path|
FileUtils.touch path/"foo"
sandbox.allow_write_path(path)
expect { sandbox.exec "chmod", "ug-w", path/"foo" }.not_to raise_error(ErrorDuringExecution)
expect { sandbox.run "chmod", "ug-w", path/"foo" }.not_to raise_error(ErrorDuringExecution)
end
end
end
describe "#disallow chmod SUID or SGID on some directory" do
it "formula does a chmod 4000 to opt" do
expect { sandbox.exec "chmod", "4000", HOMEBREW_PREFIX }.to raise_error(ErrorDuringExecution)
expect { sandbox.run "chmod", "4000", HOMEBREW_PREFIX }.to raise_error(ErrorDuringExecution)
end
it "allows chmod 4000 on a path allowed to write" do
mktmpdir do |path|
FileUtils.touch path/"foo"
sandbox.allow_write_path(path)
expect { sandbox.exec "chmod", "4000", path/"foo" }.not_to raise_error(ErrorDuringExecution)
expect { sandbox.run "chmod", "4000", path/"foo" }.not_to raise_error(ErrorDuringExecution)
end
end
end

View File

@ -6,33 +6,37 @@ require "socket"
module Utils
def self.rewrite_child_error(child_error)
error = if child_error.inner["cmd"] &&
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["cmd"] &&
child_error.inner_class == BuildError
inner_class = Object.const_get(child_error["json_class"])
error = if child_error["cmd"] && inner_class == ErrorDuringExecution
ErrorDuringExecution.new(child_error["cmd"],
status: child_error["status"],
output: child_error["output"])
elsif child_error["cmd"] && 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"])
elsif child_error.inner_class == Interrupt
BuildError.new(nil, child_error["cmd"], child_error["args"], child_error["env"])
elsif inner_class == Interrupt
Interrupt.new
else
# Everything other error in the child just becomes a RuntimeError.
RuntimeError.new(child_error.message)
RuntimeError.new <<~EOS
An exception occurred within a child process:
#{inner_class}: #{child_error["m"]}
EOS
end
error.set_backtrace child_error.backtrace
error.set_backtrace child_error["b"]
error
end
def self.safe_fork
# When using this function, remember to call `exec` as soon as reasonably possible.
# This function does not protect against the pitfalls of what you can do pre-exec in a fork.
# See `man fork` for more information.
def self.safe_fork(directory: nil, yield_parent: false)
require "json/add/exception"
Dir.mktmpdir("homebrew", HOMEBREW_TEMP) do |tmpdir|
block = proc do |tmpdir|
UNIXServer.open("#{tmpdir}/socket") do |server|
read, write = IO.pipe
@ -78,6 +82,8 @@ module Utils
pid = T.must(pid)
begin
yield(nil) if yield_parent
begin
socket = server.accept_nonblock
rescue Errno::EAGAIN, Errno::EWOULDBLOCK, Errno::ECONNABORTED, Errno::EPROTO, Errno::EINTR
@ -100,14 +106,17 @@ module Utils
if data.present?
error_hash = JSON.parse(T.must(data.lines.first))
e = ChildProcessError.new(error_hash)
raise rewrite_child_error(e)
raise rewrite_child_error(error_hash)
end
raise "Forked child process failed: #{$CHILD_STATUS}" unless $CHILD_STATUS.success?
raise ChildProcessError, $CHILD_STATUS unless $CHILD_STATUS.success?
end
end
if directory
block.call(directory)
else
Dir.mktmpdir("homebrew-fork", HOMEBREW_TEMP, &block)
end
end
end