From 8226e92eb07cc1a2ed8d30f4bd33d54c8e5328b8 Mon Sep 17 00:00:00 2001 From: Aurelien DARRAGON Date: Tue, 9 Apr 2024 11:44:54 +0200 Subject: [PATCH] BUG/MINOR: tools/log: invalid encode_{chunk,string} usage encode_{chunk,string}() is often found to be used this way: ret = encode_{chunk,string}(start, stop...) if (ret == NULL || *ret != '\0') { //error } //success Indeed, encode_{chunk,string} will always try to add terminating NULL byte to the output string, unless no space is available for even 1 byte. However, it means that for the caller to be able to spot an error, then it must provide a buffer (here: start) which is already initialized. But this is wrong: not only this is very tricky to use, but since those functions don't return NULL on failure, then if the output buffer was not properly initialized prior to calling the function, the caller will perform invalid reads when checking for failure this way. Moreover, even if the buffer is initialized, we cannot reliably tell if the function actually failed this way because if the buffer was previously initialized with NULL byte, then the caller might think that the call actually succeeded (since the function didn't return NULL and didn't update the buffer). Also, sess_build_logline() relies lf_encode_{chunk,string}() functions which are in fact wrappers for encode_{chunk,string}() functions and thus exhibit the same error handling mechanism. It turns out that sess_build_logline() makes unsafe use of those functions because it uses the error-checking logic mentionned above while buffer (tmplog) is not guaranteed to be initialized when entering the function. This may ultimately cause malfunctions or invalid reads if the output buffer is lacking space. To fix the issue once and for all and prevent similar bugs from being introduced, we make it so encode_{string, chunk} and escape_string() (based on encode_string()) now explicitly return NULL on failure (when the function failed to write at least the ending NULL byte) lf_encode_{string,chunk}() helpers had to be patched as well due to code duplication. This should be backported to all stable versions. [ada: for 2.4 and 2.6 the patch won't apply as-is, it might be helpful to backport ae1e14d65 ("CLEANUP: tools: removing escape_chunk() function") first, considering it's not very relevant to maintain a dead function] --- include/haproxy/tools.h | 13 +++++++------ src/log.c | 10 ++++++++-- src/tools.c | 22 +++++++++++++--------- 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/include/haproxy/tools.h b/include/haproxy/tools.h index 3726f63539..d76b2d13c8 100644 --- a/include/haproxy/tools.h +++ b/include/haproxy/tools.h @@ -399,11 +399,11 @@ int addr_is_local(const struct netns_entry *ns, * with the hexadecimal representation of their ASCII-code (2 digits) * prefixed by , and will store the result between (included) * and (excluded), and will always terminate the string with a '\0' - * before . The position of the '\0' is returned if the conversion - * completes. If bytes are missing between and , then the - * conversion will be incomplete and truncated. If <= , the '\0' - * cannot even be stored so we return without writing the 0. + * before . If bytes are missing between and , then the + * conversion will be incomplete and truncated. * The input string must also be zero-terminated. + * + * Return the address of the \0 character, or NULL on error */ extern const char hextab[]; extern long query_encode_map[]; @@ -424,8 +424,9 @@ char *encode_chunk(char *start, char *stop, * is reached or NULL-byte is encountered. The result will * be stored between (included) and (excluded). This * function will always try to terminate the resulting string with a '\0' - * before , and will return its position if the conversion - * completes. + * before . + * + * Return the address of the \0 character, or NULL on error */ char *escape_string(char *start, char *stop, const char escape, const long *map, diff --git a/src/log.c b/src/log.c index a1236e9f2c..847706d2c7 100644 --- a/src/log.c +++ b/src/log.c @@ -1734,6 +1734,8 @@ int get_log_facility(const char *fac) * When using the +E log format option, it will try to escape '"\]' * characters with '\' as prefix. The same prefix should not be used as * . + * + * Return the address of the \0 character, or NULL on error */ static char *lf_encode_string(char *start, char *stop, const char escape, const long *map, @@ -1764,13 +1766,14 @@ static char *lf_encode_string(char *start, char *stop, string++; } *start = '\0'; + return start; } } else { return encode_string(start, stop, escape, map, string); } - return start; + return NULL; } /* @@ -1779,6 +1782,8 @@ static char *lf_encode_string(char *start, char *stop, * When using the +E log format option, it will try to escape '"\]' * characters with '\' as prefix. The same prefix should not be used as * . + * + * Return the address of the \0 character, or NULL on error */ static char *lf_encode_chunk(char *start, char *stop, const char escape, const long *map, @@ -1814,13 +1819,14 @@ static char *lf_encode_chunk(char *start, char *stop, str++; } *start = '\0'; + return start; } } else { return encode_chunk(start, stop, escape, map, chunk); } - return start; + return NULL; } /* diff --git a/src/tools.c b/src/tools.c index 475922283b..b08fb18ce5 100644 --- a/src/tools.c +++ b/src/tools.c @@ -1969,11 +1969,11 @@ int addr_is_local(const struct netns_entry *ns, * with the hexadecimal representation of their ASCII-code (2 digits) * prefixed by , and will store the result between (included) * and (excluded), and will always terminate the string with a '\0' - * before . The position of the '\0' is returned if the conversion - * completes. If bytes are missing between and , then the - * conversion will be incomplete and truncated. If <= , the '\0' - * cannot even be stored so we return without writing the 0. + * before . If bytes are missing between and , then the + * conversion will be incomplete and truncated. * The input string must also be zero-terminated. + * + * Return the address of the \0 character, or NULL on error */ const char hextab[16] = "0123456789ABCDEF"; char *encode_string(char *start, char *stop, @@ -1995,8 +1995,9 @@ char *encode_string(char *start, char *stop, string++; } *start = '\0'; + return start; } - return start; + return NULL; } /* @@ -2025,8 +2026,9 @@ char *encode_chunk(char *start, char *stop, str++; } *start = '\0'; + return start; } - return start; + return NULL; } /* @@ -2035,8 +2037,9 @@ char *encode_chunk(char *start, char *stop, * is reached or NULL-byte is encountered. The result will * be stored between (included) and (excluded). This * function will always try to terminate the resulting string with a '\0' - * before , and will return its position if the conversion - * completes. + * before . + * + * Return the address of the \0 character, or NULL on error */ char *escape_string(char *start, char *stop, const char escape, const long *map, @@ -2056,8 +2059,9 @@ char *escape_string(char *start, char *stop, string++; } *start = '\0'; + return start; } - return start; + return NULL; } /* Check a string for using it in a CSV output format. If the string contains -- 2.39.5