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

[Mario]

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 <jeremy at laidman.org> wrote:

> Ugh, just saw your reply in my spam folder!
>
> On Fri, 1 Sept 2023 at 23:40, J.C. Cleaver <cleaver at terabithia.org> 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
> _______________________________________________
> Xymon mailing list
> Xymon at xymon.com
> http://lists.xymon.com/mailman/listinfo/xymon
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.xymon.com/pipermail/xymon/attachments/20240531/c8432743/attachment.htm>