From 0a0ed65306a3ae727a9e935fc36b52ffeab53a5d Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Tue, 21 Sep 2021 15:16:49 -0700 Subject: [PATCH] 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 | 15 ++++++++++ VERSION | 2 +- scripts/base/frameworks/logging/main.zeek | 2 +- .../logging/postprocessors/scp.zeek | 2 +- .../logging/postprocessors/sftp.zeek | 2 +- scripts/base/frameworks/notice/main.zeek | 5 ++-- scripts/policy/misc/trim-trace-file.zeek | 2 +- .../frameworks/logging/rotate-sanitize.zeek | 29 +++++++++++++++++++ 8 files changed, 52 insertions(+), 7 deletions(-) create mode 100644 testing/btest/scripts/base/frameworks/logging/rotate-sanitize.zeek diff --git a/CHANGES b/CHANGES index 28c4d98dfa..bb53f9f19f 100644 --- a/CHANGES +++ b/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) diff --git a/VERSION b/VERSION index fe61035511..f552076e42 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -4.2.0-dev.201 +4.2.0-dev.203 diff --git a/scripts/base/frameworks/logging/main.zeek b/scripts/base/frameworks/logging/main.zeek index 40dd8d1fe3..6c57173a47 100644 --- a/scripts/base/frameworks/logging/main.zeek +++ b/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)); diff --git a/scripts/base/frameworks/logging/postprocessors/scp.zeek b/scripts/base/frameworks/logging/postprocessors/scp.zeek index 22adc29e47..879801d9f8 100644 --- a/scripts/base/frameworks/logging/postprocessors/scp.zeek +++ b/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; } diff --git a/scripts/base/frameworks/logging/postprocessors/sftp.zeek b/scripts/base/frameworks/logging/postprocessors/sftp.zeek index 75ab438809..c4eef9dd18 100644 --- a/scripts/base/frameworks/logging/postprocessors/sftp.zeek +++ b/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; } diff --git a/scripts/base/frameworks/notice/main.zeek b/scripts/base/frameworks/notice/main.zeek index ebf3e29424..5908746b7b 100644 --- a/scripts/base/frameworks/notice/main.zeek +++ b/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; } diff --git a/scripts/policy/misc/trim-trace-file.zeek b/scripts/policy/misc/trim-trace-file.zeek index 81f54e991f..f702e9027c 100644 --- a/scripts/policy/misc/trim-trace-file.zeek +++ b/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) }; diff --git a/testing/btest/scripts/base/frameworks/logging/rotate-sanitize.zeek b/testing/btest/scripts/base/frameworks/logging/rotate-sanitize.zeek new file mode 100644 index 0000000000..70dab34be3 --- /dev/null +++ b/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]); + }