Fix[mqb]: improve non-printable handling in negotiation/session logs

Restrict negotiation messages (and derived fields) to printable
characters in logs.

Additionally, add a BSLS_REVIEW check for negotiation messages
containing non-printable characters in anticipation of enforcing that
string fields are limited to printable ASCII characters.

Signed-off-by: Christopher Beard <cbeard9@bloomberg.net>

Author: chrisbeard

src/groups/bmq/bmqp/bmqp_ctrlmsg.xsd

@@ -1674,6 +1674,8 @@
         userAgent.........: a string identifying the agent the client is using
                             to connect.  Should include the SDK name/language
                             and version.
+
+        String fields must only contain printable ASCII characters.
       </documentation>
     </annotation>
     <sequence>

src/groups/bmq/bmqp/bmqp_ctrlmsg_messages.h

@@ -17,12 +17,14 @@
 
 #include <bmqscm_version.h>
 // BDE
+#include <bdlb_chartype.h>
 #include <bsl_algorithm.h>
 #include <bsl_bitset.h>
 #include <bsl_cctype.h>
 #include <bsl_climits.h>
 #include <bsl_functional.h>
 #include <bsls_assert.h>
+#include <bsls_performancehint.h>
 
 namespace BloombergLP {
 namespace bmqu {
@@ -117,6 +119,20 @@ bool StringUtil::endsWith(const bslstl::StringRef& str,
     return true;
 }
 
+bool StringUtil::isPrintable(const bslstl::StringRef& str)
+{
+    for (bslstl::StringRef::const_iterator it = str.begin(); it != str.end();
+         ++it) {
+        if (BSLS_PERFORMANCEHINT_PREDICT_UNLIKELY(
+                !bdlb::CharType::isPrint(*it))) {
+            BSLS_PERFORMANCEHINT_UNLIKELY_HINT;
+            return false;  // RETURN
+        }
+    }
+
+    return true;
+}
+
 bsl::string& StringUtil::trim(bsl::string* str)
 {
     return ltrim(&rtrim(str));

src/groups/bmq/bmqu/bmqu_stringutil.cpp

@@ -57,6 +57,10 @@ struct StringUtil {
     static bool endsWith(const bslstl::StringRef& str,
                          const bslstl::StringRef& suffix);
 
+    /// Return `true` if every character in the specified string `str` is
+    /// printable, and `false` otherwise.  An empty string is printable.
+    static bool isPrintable(const bslstl::StringRef& str);
+
     /// Perform an in-place white spaces trimming at the beginning and the
     /// end of the specified string `str` and return a reference offering
     /// modifiable access to it.

src/groups/bmq/bmqu/bmqu_stringutil.h

@@ -524,6 +524,60 @@ static void test9_squeeze()
     }
 }
 
+static void test10_isPrintable()
+// ------------------------------------------------------------------------
+// bmqu::StringUtil::isPrintable
+//
+// Concerns:
+//   Ensure proper behavior of the 'isPrintable' method.
+//
+// Plan:
+//   Test printable strings and the empty string, plus strings containing
+//   various non-printable bytes (control characters, DEL, and high-bit
+//   bytes), including one with an embedded NUL.
+//
+// Testing:
+//   Proper behavior of the 'isPrintable(str)' method.
+// ------------------------------------------------------------------------
+{
+    bmqtst::TestHelper::printTestName("isPrintable");
+
+    struct Test {
+        int         d_line;
+        const char* d_str;
+        bool        d_result;
+    } k_DATA[] = {{L_, "", true},
+                  {L_, "hello world", true},
+                  {L_, " ", true},  // 0x20, first printable
+                  {L_, "~", true},  // 0x7e, last printable
+                  {L_, "abc\ndef", false},
+                  {L_, "abc\tdef", false},
+                  {L_, "\r", false},
+                  {L_, "\x1f", false},   // just below first printable
+                  {L_, "\x7f", false},   // DEL
+                  {L_, "\x80", false},   // high-bit byte
+                  {L_, "\xff", false}};  // high-bit byte
+
+    const size_t k_NUM_DATA = sizeof(k_DATA) / sizeof(*k_DATA);
+
+    for (size_t idx = 0; idx < k_NUM_DATA; ++idx) {
+        const Test& test = k_DATA[idx];
+
+        PVV(test.d_line << ": checking printability of line");
+        BMQTST_ASSERT_EQ_D("line " << test.d_line,
+                           bmqu::StringUtil::isPrintable(test.d_str),
+                           test.d_result);
+    }
+
+    // A NUL embedded mid-string must be detected, so pass an explicit length
+    // rather than relying on NUL-termination.
+    const char k_EMBEDDED_NUL[] = {'a', '\0', 'b'};
+    BMQTST_ASSERT_EQ(
+        bmqu::StringUtil::isPrintable(
+            bslstl::StringRef(k_EMBEDDED_NUL, sizeof(k_EMBEDDED_NUL))),
+        false);
+}
+
 // ============================================================================
 //                                 MAIN PROGRAM
 // ----------------------------------------------------------------------------
@@ -534,6 +588,7 @@ int main(int argc, char* argv[])
 
     switch (_testCase) {
     case 0:
+    case 10: test10_isPrintable(); break;
     case 9: test9_squeeze(); break;
     case 8: test8_match(); break;
     case 7: test7_strTokenizeRef(); break;

src/groups/bmq/bmqu/bmqu_stringutil.t.cpp

@@ -73,10 +73,12 @@
 #include <bmqst_statcontext.h>
 #include <bmqu_blob.h>
 #include <bmqu_memoutstream.h>
+#include <bmqu_stringutil.h>
 #include <bmqu_time.h>
 
 // BDE
 #include <ball_log.h>
+#include <bdlb_chartype.h>
 #include <bdlf_bind.h>
 #include <bdlf_placeholder.h>
 #include <bdlma_localsequentialallocator.h>
@@ -88,6 +90,8 @@
 #include <bslma_managedptr.h>
 #include <bsls_assert.h>
 #include <bsls_atomic.h>
+#include <bsls_performancehint.h>
+#include <bsls_review.h>
 #include <bsls_timeinterval.h>
 
 // NTC
@@ -196,19 +200,60 @@ void loadBrokerIdentity(bmqp_ctrlmsg::ClientIdentity* identity,
     identity->clusterNodeId() = nodeId;
 }
 
+/// Replace every non-printable character in `*str` with `?`, in place.
+void sanitize(bsl::string* str)
+{
+    // PRECONDITIONS
+    BSLS_ASSERT(str);
+
+    for (bsl::string::iterator it = str->begin(); it != str->end(); ++it) {
+        if (BSLS_PERFORMANCEHINT_PREDICT_UNLIKELY(
+                !bdlb::CharType::isPrint(*it))) {
+            BSLS_PERFORMANCEHINT_UNLIKELY_HINT;
+            *it = '?';
+        }
+    }
+}
+
+/// Return the streamed representation of `obj`, safe to log (non-printable
+/// characters replaced with `?`).
+template <class TYPE>
+bsl::string logSafe(const TYPE& obj)
+{
+    bmqu::MemOutStream os;
+    os << obj;
+
+    bsl::string result(os.str().data(), os.str().length());
+    sanitize(&result);
+    return result;
+}
+
+/// Return `true` if the streamed representation of `obj` contains only
+/// printable characters, and `false` otherwise.
+template <class TYPE>
+bool isPrintable(const TYPE& obj)
+{
+    bmqu::MemOutStream os;
+    os << obj;
+
+    return bmqu::StringUtil::isPrintable(os.str());
+}
+
 /// Load in the specified `out` the short description representing the
 /// specified `identity` from the specified `peerChannel`.  The format is as
 /// follow:
 ///    tskName:pid.sessionId[\@hostId]
 /// Where:
-///  - tskName   : the task name, without any optional leading path
+///  - tskName   : the process/task name, without any optional leading path
 ///  - pid       : the pid of the task
 ///  - sessionId : the sessionId, omitted if 1
 ///  - hostId    : the identity of the host where the peer is running
 ///                (omitted if local), note that the format is either
 ///                `ip:port`, or `ip~resolvedHostname:port` depending on
 ///                whether the async DNS resolution already took place or
 ///                not.
+///
+/// Note: the result is sanitized as it derives from untrusted input.
 void loadSessionDescription(bsl::string*                        out,
                             const bmqp_ctrlmsg::ClientIdentity& identity,
                             const bmqio::Channel&               peerChannel)
@@ -246,6 +291,7 @@ void loadSessionDescription(bsl::string*                        out,
     }
 
     out->assign(os.str().data(), os.str().length());
+    sanitize(out);
 }
 }  // close unnamed namespace
 
@@ -275,6 +321,10 @@ int SessionNegotiator::createSessionOnMsgType(
     const NegotiationContextSp& negotiationContext =
         context_p->negotiationContext();
 
+    // Detect non-printable characters in negotiation messages.
+    // TODO: fail negotiation if message contains non-printable characters
+    BSLS_REVIEW_OPT(isPrintable(negotiationContext->negotiationMessage()));
+
     switch (negotiationContext->negotiationMessage().selectionId()) {
     case bmqp_ctrlmsg::NegotiationMessage::SELECTION_INDEX_CLIENT_IDENTITY: {
         // This is the first message of the negotiation protocol; can either
@@ -371,7 +421,8 @@ bsl::shared_ptr<mqbnet::Session> SessionNegotiator::onClientIdentityMessage(
         negotiationMessage.clientIdentity();
 
     BALL_LOG_INFO << "Handle negotiation message received from '"
-                  << context_p->channel().get() << "': " << clientIdentity;
+                  << context_p->channel().get()
+                  << "': " << logSafe(clientIdentity);
 
     bsl::shared_ptr<mqbnet::Session> session;
 
@@ -389,7 +440,7 @@ bsl::shared_ptr<mqbnet::Session> SessionNegotiator::onClientIdentityMessage(
     case bmqp_ctrlmsg::ClientType::E_UNKNOWN:
     default: {
         errorDescription << "Unknown ClientIdentity client type: "
-                         << clientIdentity;
+                         << logSafe(clientIdentity);
         return session;  // RETURN
     }
     }
@@ -442,9 +493,10 @@ bsl::shared_ptr<mqbnet::Session> SessionNegotiator::onClientIdentityMessage(
             // but we are not member of that cluster; emit an error (but
             // still accept the connection).
             BALL_LOG_ERROR << "#CONNECTION_UNEXPECTED Client '"
-                           << clientIdentity
+                           << logSafe(clientIdentity)
                            << "' connected to me as part of cluster '"
-                           << clusterName << "' to which I do not belong!";
+                           << logSafe(clusterName)
+                           << "' to which I do not belong!";
         }
         // Virtual clusters do not advertise node status.  Therefore, the
         // identity should not advertise k_BROADCAST_TO_PROXIES feature.
@@ -537,14 +589,15 @@ bsl::shared_ptr<mqbnet::Session> SessionNegotiator::onBrokerResponseMessage(
         negotiationMessage.brokerResponse();
 
     BALL_LOG_DEBUG << "Received negotiation message from '"
-                   << context_p->channel().get() << "': " << brokerResponse;
+                   << context_p->channel().get()
+                   << "': " << logSafe(brokerResponse);
 
     bsl::shared_ptr<mqbnet::Session> session;
 
     if (brokerResponse.result().category() !=
         bmqp_ctrlmsg::StatusCategory::E_SUCCESS) {
-        errorDescription << "Failure broker's response [" << brokerResponse
-                         << "]";
+        errorDescription << "Failure broker's response ["
+                         << logSafe(brokerResponse) << "]";
         return session;  // RETURN
     }
 
@@ -830,7 +883,7 @@ bool SessionNegotiator::checkIsDeprecatedSdkVersion(
     // keep a central location of all deprecated clients.
     BALL_LOG_WARN << "#CLIENT_SDKVERSION_DEPRECATED "
                   << "Client is using a deprecated SDK: "
-                  << "[client: " << clientIdentity
+                  << "[client: " << logSafe(clientIdentity)
                   << ", minimumSDKVersionRecommended: "
                   << mqbu::SDKVersionUtil::minSdkVersionRecommended(
                          clientIdentity.sdkLanguage())
@@ -858,7 +911,7 @@ bool SessionNegotiator::checkIsUnsupportedSdkVersion(
     // keep a central location of all rejected clients.
     BALL_LOG_WARN << "#CLIENT_SDKVERSION_UNSUPPORTED "
                   << "Client is using an unsupported SDK: "
-                  << "[client: " << clientIdentity
+                  << "[client: " << logSafe(clientIdentity)
                   << ", minimumSDKVersionSupported: "
                   << mqbu::SDKVersionUtil::minSdkVersionSupported(
                          clientIdentity.sdkLanguage())