Merge remote-tracking branch 'origin/topic/robin/gh-54-sanitize'

* origin/topic/robin/gh-54-sanitize:
  Sanitize log files names before they go into system().

CHANGES

@@ -1,3 +1,18 @@
+4.2.0-dev.203 | 2021-09-21 15:16:49 -0700
+
+  * Sanitize log files names before they go into system(). (Robin Sommer, Corelight)
+
+    In principle, an attacker who's controlling the Zeek scripts being
+    loaded could have set log paths to include non-safe characters leading
+    to arbitrary command execution during log rotation. This fix avoids that
+    be sanitizing the file names / command lines.
+
+    Note, though, that this isn't a problem that we can really solve:
+    somebody controlling scripts can just as well inject custom
+    `system()` calls to begin with.
+
+    Closes #54.
+
 4.2.0-dev.201 | 2021-09-21 15:15:57 -0700
 
   * PIA - switch size to int64_t (Johanna Amann, Corelight)

VERSION

@@ -1 +1 @@
-4.2.0-dev.201
+4.2.0-dev.203

scripts/base/frameworks/logging/main.zeek

@@ -649,7 +649,7 @@ function run_rotation_postprocessor_cmd(info: RotationInfo, npath: string) : boo
 	# The date format is hard-coded here to provide a standardized
 	# script interface.
 	system(fmt("%s %s %s %s %s %d %s",
-               pp_cmd, npath, info$path,
+               pp_cmd, safe_shell_quote(npath), safe_shell_quote(info$path),
                strftime("%y-%m-%d_%H.%M.%S", info$open),
                strftime("%y-%m-%d_%H.%M.%S", info$close),
                info$terminating, writer));

scripts/base/frameworks/logging/postprocessors/scp.zeek

@@ -66,7 +66,7 @@ function scp_postprocessor(info: Log::RotationInfo): bool
 		command += fmt("scp %s %s@%s:%s;", info$fname, d$user, d$host, dst);
 		}
 
-	command += fmt("/bin/rm %s", info$fname);
+	command += fmt("/bin/rm %s", safe_shell_quote(info$fname));
 	system(command);
 	return T;
 	}

scripts/base/frameworks/logging/postprocessors/sftp.zeek

@@ -69,7 +69,7 @@ function sftp_postprocessor(info: Log::RotationInfo): bool
 		               d$host_port, d$user, d$host);
 		}
 
-	command += fmt("/bin/rm %s", info$fname);
+	command += fmt("/bin/rm %s", safe_shell_quote(info$fname));
 	system(command);
 	return T;
 	}

scripts/base/frameworks/notice/main.zeek

@@ -384,12 +384,13 @@ function log_mailing_postprocessor(info: Log::RotationInfo): bool
 		{
 		local headers = email_headers(fmt("Log Contents: %s", info$fname),
 		                              mail_dest);
-		local tmpfilename = fmt("%s.mailheaders.tmp", info$fname);
+		local tmpfilename = safe_shell_quote(fmt("%s.mailheaders.tmp", info$fname));
 		local tmpfile = open(tmpfilename);
 		write_file(tmpfile, headers);
 		close(tmpfile);
 		system(fmt("/bin/cat %s %s | %s -t -oi && /bin/rm %s %s",
-		       tmpfilename, info$fname, sendmail, tmpfilename, info$fname));
+		       tmpfilename, safe_shell_quote(info$fname), sendmail,
+			   tmpfilename, safe_shell_quote(info$fname)));
 		}
 	return T;
 	}

scripts/policy/misc/trim-trace-file.zeek

@@ -24,7 +24,7 @@ event TrimTraceFile::go(first_trim: bool)
 		{
 		local info = rotate_file_by_name(trace_output_file);
 		if ( info$old_name != "" )
-			system(fmt("/bin/rm %s", info$new_name));
+			system(fmt("/bin/rm %s", safe_shell_quote(info$new_name)));
 		}
 	
 	schedule trim_interval { TrimTraceFile::go(F) };

testing/btest/scripts/base/frameworks/logging/rotate-sanitize.zeek

@@ -0,0 +1,29 @@
+# @TEST-EXEC: zeek -b -r ${TRACES}/rotation.trace %INPUT
+# @TEST-EXEC-FAIL: test -f must-not-exist
+
+module Test;
+
+export {
+	# Create a new ID for our log stream
+	redef enum Log::ID += { LOG };
+
+	# Define a record with all the columns the log file can have.
+	# (I'm using a subset of fields from ssh-ext for demonstration.)
+	type Log: record {
+		t: time;
+		id: conn_id; # Will be rolled out into individual columns.
+	} &log;
+}
+
+redef Log::default_rotation_interval = 1hr;
+redef Log::default_rotation_postprocessor_cmd = "echo";
+
+event zeek_init()
+{
+	Log::create_stream(Test::LOG, [$columns=Log, $path="; touch must-not-exist; true "]);
+}
+
+event new_connection(c: connection)
+	{
+	Log::write(Test::LOG, [$t=network_time(), $id=c$id]);
+	}