Merge #17095: util: Filter control characters out of log messages

d7820a1 util: Filter control characters out of log messages (Wladimir J. van der Laan)

Pull request description:

  Belts and suspenders: make sure outgoing log messages don't contain potentially suspicious characters, such as terminal control codes.

  This escapes control characters except newline ('\n') in C syntax. It escapes instead of removes them to still allow for troubleshooting issues where they accidentally end up in strings (it is a debug log, after all).

  (more checks could be added such as UTF-8 validity and unicode code-point range checking—this is substantially more involved and would need to keep track of state between characters and even `LogPrint` calls as they could end up split up—but escape codes seem to be the most common attack vector for terminals.)

ACKs for top commit:
  practicalswift:
    ACK d7820a1 - tested and works as expected :)

Tree-SHA512: 0806265addebdcec1062a6def3e903555e62ba5e93967ce9ee6943d16462a222b3f41135a5bff0a76966ae9e7ed75f211d7785bceda788ae0b0654bf3fd891bf

Author: laanwj

src/logging.cpp

@@ -224,10 +224,32 @@ std::string BCLog::Logger::LogTimestampStr(const std::string& str)
     return strStamped;
 }
 
+namespace BCLog {
+    /** Belts and suspenders: make sure outgoing log messages don't contain
+     * potentially suspicious characters, such as terminal control codes.
+     *
+     * This escapes control characters except newline ('\n') in C syntax.
+     * It escapes instead of removes them to still allow for troubleshooting
+     * issues where they accidentally end up in strings.
+     */
+    std::string LogEscapeMessage(const std::string& str) {
+        std::string ret;
+        for (char ch_in : str) {
+            uint8_t ch = (uint8_t)ch_in;
+            if ((ch >= 32 || ch == '\n') && ch != '\x7f') {
+                ret += ch_in;
+            } else {
+                ret += strprintf("\\x%02x", ch);
+            }
+        }
+        return ret;
+    }
+}
+
 void BCLog::Logger::LogPrintStr(const std::string& str)
 {
     std::lock_guard<std::mutex> scoped_lock(m_cs);
-    std::string str_prefixed = str;
+    std::string str_prefixed = LogEscapeMessage(str);
 
     if (m_log_threadnames && m_started_new_line) {
         str_prefixed.insert(0, "[" + util::ThreadGetInternalName() + "] ");

src/test/util_tests.cpp

@@ -26,6 +26,11 @@
 
 #include <boost/test/unit_test.hpp>
 
+/* defined in logging.cpp */
+namespace BCLog {
+    std::string LogEscapeMessage(const std::string& str);
+}
+
 BOOST_FIXTURE_TEST_SUITE(util_tests, BasicTestingSetup)
 
 BOOST_AUTO_TEST_CASE(util_criticalsection)
@@ -1696,4 +1701,17 @@ BOOST_AUTO_TEST_CASE(test_spanparsing)
     BOOST_CHECK_EQUAL(SpanToStr(results[3]), "");
 }
 
+BOOST_AUTO_TEST_CASE(test_LogEscapeMessage)
+{
+    // ASCII and UTF-8 must pass through unaltered.
+    BOOST_CHECK_EQUAL(BCLog::LogEscapeMessage("Valid log message貓"), "Valid log message貓");
+    // Newlines must pass through unaltered.
+    BOOST_CHECK_EQUAL(BCLog::LogEscapeMessage("Message\n with newlines\n"), "Message\n with newlines\n");
+    // Other control characters are escaped in C syntax.
+    BOOST_CHECK_EQUAL(BCLog::LogEscapeMessage("\x01\x7f Corrupted log message\x0d"), R"(\x01\x7f Corrupted log message\x0d)");
+    // Embedded NULL characters are escaped too.
+    const std::string NUL("O\x00O", 3);
+    BOOST_CHECK_EQUAL(BCLog::LogEscapeMessage(NUL), R"(O\x00O)");
+}
+
 BOOST_AUTO_TEST_SUITE_END()