From ad376468301499df5f20111d8184534b0a2083de Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 28 Sep 2026 17:28:38 -0400 Subject: [PATCH] Harden bin/example-image and bin/load against shell injection `bin/example-image` ran `git ls-files` through a shell with the checkout path in the command string, and `bin/load` put its scenario, duration and thread count into a `bash -c` string run in the driver container. A value containing shell syntax ran as a command. Pass these values to the commands as arguments. [Fix #33] --- CHANGELOG.md | 1 + bin/example-image | 2 +- bin/load | 6 ++--- examples/gate | 68 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6dd7a8e..f4f91f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ Some actions that application developers should consider taking when upgrading f #### Fixed * `bin/conformance` no longer fails intermittently at "offered overload answers capacity" against a healthy cell. A worker still cleaning up after the previous check could take one of the places the overload check fills, so the cell never filled. The check now waits for the cell to go idle first. +* `bin/example-image` and `bin/load` no longer pass their inputs through a shell. Before, a checkout path that contained shell syntax ran as a command in `bin/example-image`. A `bin/load` scenario, duration or thread count that contained shell syntax ran as a command in the driver container. Now the scripts pass these values as arguments. (#33) ## v0.5.0 / 2026-09-09 diff --git a/bin/example-image b/bin/example-image index d204090..bf6b8dc 100755 --- a/bin/example-image +++ b/bin/example-image @@ -57,7 +57,7 @@ FileUtils.cp Dir.glob(File.join(REPO, "examples", "operations", "*.rb")), # Dockerfile has to copy them in before `bundle install` runs. puts "pointing the cell's Gemfile at this checkout" GEMS.each do |gem| - files = `git -C #{REPO} ls-files #{gem}`.lines(chomp: true) + files = IO.popen([ "git", "-C", REPO, "ls-files", "--", gem ], &:read).lines(chomp: true) abort "git listed no files under #{gem}" if files.empty? files.each do |file| diff --git a/bin/load b/bin/load index 996842b..e12ab01 100755 --- a/bin/load +++ b/bin/load @@ -59,11 +59,11 @@ if ! docker run --rm \ --env HOTCELL_REPO=/repo \ --env BUNDLE_GEMFILE=/tmp/driver/Gemfile \ --env BUNDLE_PATH=/tmp/driver/bundle \ - "$DRIVER_IMAGE" bash -c " + "$DRIVER_IMAGE" bash -c ' install -D /repo/examples/Gemfile /tmp/driver/Gemfile && bundle install --quiet && - bundle exec ruby /repo/examples/load $SCENARIO $SECONDS_ARG $THREADS - " + bundle exec ruby /repo/examples/load "$@" + ' bash "$SCENARIO" "$SECONDS_ARG" "$THREADS" then echo "FAIL. Cell log tail:" docker logs --tail 20 "$CELL" 2>&1 | sed 's/^/ /' diff --git a/examples/gate b/examples/gate index 0532b53..556239e 100755 --- a/examples/gate +++ b/examples/gate @@ -22,10 +22,17 @@ # still hold a place when the overload check starts, and the cell refuses a sleep request sent then. # This test runs the overload check against a fake cell that holds such a leftover worker. # +# 4. bin/example-image and bin/load must pass their inputs to commands as arguments, never through a +# shell. Each runs here against a fake docker, with a checkout path or arguments that run a command +# if a shell parses them. +# # The container conformance run and its negative jobs (see .github/workflows/ci.yml) are the end-to-end # proof; this is the fast guard that keeps the gate honest without Docker. require "bundler/setup" +require "fileutils" +require "open3" +require "tmpdir" require_relative "lib/battery" REPO = File.expand_path("..", __dir__) @@ -182,6 +189,67 @@ ensure Examples::Sleep.singleton_class.send(:remove_method, :perform_in_hotcell) end +# Each name runs a command, and so creates `injected` in the working directory, only if a shell parses it. +INJECTION = "$(touch injected)" + +def fake_command(bin, name, script) + path = File.join(bin, name) + File.write path, "#!/usr/bin/env bash\n#{script}\n" + File.chmod 0o755, path +end + +check("bin/example-image lists the gems in a checkout whose path a shell would parse") do + Dir.mktmpdir do |tmp| + checkout = File.join(tmp, "hot cell #{INJECTION}") + files = [ "bin/example-image", "VERSION", *Dir.glob("examples/operations/*.rb", base: REPO) ] + files += Open3.capture2("git", "-C", REPO, "ls-files", "--", "hotcell-core", "hotcell-server").first.lines(chomp: true) + files.each do |file| + FileUtils.mkdir_p File.join(checkout, File.dirname(file)) + FileUtils.cp File.join(REPO, file), File.join(checkout, file) + end + _, status = Open3.capture2e("git", "init", "--quiet", checkout) + raise "git init failed" unless status.success? + _, status = Open3.capture2e("git", "-C", checkout, "add", ".") + raise "git add failed" unless status.success? + + bin = File.join(tmp, "bin") + FileUtils.mkdir_p bin + fake_command bin, "docker", "exit 0" + + output, status = Open3.capture2e({ "PATH" => "#{bin}:#{ENV["PATH"]}" }, + RbConfig.ruby, File.join(checkout, "bin/example-image"), chdir: tmp) + raise "a shell ran the command in the checkout path" if File.exist?(File.join(tmp, "injected")) + raise "it failed:\n#{output}" unless status.success? + end +end + +check("bin/load passes the scenario, duration and thread count to the driver as arguments") do + arguments = [ "echo #{INJECTION}", "5 #{INJECTION}", "2 #{INJECTION}" ] + + Dir.mktmpdir do |tmp| + bin = File.join(tmp, "bin") + FileUtils.mkdir_p bin + # The driver container's `bash -c` runs here, so what reaches examples/load is what bundle receives. + fake_command bin, "docker", <<~BASH + [ "$1 $2" = "run --rm" ] || exit 0 + while [ "$1" != bash ]; do shift; done + exec "$@" + BASH + fake_command bin, "install", "exit 0" + fake_command bin, "bundle", %(printf '%s\\n' "$@" >> "#{tmp}/bundle-arguments") + + output, status = Open3.capture2e({ "PATH" => "#{bin}:#{ENV["PATH"]}" }, + File.join(REPO, "bin/load"), "hotcell:example", *arguments, + chdir: tmp) + raise "a shell ran a command in the arguments" if File.exist?(File.join(tmp, "injected")) + raise "it failed:\n#{output}" unless status.success? + + load = File.readlines(File.join(tmp, "bundle-arguments"), chomp: true).drop_while { |argument| argument != "exec" } + expected = [ "exec", "ruby", "/repo/examples/load", *arguments ] + raise "bundle received #{load.inspect}" unless load == expected + end +end + if FAILURES.empty? puts "PASS" else