From 64827572c0612e6eb15ed324d8c4fa020a0c5a41 Mon Sep 17 00:00:00 2001 From: Philipp Wollermann Date: Fri, 8 Sep 2023 00:31:25 +0900 Subject: [PATCH] SendEmail: Protect users against vulnerable logmailers (#939) glog is used on a variety of systems, and we must assume that some of them still use vulnerable mailers that have bugs or "interesting features" such as https://nvd.nist.gov/vuln/detail/CVE-2004-2771. Let's protect users against accidental shell injection by validating the email addresses against a slightly stricter version of the regex used by HTML5 to validate addresses[1]. This should prevent triggering any unexpected behavior in these tools. Also add some basic unit tests for the SendEmail method. [1] https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address --- src/glog/logging.h.in | 3 +++ src/googletest.h | 5 +++- src/logging.cc | 52 +++++++++++++++++++++++++++++++++++++++-- src/logging_unittest.cc | 28 ++++++++++++++++++++++ 4 files changed, 85 insertions(+), 3 deletions(-) diff --git a/src/glog/logging.h.in b/src/glog/logging.h.in index 26fd371d2..812fc92af 100644 --- a/src/glog/logging.h.in +++ b/src/glog/logging.h.in @@ -470,6 +470,9 @@ DECLARE_bool(stop_logging_if_full_disk); // Use UTC time for logging DECLARE_bool(log_utc_time); +// Mailer used to send logging email +DECLARE_string(logmailer); + // Log messages below the GOOGLE_STRIP_LOG level will be compiled away for // security reasons. See LOG(severtiy) below. diff --git a/src/googletest.h b/src/googletest.h index a12c38763..ff910a844 100644 --- a/src/googletest.h +++ b/src/googletest.h @@ -560,17 +560,20 @@ struct FlagSaver { : v_(FLAGS_v), stderrthreshold_(FLAGS_stderrthreshold), logtostderr_(FLAGS_logtostderr), - alsologtostderr_(FLAGS_alsologtostderr) {} + alsologtostderr_(FLAGS_alsologtostderr), + logmailer_(FLAGS_logmailer) {} ~FlagSaver() { FLAGS_v = v_; FLAGS_stderrthreshold = stderrthreshold_; FLAGS_logtostderr = logtostderr_; FLAGS_alsologtostderr = alsologtostderr_; + FLAGS_logmailer = logmailer_; } int v_; int stderrthreshold_; bool logtostderr_; bool alsologtostderr_; + std::string logmailer_; }; #endif diff --git a/src/logging.cc b/src/logging.cc index 53b485c0b..0a5df057b 100644 --- a/src/logging.cc +++ b/src/logging.cc @@ -59,6 +59,8 @@ #include #include // for errno #include +#include +#include // for std::isspace #ifdef GLOG_OS_WINDOWS #include "windows/dirent.h" #else @@ -2205,6 +2207,13 @@ static string ShellEscape(const string& src) { } return result; } + +// Trim whitespace from both ends of the provided string. +static inline void trim(std::string &s) { + const auto toRemove = [](char ch) { return std::isspace(ch) == 0; }; + s.erase(s.begin(), std::find_if(s.begin(), s.end(), toRemove)); + s.erase(std::find_if(s.rbegin(), s.rend(), toRemove).base(), s.end()); +} #endif // use_logging controls whether the logging functions LOG/VLOG are used @@ -2214,6 +2223,45 @@ static bool SendEmailInternal(const char*dest, const char *subject, const char*body, bool use_logging) { #ifndef GLOG_OS_EMSCRIPTEN if (dest && *dest) { + // Split the comma-separated list of email addresses, validate each one and + // build a sanitized new comma-separated string without whitespace. + std::istringstream ss(dest); + std::ostringstream sanitized_dests; + std::string s; + while (std::getline(ss, s, ',')) { + trim(s); + if (s.empty()) { + continue; + } + // We validate the provided email addresses using the same regular + // expression that HTML5 uses[1], except that we require the address to + // start with an alpha-numeric character. This is because we don't want to + // allow email addresses that start with a special character, such as a + // pipe or dash, which could be misunderstood as a command-line flag by + // certain versions of `mail` that are vulnerable to command injection.[2] + // [1] https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address + // [2] e.g. https://nvd.nist.gov/vuln/detail/CVE-2004-2771 + if (!std::regex_match( + s, + std::regex("^[a-zA-Z0-9]" + "[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]*@[a-zA-Z0-9]" + "(?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\\.[a-zA-Z0-9]" + "(?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$"))) { + if (use_logging) { + VLOG(1) << "Invalid destination email address:" << s; + } else { + fprintf(stderr, "Invalid destination email address: %s\n", + s.c_str()); + } + return false; + } + if (!sanitized_dests.str().empty()) { + sanitized_dests << ","; + } + sanitized_dests << s; + } + dest = sanitized_dests.str().c_str(); + if ( use_logging ) { VLOG(1) << "Trying to send TITLE:" << subject << " BODY:" << body << " to " << dest; @@ -2235,8 +2283,8 @@ static bool SendEmailInternal(const char*dest, const char *subject, FILE* pipe = popen(cmd.c_str(), "w"); if (pipe != nullptr) { - // Add the body if we have one - if (body) { + // Add the body if we have one + if (body) { fwrite(body, sizeof(char), strlen(body), pipe); } bool ok = pclose(pipe) != -1; diff --git a/src/logging_unittest.cc b/src/logging_unittest.cc index 85e851674..fff766070 100644 --- a/src/logging_unittest.cc +++ b/src/logging_unittest.cc @@ -1489,3 +1489,31 @@ TEST(LogMsgTime, gmtoff) { const long utc_max_offset = 50400; EXPECT_TRUE( (nGmtOff >= utc_min_offset) && (nGmtOff <= utc_max_offset) ); } + +TEST(EmailLogging, ValidAddress) { + FlagSaver saver; + FLAGS_logmailer = "/usr/bin/true"; + + EXPECT_TRUE(SendEmail("example@example.com", "Example subject", "Example body")); +} + +TEST(EmailLogging, MultipleAddresses) { + FlagSaver saver; + FLAGS_logmailer = "/usr/bin/true"; + + EXPECT_TRUE(SendEmail("example@example.com,foo@bar.com", "Example subject", "Example body")); +} + +TEST(EmailLogging, InvalidAddress) { + FlagSaver saver; + FLAGS_logmailer = "/usr/bin/true"; + + EXPECT_FALSE(SendEmail("hello world@foo", "Example subject", "Example body")); +} + +TEST(EmailLogging, MaliciousAddress) { + FlagSaver saver; + FLAGS_logmailer = "/usr/bin/true"; + + EXPECT_FALSE(SendEmail("!/bin/true@example.com", "Example subject", "Example body")); +}