Deprecate str_shell_escape, add safe_shell_quote replacement

CHANGES

@@ -1,4 +1,8 @@
 
+2.6-192 | 2019-03-25 17:49:18 -0700
+
+  * Deprecate str_shell_escape, add safe_shell_quote replacement (Jon Siwek, Corelight)
+
 2.6-191 | 2019-03-25 16:43:10 -0700
 
   * Add support for SMB filenames to the intel framework (Stephen Hosom)

NEWS

@@ -106,6 +106,10 @@ Removed Functionality
 Deprecated Functionality
 ------------------------
 
+- The ``str_shell_escape` function is now deprecated, use ``safe_shell_quote``
+  instead.  The later will automatically return a value that is enclosed
+  in double-quotes.
+
 Bro 2.6
 =======
 

VERSION

@@ -1 +1 @@
-2.6-191
+2.6-192

doc

@@ -1 +1 @@
-Subproject commit 406d0c857491927d7fbee7aef954b8a40f23978d
+Subproject commit e404fc80c5c4ecfd0c4441b6b83826761bd985e9

scripts/base/utils/active-http.bro

@@ -57,10 +57,10 @@ export {
 
 function request2curl(r: Request, bodyfile: string, headersfile: string): string
 	{
-	local cmd = fmt("curl -s -g -o \"%s\" -D \"%s\" -X \"%s\"",
+	local cmd = fmt("curl -s -g -o %s -D %s -X %s",
-	                str_shell_escape(bodyfile),
+	                safe_shell_quote(bodyfile),
-	                str_shell_escape(headersfile),
+	                safe_shell_quote(headersfile),
-	                str_shell_escape(r$method));
+	                safe_shell_quote(r$method));
 
 	cmd = fmt("%s -m %.0f", cmd, r$max_time);
 
@@ -70,9 +70,9 @@ function request2curl(r: Request, bodyfile: string, headersfile: string): string
 	if ( r?$addl_curl_args )
 		cmd = fmt("%s %s", cmd, r$addl_curl_args);
 
-	cmd = fmt("%s \"%s\"", cmd, str_shell_escape(r$url));
+	cmd = fmt("%s %s", cmd, safe_shell_quote(r$url));
 	# Make sure file will exist even if curl did not write one.
-	cmd = fmt("%s && touch %s", cmd, str_shell_escape(bodyfile));
+	cmd = fmt("%s && touch %s", cmd, safe_shell_quote(bodyfile));
 	return cmd;
 	}
 

scripts/base/utils/dir.bro

@@ -28,7 +28,7 @@ event Dir::monitor_ev(dir: string, last_files: set[string],
                       callback: function(fname: string),
                       poll_interval: interval)
 	{
-	when ( local result = Exec::run([$cmd=fmt("ls -1 \"%s/\"", str_shell_escape(dir))]) )
+	when ( local result = Exec::run([$cmd=fmt("ls -1 %s/", safe_shell_quote(dir))]) )
 		{
 		if ( result$exit_code != 0 )
 			{

scripts/base/utils/exec.bro

@@ -8,7 +8,7 @@ export {
 	type Command: record {
 		## The command line to execute.  Use care to avoid injection
 		## attacks (i.e., if the command uses untrusted/variable data,
-		## sanitize it with :bro:see:`str_shell_escape`).
+		## sanitize it with :bro:see:`safe_shell_quote`).
 		cmd:         string;
 		## Provide standard input to the program as a string.
 		stdin:       string      &default="";
@@ -122,7 +122,7 @@ event Input::end_of_data(orig_name: string, source:string)
 		delete pending_files[name][track_file];
 		if ( |pending_files[name]| == 0 )
 			delete pending_commands[name];
-		system(fmt("rm \"%s\"", str_shell_escape(track_file)));
+		system(fmt("rm %s", safe_shell_quote(track_file)));
 		}
 	}
 
@@ -191,5 +191,5 @@ event bro_done()
 	# We are punting here and just deleting any unprocessed files.
 	for ( uid in pending_files )
 		for ( fname in pending_files[uid] )
-			system(fmt("rm \"%s\"", str_shell_escape(fname)));
+			system(fmt("rm %s", safe_shell_quote(fname)));
 	}

src/bro.bif

@@ -440,13 +440,13 @@ static int do_system(const char* s)
 ## Invokes a command via the ``system`` function of the OS.
 ## The command runs in the background with ``stdout`` redirecting to
 ## ``stderr``. Here is a usage example:
-## ``system(fmt("rm \"%s\"", str_shell_escape(sniffed_data)));``
+## ``system(fmt("rm %s", safe_shell_quote(sniffed_data)));``
 ##
 ## str: The command to execute.
 ##
 ## Returns: The return value from the OS ``system`` function.
 ##
-## .. bro:see:: system_env str_shell_escape piped_exec
+## .. bro:see:: system_env safe_shell_quote piped_exec
 ##
 ## .. note::
 ##
@@ -472,7 +472,7 @@ function system%(str: string%): int
 ##
 ## Returns: The return value from the OS ``system`` function.
 ##
-## .. bro:see:: system str_shell_escape piped_exec
+## .. bro:see:: system safe_shell_quote piped_exec
 function system_env%(str: string, env: table_string_of_string%): int
 	%{
 	if ( env->Type()->Tag() != TYPE_TABLE )

src/strings.bif

@@ -1184,10 +1184,54 @@ function string_fill%(len: int, source: string%): string
 ##
 ## source: The string to escape.
 ##
+## Returns: A shell-escaped version of *source*.  Specifically, this
+##          backslash-escapes characters whose literal value is not otherwise
+##          preserved by enclosure in double-quotes (dollar-sign, backquote,
+##          backslash, and double-quote itself), and then encloses that
+##          backslash-escaped string in double-quotes to ultimately preserve
+##          the literal value of all input characters.
+##
+## .. bro:see:: system safe_shell_quote
+function safe_shell_quote%(source: string%): string
+	%{
+	unsigned j = 0;
+	const u_char* src = source->Bytes();
+	unsigned n = source->Len();
+	byte_vec dst = new u_char[n * 2 + 1 + 2];
+	dst[j++] = '"';
+
+	for ( unsigned i = 0; i < n; ++i )
+		{
+		switch ( src[i] ) {
+		case '`': case '"': case '\\': case '$':
+			dst[j++] = '\\';
+			break;
+		default:
+			break;
+		}
+
+		dst[j++] = src[i];
+		}
+
+	dst[j++] = '"';
+	dst[j] = '\0';
+	return new StringVal(new BroString(1, dst, j));
+	%}
+
+## Takes a string and escapes characters that would allow execution of
+## commands at the shell level. Must be used before including strings in
+## :bro:id:`system` or similar calls.  This function is deprecated, use
+## :bro:see:`safe_shell_quote` as a replacement.  The difference is that
+## :bro:see:`safe_shell_quote` automatically returns a value that is
+## wrapped in double-quotes, which is required to correctly and fully
+## escape any characters that might be interpreted by the shell.
+##
+## source: The string to escape.
+##
 ## Returns: A shell-escaped version of *source*.
 ##
-## .. bro:see:: system
+## .. bro:see:: system safe_shell_quote
-function str_shell_escape%(source: string%): string
+function str_shell_escape%(source: string%): string &deprecated
 	%{
 	unsigned j = 0;
 	const u_char* src = source->Bytes();

testing/btest/Baseline/bifs.safe_shell_quote/out

@@ -0,0 +1,2 @@
+echo `pwd` ${TEST} > "my file"; echo -e "\n"
+"echo \`pwd\` \${TEST} > \"my file\"; echo -e \"\\n\""

testing/btest/bifs/safe_shell_quote.bro

@@ -0,0 +1,12 @@
+#
+# @TEST-EXEC: bro -b %INPUT >out
+# @TEST-EXEC: btest-diff out
+
+event bro_init()
+	{
+	local a = "echo `pwd` ${TEST} > \"my file\"; echo -e \"\\n\"";
+	print a;
+
+	local b = safe_shell_quote(a);
+	print b;
+	}