On Tue, August 29, 2023 23:40, Jeremy Laidman wrote:
Elsewhere in other code, the SBUF_REALLOC macro is used to add extra
bufferspace by calculating a new buffer size of [existing buffer size plus
4k plus the length of the string being added]. So if the string to be
added
had 20 bytes, SBUF_REALLOC would be used to add 4k+20 bytes to the buffer
size. Notably, the size of the string is included in the calculation. For
example this bit of code elsewhere in the same file does a similar buffer
extension:
if ((strlen(rrdlink) + strlen(partlink) + 1) >=
rrdlink_buflen) {
SBUF_REALLOC(rrdlink, rrdlink_buflen +
strlen(partlink) + 4096);
}
strncat(rrdlink, partlink, (rrdlink_buflen -
strlen(rrdlink)));
Note the extra "+ strlen(<stringvar>)" bit in the extra memory being
allocated that isn't there in the first block of code above. So my
thinking
is that the SBUF_REALLOC() needs to take the string's size into account,
like so:
SBUF_REALLOC(allrrdlinks,
allrrdlinks_buflen+strlen(onelink)+4096);
Is anyone with C skills able to confirm that this does indeed look like a
bug, and also that my proposed change is a suitable fix? After applying
this change and recompiling, we haven't yet seen a seg fault from this bit
of code, so I'm hoping it's all fixed, and without negative side effects.
Cheers
Jeremy
Based on my looking over it, I would confirm that this seems to be a bug.
Specifically, it looks like we missed this part of the fix in
https://sourceforge.net/p/xymon/code/8069/?page=7#diff-1 while trying to
get ahead of possible buffer overflows for various CVE's
(https://nvd.nist.gov/vuln/detail/CVE-2019-13486, I suppose). I can't see
a reason not to add the length in here without a guard somewhere else for
the length.
Furthermore, other uses of this macro pretty much all include the length
of the incoming string or limit the concat to a specific size, which
obviously isn't ideal for URLs here.
If you have a reproducer and your patch fixes it, I suspect that will be
sufficient and we can add it in. Can you send a coredump from a crash off
list by any chance (using the debuginfo RPMs) just to double-check?
This is limited to trends pages and I'm not sure it would be exploitable
via construction without hostsvc-add ability in xymond, but if so that's a
concern.
Thanks,
-jc