From 8b6a3bf5a681a2550b255bff61d63b8a64a88ef7 Mon Sep 17 00:00:00 2001 From: Philipp Wollermann Date: Thu, 3 Aug 2023 12:51:34 +0200 Subject: [PATCH 1/3] Protect users against vulnerable logmailers 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 | 64 +++++++++++++++++++++++++++++++++++++++-- src/logging_unittest.cc | 28 ++++++++++++++++++ 4 files changed, 97 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..e58427679 100644 --- a/src/logging.cc +++ b/src/logging.cc @@ -59,6 +59,7 @@ #include #include // for errno #include +#include #ifdef GLOG_OS_WINDOWS #include "windows/dirent.h" #else @@ -2207,13 +2208,72 @@ static string ShellEscape(const string& src) { } #endif +// Trim whitespace from the start of the provided string. +static inline void ltrim(std::string &s) { + s.erase(s.begin(), std::find_if(s.begin(), s.end(), [](char ch) { + return std::isspace(ch) == 0; + })); +} + +// Trim whitespace from the end of the provided string. +static inline void rtrim(std::string &s) { + s.erase(std::find_if(s.rbegin(), s.rend(), [](char ch) { + return std::isspace(ch) == 0; + }).base(), s.end()); +} + +// Trim whitespace from both ends of the provided string. +static inline void trim(std::string &s) { + rtrim(s); + ltrim(s); +} + // use_logging controls whether the logging functions LOG/VLOG are used // to log errors. It should be set to false when the caller holds the // log_mutex. static bool SendEmailInternal(const char*dest, const char *subject, const char*body, bool use_logging) { #ifndef GLOG_OS_EMSCRIPTEN + // 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 + const std::regex pattern("^[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 (dest && *dest) { + // Split the comma-separated list of email addresses, validate each one and + // build a sanitized new comma-separated string without whitespace. + std::stringstream ss(dest); + std::ostringstream sanitized_dests; + std::string s; + while (std::getline(ss, s, ',')) { + trim(s); + if (s.empty()) { + continue; + } + if (!std::regex_match(s, pattern)) { + 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 +2295,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")); +} From fde9ddcaee9a3b88adb9f721cb4a38e9d0313ec5 Mon Sep 17 00:00:00 2001 From: Philipp Wollermann Date: Fri, 18 Aug 2023 15:47:19 +0900 Subject: [PATCH 2/3] Address reviewer comments --- src/logging.cc | 51 +++++++++++++++++++------------------------------- 1 file changed, 19 insertions(+), 32 deletions(-) diff --git a/src/logging.cc b/src/logging.cc index e58427679..404afc55f 100644 --- a/src/logging.cc +++ b/src/logging.cc @@ -2206,27 +2206,14 @@ static string ShellEscape(const string& src) { } return result; } -#endif - -// Trim whitespace from the start of the provided string. -static inline void ltrim(std::string &s) { - s.erase(s.begin(), std::find_if(s.begin(), s.end(), [](char ch) { - return std::isspace(ch) == 0; - })); -} - -// Trim whitespace from the end of the provided string. -static inline void rtrim(std::string &s) { - s.erase(std::find_if(s.rbegin(), s.rend(), [](char ch) { - return std::isspace(ch) == 0; - }).base(), s.end()); -} // Trim whitespace from both ends of the provided string. static inline void trim(std::string &s) { - rtrim(s); - ltrim(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 // to log errors. It should be set to false when the caller holds the @@ -2234,23 +2221,10 @@ static inline void trim(std::string &s) { static bool SendEmailInternal(const char*dest, const char *subject, const char*body, bool use_logging) { #ifndef GLOG_OS_EMSCRIPTEN - // 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 - const std::regex pattern("^[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 (dest && *dest) { // Split the comma-separated list of email addresses, validate each one and // build a sanitized new comma-separated string without whitespace. - std::stringstream ss(dest); + std::istringstream ss(dest); std::ostringstream sanitized_dests; std::string s; while (std::getline(ss, s, ',')) { @@ -2258,7 +2232,20 @@ static bool SendEmailInternal(const char*dest, const char *subject, if (s.empty()) { continue; } - if (!std::regex_match(s, pattern)) { + // 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 { From 0bad5a5624827f7f21d19ce578121a040277a789 Mon Sep 17 00:00:00 2001 From: Philipp Wollermann Date: Thu, 7 Sep 2023 20:11:01 +0900 Subject: [PATCH 3/3] Add missing header for std::isspace --- src/logging.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/logging.cc b/src/logging.cc index 404afc55f..0a5df057b 100644 --- a/src/logging.cc +++ b/src/logging.cc @@ -60,6 +60,7 @@ #include // for errno #include #include +#include // for std::isspace #ifdef GLOG_OS_WINDOWS #include "windows/dirent.h" #else