From 28548f812fb7070e051fa79c8b28bba4f8a362e1 Mon Sep 17 00:00:00 2001 From: Aurelien DARRAGON Date: Tue, 9 Apr 2024 15:59:42 +0200 Subject: [PATCH] BUG/MINOR: log: invalid snprintf() usage in sess_build_logline() According to snprintf() man page: The functions snprintf() and vsnprintf() do not write more than size bytes (including the terminating null byte ('\0')). If the output was truncated due to this limit, then the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available. Thus, a return value of size or more means that the output was truncated. However, in sess_build_logline(), each time we need to check the return value of snprintf(), here is how we proceed: iret = snprintf(tmplog, max, ...); if (iret < 0 || iret > max) // error // success tmplog += iret; Here is the issue: if snprintf() lacks 1 byte space to write the terminating NULL byte, it will return max. Which means in this case that we fail to know that snprintf() truncated the output in reality, and we still add iret to tmplog pointer. Considering sess_build_logline() should NOT write more than bytes (including the terminating NULL byte) as per the function description, in this case the function would write +1 byte (to write the terminating NULL byte upon return), which may lead to invalid write if was meant to hold bytes at maximum. Hopefully, this bug wasn't triggered so far because sess_build_logline() is called with logline as argument and as argument, logline being initialized with 1 extra byte upon startup. But we better fix this to comply with the function description and prevent any side-effect since some sess_build_logline() helpers may assume that 'tmplog-dst < maxsize' is always true. Also sess_build_logline() users probably don't expect NULL-byte to be accounted for in the produced logline length. This should be backported to all stable versions. [ada: for backports, the patch needs to be applied manually because of c6a713842 ("MINOR: log: simplify last_isspace in sess_build_logline()")] --- src/log.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/log.c b/src/log.c index 847706d2c..8c9180846 100644 --- a/src/log.c +++ b/src/log.c @@ -1914,7 +1914,7 @@ char *lf_ip(char *dst, const struct sockaddr *sockaddr, size_t size, const struc default: return NULL; } - if (iret < 0 || iret > size) + if (iret < 0 || iret >= size) return NULL; ret += iret; } else { @@ -1938,7 +1938,7 @@ char *lf_port(char *dst, const struct sockaddr *sockaddr, size_t size, const str if (node->options & LOG_OPT_HEXA) { const unsigned char *port = (const unsigned char *)&((struct sockaddr_in *)sockaddr)->sin_port; iret = snprintf(dst, size, "%02X%02X", port[0], port[1]); - if (iret < 0 || iret > size) + if (iret < 0 || iret >= size) return NULL; ret += iret; } else { @@ -3206,7 +3206,7 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t case LOG_FMT_TS: // %Ts if (tmp->options & LOG_OPT_HEXA) { iret = snprintf(tmplog, dst + maxsize - tmplog, "%04X", (unsigned int)logs->accept_date.tv_sec); - if (iret < 0 || iret > dst + maxsize - tmplog) + if (iret < 0 || iret >= dst + maxsize - tmplog) goto out; tmplog += iret; } else { @@ -3220,7 +3220,7 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t case LOG_FMT_MS: // %ms if (tmp->options & LOG_OPT_HEXA) { iret = snprintf(tmplog, dst + maxsize - tmplog, "%02X",(unsigned int)logs->accept_date.tv_usec/1000); - if (iret < 0 || iret > dst + maxsize - tmplog) + if (iret < 0 || iret >= dst + maxsize - tmplog) goto out; tmplog += iret; } else { @@ -3837,7 +3837,7 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t case LOG_FMT_COUNTER: // %rt if (tmp->options & LOG_OPT_HEXA) { iret = snprintf(tmplog, dst + maxsize - tmplog, "%04X", uniq_id); - if (iret < 0 || iret > dst + maxsize - tmplog) + if (iret < 0 || iret >= dst + maxsize - tmplog) goto out; tmplog += iret; } else { @@ -3851,7 +3851,7 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t case LOG_FMT_LOGCNT: // %lc if (tmp->options & LOG_OPT_HEXA) { iret = snprintf(tmplog, dst + maxsize - tmplog, "%04X", fe->log_count); - if (iret < 0 || iret > dst + maxsize - tmplog) + if (iret < 0 || iret >= dst + maxsize - tmplog) goto out; tmplog += iret; } else { @@ -3873,7 +3873,7 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t case LOG_FMT_PID: // %pid if (tmp->options & LOG_OPT_HEXA) { iret = snprintf(tmplog, dst + maxsize - tmplog, "%04X", pid); - if (iret < 0 || iret > dst + maxsize - tmplog) + if (iret < 0 || iret >= dst + maxsize - tmplog) goto out; tmplog += iret; } else {