Log cyphered payloads could crash the log system

[Pascal-Fremaux]

Describe the bug
When activating the packet logs (sending or receiving) with SSL cyphered payloads, it "prints" non printable characters, and it could happen that the given payload crash the log system (depends on the log system, the sequence...)

To Reproduce
Use Paho with SSL, use a log system like syslogd or DLT, and run other real life packets until you got problems...

Expected behavior
Non printable characters sequences (byte buffers) should not be logged as genuine characters.

Screenshots
None

Log files
None

** Environment (please complete the following information):**

• OS: Linux
• Version: any (1.3.12 or others)

Additional context
The problems is old so I do not have the exact details (log system).

Here is a patch we use to avoid that problem (probably a better and more optimized solution could be done): the non printable buffers are replaced by a log string "<non printable>"

From cd45a0890084278941587093c3b24d848922120b Mon Sep 17 00:00:00 2001
From: Punith B T <punith.t@aricent.com>
Date: Wed, 14 Oct 2020 18:18:58 +0530
Subject: [PATCH] Compressed messages should not break the logging in HWless CI

---
 src/Log.c                | 21 +++++++++++++++++++++
 src/Log.h                |  1 +
 src/MQTTPacket.c         |  2 +-
 src/MQTTProtocolClient.c |  3 ++-
 4 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/src/Log.c b/src/Log.c
index 92054db0..b0f0d33a 100644
--- a/src/Log.c
+++ b/src/Log.c
@@ -37,6 +37,7 @@
 #include <stdarg.h>
 #include <time.h>
 #include <string.h>
+#include <ctype.h>
 
 #if !defined(_WIN32) && !defined(_WIN64)
 #include <syslog.h>
@@ -443,6 +444,26 @@ void Log(enum LOG_LEVELS log_level, int msgno, const char *format, ...)
 	}
 }
 
+/** method that format a log for a string that could be non printable
+ * @param str the input string
+ * @param len the length of str
+ * @return the input string if printable or the message "<non printable>"
+ */
+const char* formatPrintableString(const char* str, int len)
+{
+	int nonPrintable = 0;
+	int index = 0;
+	for (; index < len; index++) {
+		if (!isprint(str[index])) {
+			nonPrintable = 1;
+			break;
+		}
+	}
+
+	if (nonPrintable)
+		return "<non printable>";
+	return str;
+}
 
 /**
  * The reason for this function is to make trace logging as fast as possible so that the
diff --git a/src/Log.h b/src/Log.h
index ae9bb9ef..c28b0830 100644
--- a/src/Log.h
+++ b/src/Log.h
@@ -89,5 +89,6 @@ void Log_stackTrace(enum LOG_LEVELS, int, thread_id_type, int, const char*, int,
 typedef void Log_traceCallback(enum LOG_LEVELS level, const char *message);
 void Log_setTraceCallback(Log_traceCallback* callback);
 void Log_setTraceLevel(enum LOG_LEVELS level);
+const char* formatPrintableString(const char* str, int len);
 
 #endif
diff --git a/src/MQTTPacket.c b/src/MQTTPacket.c
index 9d8f08dd..510a5acc 100644
--- a/src/MQTTPacket.c
+++ b/src/MQTTPacket.c
@@ -908,7 +908,7 @@ int MQTTPacket_send_publish(Publish* pack, int dup, int qos, int retained, netwo
 				min(20, pack->payloadlen), pack->payload);
 	else
 		Log(LOG_PROTOCOL, 10, NULL, net->socket, clientID, pack->msgId, qos, retained, rc, pack->payloadlen,
-				min(20, pack->payloadlen), pack->payload);
+				min(20, pack->payloadlen), formatPrintableString(pack->payload, pack->payloadlen));
 exit_free:
 	if (rc != TCPSOCKET_INTERRUPTED)
 		free(topiclen);
diff --git a/src/MQTTProtocolClient.c b/src/MQTTProtocolClient.c
index e4b8f16c..03611d25 100644
--- a/src/MQTTProtocolClient.c
+++ b/src/MQTTProtocolClient.c
@@ -332,7 +332,8 @@ int MQTTProtocol_handlePublishes(void* pack, SOCKET sock)
 	client = (Clients*)(ListFindItem(bstate->clients, &sock, clientSocketCompare)->content);
 	clientid = client->clientID;
 	Log(LOG_PROTOCOL, 11, NULL, sock, clientid, publish->msgId, publish->header.bits.qos,
-					publish->header.bits.retain, publish->payloadlen, min(20, publish->payloadlen), publish->payload);
+			publish->header.bits.retain, publish->payloadlen, min(20, publish->payloadlen),
+			formatPrintableString(publish->payload, publish->payloadlen));
 
 	if (publish->header.bits.qos == 0)
 	{
-- 
GitLab

[Pascal-Fremaux]

A probably better fix would be to know that you have a cyphered buffer and not log it !

[icraggs]

We do also have a PR for logging the payload in hex, I'll take a look at that too.