From f48029fb50d1005af874254e655a38bfebbcb144 Mon Sep 17 00:00:00 2001 From: Robert Estelle Date: Wed, 29 Jul 2020 13:50:35 -0700 Subject: [PATCH] Fix dangerous command splicing --- src/swapper.rs | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/src/swapper.rs b/src/swapper.rs index 3b52277..0459ea4 100644 --- a/src/swapper.rs +++ b/src/swapper.rs @@ -298,8 +298,38 @@ impl<'a> Swapper<'a> { self.command.clone() }; - let final_command = str::replace(execute_command.as_str(), "{}", text.trim_end()); - let retrieve_command = vec!["bash", "-c", final_command.as_str()]; + // The command we run has two arguments: + // * The first arg is the (trimmed) text. This gets stored in a variable, in order to + // preserve quoting and special characters. + // + // * The second argument is the user's command, with the '{}' token replaced with an + // unquoted reference to the variable containing the text. + // + // The reference is unquoted, unfortunately, because the token may already have been + // spliced into a string (e.g 'tmux display-message "Copied {}"'), and it's impossible (or + // at least exceedingly difficult) to determine the correct quoting level. + // + // The alternative of literally splicing the text into the command is bad and it causes all + // kinds of harmful escaping issues that the user cannot reasonable avoid. + // + // For example, imagine some pattern matched the text "foo;rm *" and the user's command was + // an innocuous "echo {}". With literal splicing, we would run the command "echo foo;rm *". + // That's BAD. Without splicing, instead we execute "echo ${THUMB}" which does mostly the + // right thing regardless the contents of the text. (At worst, bash will word-separate the + // unquoted variable; but it won't _execute_ those words in common scenarios). + // + // Ideally user commands would just use "${THUMB}" to begin with rather than having any + // sort of ad-hoc string splicing here at all, and then they could specify the quoting they + // want, but that would break backwards compatibility. + let final_command = str::replace(execute_command.as_str(), "{}", "${THUMB}"); + let retrieve_command = vec![ + "bash", + "-c", + "THUMB=\"$1\"; eval \"$2\"", + "--", + text.trim_end(), + final_command.as_str(), + ]; let params = retrieve_command.iter().map(|arg| arg.to_string()).collect(); self.executor.execute(params);