Possible bug in xymon svcstatus causing segfault (buffer overrun?)
list Jeremy Laidman
Hi all
I'm not a C programmer by any stretch, so I'm seeking confirmation that my
working is sound.
At our site we run 4.3.30-1 from Terabithia RPM. For most hosts, the
"trends" page displays just fine, but for some hosts, the webserver gives a
CGI error, and when I manually run the CGI directly it crashes with a seg
fault. This seems to be more likely for hosts with a lot of entries on the
trends page, but some with more trends graphs are OK than others with fewer.
I believe I've tracked the issue down to the last few lines of code in
svcstatus-trends.c, where a URL is constructed for one graph entry, before
being added to the list of trends in a buffer.
My reading of the code suggests that it first pre-allocates (using
malloc()) a 16k buffer for the trends data, then sequentially appends trend
strings (which include a URL) into the buffer, until the length of a string
would exceed the size of the buffer. In that case, an extra 4k of memory is
added to the buffer (using realloc()), and then the string is added to the
buffer.
The problem with the code, as I see it, is that the string might be more
than 4k long, in which case, adding string would cause a buffer overrun. I
don't really know what's in each string, so I don't know if it's ever
likely to be 4k, but we're seeing the process dumping core immediately
after this bit of code, so that's the suspicion.
Here's the bit of code, which is attempting to add the string "onelink" to
the buffer, after seeing if it's going to exceed the buffer, and extending
the buffer if required:
if ((buflen + strlen(onelink)) >=
allrrdlinks_buflen) {
SBUF_REALLOC(allrrdlinks,
allrrdlinks_buflen+4096);
allrrdlinksend = allrrdlinks + buflen;
}
allrrdlinksend += snprintf(allrrdlinksend,
(allrrdlinks_buflen - (allrrdlinksend - allrrdlinks)), "%s", onelink);
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
list Japheth Cleaver
▸
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
JeremyBased 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
list Jeremy Laidman
Ugh, just saw your reply in my spam folder!
▸
On Fri, 1 Sept 2023 at 23:40, J.C. Cleaver <user-87556346d4af@xymon.invalid> wrote:
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 JeremyBased 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?
Sorry, can you define "reproducer"? Is that a coding term I haven't yet come across? I have core dumps, but I'll see about using the debuginfo RPMs (assuming they're on Terabithia) and get a core dump from an install of that. Thanks
list Mario Andre
Hello Jeremy/JC, I?m facing the same issue with switches interfaces graphs not loading in trends or causing internal server error. Do you have a patch to fix this? Or maybe I need to downgrade xymon 4.3.30 version? Thanks & regards, Mario
▸
On Wed, Sep 13, 2023 at 4:59?AM Jeremy Laidman <user-0608abae5e7c@xymon.invalid> wrote:
Ugh, just saw your reply in my spam folder! On Fri, 1 Sept 2023 at 23:40, J.C. Cleaver <user-87556346d4af@xymon.invalid> wrote: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.Forexample 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 JeremyBased 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?Sorry, can you define "reproducer"? Is that a coding term I haven't yet come across? I have core dumps, but I'll see about using the debuginfo RPMs (assuming they're on Terabithia) and get a core dump from an install of that. Thanks
list Jeremy Laidman
Mario Attached is a diff to the file that I used. It's against version 4.3.30. Cheers Jeremy
▸
On Sat, 1 Jun 2024 at 06:29, Mario <user-82c7780661a4@xymon.invalid> wrote:
Hello Jeremy/JC, I?m facing the same issue with switches interfaces graphs not loading in trends or causing internal server error. Do you have a patch to fix this? Or maybe I need to downgrade xymon 4.3.30 version? Thanks & regards, Mario On Wed, Sep 13, 2023 at 4:59?AM Jeremy Laidman <user-0608abae5e7c@xymon.invalid> wrote:Ugh, just saw your reply in my spam folder! On Fri, 1 Sept 2023 at 23:40, J.C. Cleaver <user-87556346d4af@xymon.invalid> wrote: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.Forexample 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 JeremyBased 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?Sorry, can you define "reproducer"? Is that a coding term I haven't yet come across? I have core dumps, but I'll see about using the debuginfo RPMs (assuming they're on Terabithia) and get a core dump from an install of that. Thanks
Attachments (1)
list Jeremy Laidman
No, please ignore that. It's not the diff I was looking for. Attached is the correct diff. J
▸
On Mon, 3 Jun 2024 at 09:35, Jeremy Laidman <user-0608abae5e7c@xymon.invalid> wrote:
Mario Attached is a diff to the file that I used. It's against version 4.3.30. Cheers Jeremy On Sat, 1 Jun 2024 at 06:29, Mario <user-82c7780661a4@xymon.invalid> wrote:Hello Jeremy/JC, I?m facing the same issue with switches interfaces graphs not loading in trends or causing internal server error. Do you have a patch to fix this? Or maybe I need to downgrade xymon 4.3.30 version? Thanks & regards, Mario On Wed, Sep 13, 2023 at 4:59?AM Jeremy Laidman <user-0608abae5e7c@xymon.invalid> wrote:Ugh, just saw your reply in my spam folder! On Fri, 1 Sept 2023 at 23:40, J.C. Cleaver <user-87556346d4af@xymon.invalid> wrote: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.Forexample 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 JeremyBased 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?Sorry, can you define "reproducer"? Is that a coding term I haven't yet come across? I have core dumps, but I'll see about using the debuginfo RPMs (assuming they're on Terabithia) and get a core dump from an install of that. Thanks
Attachments (1)
list Mario Andre
Hello Jeremy, It worked! Now all the graphs are showing in trends. Thank you very much!! Best regards, Mario
▸
On Sun, Jun 2, 2024 at 9:09?PM Jeremy Laidman <user-0608abae5e7c@xymon.invalid> wrote:
No, please ignore that. It's not the diff I was looking for. Attached is the correct diff. J On Mon, 3 Jun 2024 at 09:35, Jeremy Laidman <user-0608abae5e7c@xymon.invalid> wrote:Mario Attached is a diff to the file that I used. It's against version 4.3.30. Cheers Jeremy On Sat, 1 Jun 2024 at 06:29, Mario <user-82c7780661a4@xymon.invalid> wrote:Hello Jeremy/JC, I?m facing the same issue with switches interfaces graphs not loading in trends or causing internal server error. Do you have a patch to fix this? Or maybe I need to downgrade xymon 4.3.30 version? Thanks & regards, Mario On Wed, Sep 13, 2023 at 4:59?AM Jeremy Laidman <user-0608abae5e7c@xymon.invalid> wrote:Ugh, just saw your reply in my spam folder! On Fri, 1 Sept 2023 at 23:40, J.C. Cleaver <user-87556346d4af@xymon.invalid> wrote: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 JeremyBased 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?Sorry, can you define "reproducer"? Is that a coding term I haven't yet come across? I have core dumps, but I'll see about using the debuginfo RPMs (assuming they're on Terabithia) and get a core dump from an install of that. Thanks