Skip to content

Commit

Permalink
Protect users against vulnerable logmailers
Browse files Browse the repository at this point in the history
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
  • Loading branch information
philwo committed Sep 7, 2023
1 parent 6fbc93a commit 8b6a3bf
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 3 deletions.
3 changes: 3 additions & 0 deletions src/glog/logging.h.in
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
5 changes: 4 additions & 1 deletion src/googletest.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
64 changes: 62 additions & 2 deletions src/logging.cc
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
#include <vector>
#include <cerrno> // for errno
#include <sstream>
#include <regex>
#ifdef GLOG_OS_WINDOWS
#include "windows/dirent.h"
#else
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
28 changes: 28 additions & 0 deletions src/logging_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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("[email protected]", "Example subject", "Example body"));
}

TEST(EmailLogging, MultipleAddresses) {
FlagSaver saver;
FLAGS_logmailer = "/usr/bin/true";

EXPECT_TRUE(SendEmail("[email protected],[email protected]", "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/[email protected]", "Example subject", "Example body"));
}

0 comments on commit 8b6a3bf

Please sign in to comment.