Possible bug in xymon svcstatus causing segfault (buffer overrun?)

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
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?

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