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 <maxsize> bytes (including the terminating NULL
byte) as per the function description, in this case the function would
write <maxsize>+1 byte (to write the terminating NULL byte upon return),
which may lead to invalid write if <dst> was meant to hold <maxsize> bytes
at maximum.

Hopefully, this bug wasn't triggered so far because sess_build_logline()
is called with logline as <dst> argument and <global.max_syslog_len> as
<maxsize> 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()")]

diff --git a/src/log.c b/src/log.c
index 847706d2c78598b62582ef1767d51f1d8508d2e8..8c918084660352eb215805b269c130521942dcc0 100644 (file)
--- 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 {