mirrors/haproxy
1
0
Fork 0
mirror of https://git.haproxy.org/git/haproxy.git/ synced 2025-08-06 15:17:01 +02:00
Code Issues Projects Releases Wiki Activity

BUG/MINOR: log: invalid snprintf() usage in sess_build_logline()

Browse Source 
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()")]
This commit is contained in:
Aurelien DARRAGON 2024-04-09 15:59:42 +02:00
parent 8226e92eb0
commit 28548f812f
1 changed files with 7 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
src/log.c
View File

@ -1914,7 +1914,7 @@ char *lf_ip(char *dst, const struct sockaddr *sockaddr, size_t size, const struc
default: default:
return NULL; return NULL;
} }
if (iret < 0 || iret > size) if (iret < 0 || iret >= size)
return NULL; return NULL;
ret += iret; ret += iret;
} else { } 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) { if (node->options & LOG_OPT_HEXA) {
const unsigned char *port = (const unsigned char *)&((struct sockaddr_in *)sockaddr)->sin_port; const unsigned char *port = (const unsigned char *)&((struct sockaddr_in *)sockaddr)->sin_port;
iret = snprintf(dst, size, "%02X%02X", port[0], port[1]); iret = snprintf(dst, size, "%02X%02X", port[0], port[1]);
if (iret < 0 || iret > size) if (iret < 0 || iret >= size)
return NULL; return NULL;
ret += iret; ret += iret;
} else { } else {
@ -3206,7 +3206,7 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t
case LOG_FMT_TS: // %Ts case LOG_FMT_TS: // %Ts
if (tmp->options & LOG_OPT_HEXA) { if (tmp->options & LOG_OPT_HEXA) {
iret = snprintf(tmplog, dst + maxsize - tmplog, "%04X", (unsigned int)logs->accept_date.tv_sec); 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; goto out;
tmplog += iret; tmplog += iret;
} else { } else {
@ -3220,7 +3220,7 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t
case LOG_FMT_MS: // %ms case LOG_FMT_MS: // %ms
if (tmp->options & LOG_OPT_HEXA) { if (tmp->options & LOG_OPT_HEXA) {
iret = snprintf(tmplog, dst + maxsize - tmplog, "%02X",(unsigned int)logs->accept_date.tv_usec/1000); 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; goto out;
tmplog += iret; tmplog += iret;
} else { } else {
@ -3837,7 +3837,7 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t
case LOG_FMT_COUNTER: // %rt case LOG_FMT_COUNTER: // %rt
if (tmp->options & LOG_OPT_HEXA) { if (tmp->options & LOG_OPT_HEXA) {
iret = snprintf(tmplog, dst + maxsize - tmplog, "%04X", uniq_id); 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; goto out;
tmplog += iret; tmplog += iret;
} else { } else {
@ -3851,7 +3851,7 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t
case LOG_FMT_LOGCNT: // %lc case LOG_FMT_LOGCNT: // %lc
if (tmp->options & LOG_OPT_HEXA) { if (tmp->options & LOG_OPT_HEXA) {
iret = snprintf(tmplog, dst + maxsize - tmplog, "%04X", fe->log_count); 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; goto out;
tmplog += iret; tmplog += iret;
} else { } else {
@ -3873,7 +3873,7 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t
case LOG_FMT_PID: // %pid case LOG_FMT_PID: // %pid
if (tmp->options & LOG_OPT_HEXA) { if (tmp->options & LOG_OPT_HEXA) {
iret = snprintf(tmplog, dst + maxsize - tmplog, "%04X", pid); iret = snprintf(tmplog, dst + maxsize - tmplog, "%04X", pid);
if (iret < 0 || iret > dst + maxsize - tmplog) if (iret < 0 || iret >= dst + maxsize - tmplog)
goto out; goto out;
tmplog += iret; tmplog += iret;
} else { } else {