Escape unprintable characters in invalid host names. Also, print some more details where invalid host names come from when we receive them from upstream. Before this change, such information is only available if debug.resolver = true which seems undesirable here

See https://discourse.pi-hole.net/t/host-name-of-client-xxx-contains-at-least-one-invalid-character-at-position-0/69132/92?u=dl6er as reference for why we do this

Signed-off-by: DL6ER <dl6er@dl6er.de>

src/database/message-table.c

@@ -624,40 +624,18 @@ static void format_subnet_message(char *plain, const int sizeof_plain, char *htm
 
 static void format_hostname_message(char *plain, const int sizeof_plain, char *html, const int sizeof_html, const char *ip, const char *name, const int pos)
 {
-	char *namep = escape_json(name);
-	if(namep == NULL)
-	{
-		log_err("format_hostname_message(): Failed to JSON escape host name \"%s\" of client \"%s\"", name, ip);
-		return;
-	}
-
-	// Check if the position is within the string before proceeding
-	// This is a safety measure to prevent buffer overflows caused by
-	// malicious database records
-	if(pos > (int)strlen(name))
-	{
-		log_err("format_hostname_message(): Invalid position %i for host name \"%s\" of client \"%s\"", pos, namep, ip);
-		if(namep != NULL)
-			free(namep);
-		return;
-	}
-
 	// Format the plain text message (the JSON string is already escaped and
 	// contains "" around the string)
-	if(snprintf(plain, sizeof_plain, "Host name of client \"%s\" => %s contains (at least) one invalid character (hex %02x) at position %i",
-			ip, namep, (unsigned char)name[pos], pos) > sizeof_plain)
+	if(snprintf(plain, sizeof_plain, "Host name of client \"%s\" => %s contains (at least) one invalid character at position %i",
+			ip, name, pos) > sizeof_plain)
 		log_warn("format_hostname_message(): Buffer too small to hold plain message, warning truncated");
 
 	// Return early if HTML text is not required
 	if(sizeof_html < 1 || html == NULL)
-	{
-		if(namep != NULL)
-			free(namep);
 		return;
-	}
 
 	char *escaped_ip = escape_html(ip);
-	char *escaped_name = escape_html(namep);
+	char *escaped_name = escape_html(name);
 
 	// Return early if memory allocation failed
 	if(escaped_ip == NULL || escaped_name == NULL)
@@ -666,8 +644,6 @@ static void format_hostname_message(char *plain, const int sizeof_plain, char *h
 			free(escaped_ip);
 		if(escaped_name != NULL)
 			free(escaped_name);
-		if(namep != NULL)
-			free(namep);
 		return;
 	}
 
@@ -677,8 +653,6 @@ static void format_hostname_message(char *plain, const int sizeof_plain, char *h
 
 	free(escaped_ip);
 	free(escaped_name);
-	if(namep != NULL)
-		free(namep);
 }
 
 static void format_dnsmasq_config_message(char *plain, const int sizeof_plain, char *html, const int sizeof_html, const char *message)
@@ -1394,7 +1368,7 @@ void logg_subnet_warning(const char *ip, const int matching_count, const char *m
 	free(names);
 }
 
-void logg_hostname_warning(const char *ip, const char *name, const unsigned int pos)
+void log_hostname_warning(const char *ip, const char *name, const unsigned int pos)
 {
 	// Create message
 	char buf[2048] = { 0 };

src/database/message-table.h

@@ -22,7 +22,7 @@ void logg_regex_warning(const char *type, const char *warning, const int dbindex
 void logg_subnet_warning(const char *ip, const int matching_count, const char *matching_ids,
                          const int matching_bits, const char *chosen_match_text,
                          const int chosen_match_id);
-void logg_hostname_warning(const char *ip, const char *name, const unsigned int pos);
+void log_hostname_warning(const char *ip, const char *name, const unsigned int pos);
 void logg_fatal_dnsmasq_message(const char *message);
 void logg_rate_limit_message(const char *clientIP, const unsigned int rate_limit_count);
 void logg_warn_dnsmasq_message(char *message);

src/log.c

@@ -572,8 +572,8 @@ const char __attribute__ ((const)) *get_ordinal_suffix(unsigned int number)
 // Converts a buffer of specified length to ASCII representation as it was a C
 // string literal. Returns how much bytes from source was processed
 // Inspired by https://stackoverflow.com/a/56123950
-int binbuf_to_escaped_C_literal(const char *src_buf, size_t src_sz,
-                                      char *dst_str, size_t dst_sz)
+static int binbuf_to_escaped_C_literal(const char *src_buf, size_t src_sz,
+                                       char *dst_str, size_t dst_sz)
 {
 	const char *src = src_buf;
 	char *dst = dst_str;
@@ -628,7 +628,7 @@ int binbuf_to_escaped_C_literal(const char *src_buf, size_t src_sz,
 					*dst++ = '0';
 					break;
 				default:
-					sprintf(dst, "0x%02X", (unsigned char)*src);
+					sprintf(dst, "\\x%02X", (unsigned char)*src);
 					dst += 4;
 					break;
 			}
@@ -642,6 +642,37 @@ int binbuf_to_escaped_C_literal(const char *src_buf, size_t src_sz,
 	return src - src_buf;
 }
 
+/**
+ * @brief Escapes a binary string to be printable as a C string literal.
+ *
+ * This function allocates a new string and converts the input buffer into an escaped
+ * C string literal, suitable for safe printing or logging. Each character in the source
+ * buffer may be escaped, so the output buffer is allocated with enough space for the
+ * worst-case scenario (every character is escaped as \xNN).
+ *
+ * @param src_buf Pointer to the source buffer to escape.
+ * @param src_sz  Size of the source buffer in bytes.
+ * @return Pointer to the newly allocated escaped string, or NULL on allocation or conversion failure.
+ *         The returned string must be freed by the caller.
+ */
+char * __attribute__((malloc)) escape_str(const char *src_buf, size_t src_sz)
+{
+	// Allocate memory for the escaped string
+	char *escaped_str = malloc(src_sz * 4 + 1); // Worst case: every char is escaped
+	if(!escaped_str)
+		return NULL;
+
+	// Convert buffer to escaped C literal
+	const int processed = binbuf_to_escaped_C_literal(src_buf, src_sz, escaped_str, src_sz * 4 + 1);
+	if(processed < 0)
+	{
+		free(escaped_str);
+		return NULL;
+	}
+
+	return escaped_str;
+}
+
 const char * __attribute__ ((pure)) short_path(const char *full_path)
 {
 	const char *shorter = strstr(full_path, "src/");

src/log.h

@@ -74,7 +74,7 @@ void FTL_log_dnsmasq_fatal(const char *format, ...) __attribute__ ((format (prin
 void log_ctrl(bool vlog, bool vstdout);
 void FTL_log_helper(const unsigned int n, ...);
 
-int binbuf_to_escaped_C_literal(const char *src_buf, size_t src_sz, char *dst_str, size_t dst_sz);
+char *escape_str(const char *src_buf, size_t src_sz) __attribute__((malloc));
 
 const char *short_path(const char *full_path) __attribute__ ((pure));
 

src/resolve.c

@@ -25,7 +25,7 @@
 #include "database/network-table.h"
 // resolver_ready
 #include "daemon.h"
-// logg_hostname_warning()
+// log_hostname_warning()
 #include "database/message-table.h"
 // Eventqueue routines
 #include "events.h"
@@ -215,8 +215,7 @@ static bool valid_hostname(char *name, const char *clientip)
 			 c == '.' )
 			continue;
 
-		// Invalid character found, log and return hostname being invalid
-		logg_hostname_warning(clientip, name, i);
+		// Invalid character found => return hostname being invalid
 		return false;
 	}
 
@@ -452,7 +451,7 @@ static char *__attribute__((malloc)) ngethostbyname(const int sock, const bool t
 
 		name = (char *)answers[i].rdata;
 		log_debug(DEBUG_RESOLVER, "Answer %u is PTR \"%s\" => \"%s\"",
-		          i, answers[i].name, answers[i].rdata);
+		          i, answers[i].name, name);
 
 		// We break out of the loop if this is a valid hostname
 		if(strlen(name) > 0 && valid_hostname(name, ipaddr))
@@ -462,9 +461,19 @@ static char *__attribute__((malloc)) ngethostbyname(const int sock, const bool t
 		}
 		else
 		{
+			char *escaped_name = escape_str(name, strlen(name));
+			log_warn("Resolved PTR \"%s\" on 127.0.0.1#%u (%s) with status %s (%i): answer %u (PTR \"%s\" => \"%s\") is invalid",
+			         host, config.dns.port.v.u16, tcp ? "TCP" : "UDP",
+			         getDNScode(dns->rcode), dns->rcode, i, answers[i].name, escaped_name);
+			log_hostname_warning(ipaddr, escaped_name, i);
+
 			// Discard this answer: free memory and set name to NULL
 			free(answers[i].name);
 			free(answers[i].rdata);
+			if(escaped_name != NULL)
+				free(escaped_name);
+
+			// Set name to NULL so we can return an empty string later
 			name = NULL;
 		}
 	}

src/shmem.c

@@ -1543,15 +1543,7 @@ void dump_strings(void)
 
 			// If the string is not printable, we escape it
 			if(!string_is_printable)
-			{
-				buffer = calloc(len * 4 + 1, sizeof(char));
-				if(buffer == NULL)
-				{
-					log_err("Failed to allocate memory for string buffer");
-					break;
-				}
-				binbuf_to_escaped_C_literal(sstr, len, buffer, len * 4 + 1);
-			}
+				buffer = escape_str(sstr, len);
 
 			// Print string to file
 			fprintf(str_dumpfile, "%s %04zu: \"%s\" (%zu/%zu)\n", string_is_printable ? "    " : "NONP",

src/tools/dhcp-discover.c

@@ -345,14 +345,10 @@ static void print_dhcp_offer(struct in_addr source, struct dhcp_packet_data *off
 			}
 			else if(opttab[i].size & OT_NAME)
 			{
-				// We may need to escape this, buffer size: 4
-				// chars per control character plus room for
-				// possible "(empty)"
-				const size_t bufsiz = 4*optlen + 9;
-				char *buffer = calloc(bufsiz, sizeof(char));
-				binbuf_to_escaped_C_literal((char*)&offer_packet->options[x], optlen, buffer, bufsiz);
+				char *buffer = escape_str((char*)&offer_packet->options[x], optlen);
 				printf("%s: \"%s\"\n", opttab[i].name, buffer);
-				free(buffer);
+				if(buffer != NULL)
+					free(buffer);
 			}
 			else if(opttab[i].size & OT_TIME)
 			{
@@ -360,7 +356,7 @@ static void print_dhcp_offer(struct in_addr source, struct dhcp_packet_data *off
 				memcpy(&time, &offer_packet->options[x], sizeof(time));
 				time = ntohl(time);
 				const char *optname = opttab[i].name;
-				// Some timers deserve a more user-friedly name
+				// Some timers deserve a more user-friendly name
 				if(opttype == 58)
 					optname = "renewal-time"; // "T1" in dnsmasq-notation
 				else if(opttype == 59)
@@ -437,9 +433,7 @@ static void print_dhcp_offer(struct in_addr source, struct dhcp_packet_data *off
 				// We may need to escape this, buffer size: 4
 				// chars per control character plus room for
 				// possible "(empty)"
-				size_t bufsiz = 4*optlen + 9;
-				char *buffer = calloc(bufsiz, sizeof(char));
-				binbuf_to_escaped_C_literal((char*)&offer_packet->options[x], optlen, buffer, bufsiz);
+				char *buffer = escape_str((char*)&offer_packet->options[x], optlen);
 				printf("wpad-server: \"%s\"\n", buffer);
 				free(buffer);
 			}
@@ -635,9 +629,7 @@ static unsigned int get_dhcp_offer(const int sock, const uint32_t xid, const cha
 		if(offer_packet.sname[0] != 0)
 		{
 			size_t len = strlen(offer_packet.sname);
-			size_t bufsiz = 4*len + 9;
-			char *buffer = calloc(bufsiz, sizeof(char));
-			binbuf_to_escaped_C_literal(offer_packet.sname, len, buffer, bufsiz);
+			char *buffer = escape_str(offer_packet.sname, len);
 			printf("%s\n", buffer);
 			free(buffer);
 		}
@@ -648,9 +640,7 @@ static unsigned int get_dhcp_offer(const int sock, const uint32_t xid, const cha
 		if(offer_packet.file[0] != 0)
 		{
 			size_t len = strlen(offer_packet.file);
-			size_t bufsiz = 4*len + 9;
-			char *buffer = calloc(bufsiz, sizeof(char));
-			binbuf_to_escaped_C_literal(offer_packet.file, len, buffer, bufsiz);
+			char *buffer = escape_str(offer_packet.file, len);
 			printf("%s\n", buffer);
 			free(buffer);
 		}