From 0ec5fd5262647633399e856b0fba93735f6d6c11 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 15 Apr 2021 16:29:37 +0100 Subject: [PATCH] DEV: Raise exception when execute_command will spawn a shell (#12716) --- lib/discourse.rb | 8 +++++++- spec/components/discourse_spec.rb | 15 +++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/lib/discourse.rb b/lib/discourse.rb index b13f785eb71..34aca6dca5e 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -95,10 +95,16 @@ module Discourse private - def execute_command(*command, timeout: nil, failure_message: "", success_status_codes: [0], chdir: ".") + def execute_command(*command, timeout: nil, failure_message: "", success_status_codes: [0], chdir: ".", unsafe_shell: false) env = nil env = command.shift if command[0].is_a?(Hash) + if !unsafe_shell && (command.length == 1) && command[0].include?(" ") + # Sending a single string to Process.spawn will launch a shell + # This means various things (e.g. subshells) are possible, and could present injection risk + raise "Arguments should be provided as separate strings" + end + if timeout # will send a TERM after timeout # will send a KILL after timeout * 2 diff --git a/spec/components/discourse_spec.rb b/spec/components/discourse_spec.rb index cc05124e250..99c458f2144 100644 --- a/spec/components/discourse_spec.rb +++ b/spec/components/discourse_spec.rb @@ -438,6 +438,21 @@ describe Discourse do has_checked_chdir = true thread.join end + + it "raises error for unsafe shell" do + expect(Discourse::Utils.execute_command("pwd").strip).to eq(Rails.root.to_s) + + expect do + Discourse::Utils.execute_command("echo a b c") + end.to raise_error(RuntimeError) + + expect do + Discourse::Utils.execute_command({ "ENV1" => "VAL" }, "echo a b c") + end.to raise_error(RuntimeError) + + expect(Discourse::Utils.execute_command("echo", "a", "b", "c").strip).to eq("a b c") + expect(Discourse::Utils.execute_command("echo a b c", unsafe_shell: true).strip).to eq("a b c") + end end end