Package: posh Version: 0.14.2 Severity: serious Tags: security patch X-Debbugs-CC: [email protected]
Dear Maintainer, While building posh 0.14.2 from the AUR on CachyOS, the compiler reports a clear heap buffer overflow: tree.c: In function 'vfptreef': tree.c:422:13: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=] 422 | p[20] = '\0'; | ^ tree.c:420:18: note: at offset 20 into destination object of size 20 allocated by 'malloc' 420 | p = q = malloc(20); | ^ Exact buggy code with context (tree.c lines 416–426): case 'd': case 'u': /* decimal */ n = (c == 'd') ? va_arg(va, int) : va_arg(va, unsigned int); neg = c=='d' && n<0; p = q = malloc(20); snprintf(p, 19, "%ld", (neg) ? -n : n); p[20] = '\0'; if (neg) *--p = '-'; while (*p) tputc(*p++, shf); free(q); break; The manual `p[20] = '\0'` is redundant because `snprintf(p, 19, ...)` already guarantees NULL-termination within the 20-byte buffer. Snipping relevant bits from the POSIX manual (https://www.unix.com/man_page/posix/3posix/snprintf/): > int snprintf(char *restrict s, size_t n, > const char *restrict format, ...); [...] > The snprintf() function shall be equivalent to sprintf(), with the > addition of the n argument which states the size of the buffer referred > to by s. If n is zero, nothing shall be written and s may be a null > pointer. Otherwise, output bytes beyond the n-1st shall be discarded > instead of being written to the array, and a null byte is written at > the end of the bytes actually written into the array. (Similar, arguably more clear, language is included in manual pages of more modern C Library implementations, and the functionality of \0' termination in snprintf() has been carried forward -- but the POSIX manual is quoted here because posh aims to be maximally compatible.) The minimal and obviously correct fix is to just delete the offending line (tested: warning disappears): --- a/tree.c +++ b/tree.c @@ -419,8 +419,6 @@ neg = c=='d' && n<0; p = q = malloc(20); snprintf(p, 19, "%ld", (neg) ? -n : n); - p[20] = '\0'; - if (neg) *--p = '-'; while (*p) Suggested Future Robustness Improvements: 1. The snprintf() above may fail for large numbers due to the "19" being passed as buffer size... It would be safest, less confusing and likely most performant to both malloc() a buffer of 24 bytes and pass the buffer size of 24 to snprintf(). The number of bytes that may be needed by snprintf() to convert a long positive integer on a 64-bit CPU to (NULL-terminated) string is not 19 bytes, but 20 bytes (it's 21 bytes if we were to need the negative sign); however, it would IMO be preferable to use a buffer of 24 as such an allocation is byte-aligned to 64 bits and stops us from trying to be too clever in the code. I'd think readers would also appreciate if the buffer size requirement would be commented in the code so that folks (like me) would not need or be too tempted to go GROK-ing for the answer. 2. The return of malloc() should be checked that it is non-NULL universally throughout the code. 3. The return of snprintf() should also be sanity-checked universally throughout the code. Because this is possibly a reachable heap buffer overflow [depending on the behavior of malloc()], I have marked it "serious" and CC'd the security team. Thank you! Kind regards, Michael Back <[email protected]>
