Segfault in confreport-critical.sh / confreport.cgi in 4.3.24 🔗 link

Hi,

Via corekeeper (https://packages.debian.org/stable/corekeeper) I
became aware of a segfault of confreport.cgi. I'm able to reproduce it
(at least) in a fresh browser instance (all cookies removed), going to
"Find host", searching for a non-existing host and then clicking on
"Config Report (Critical)". It says "172 hosts included" (i.e. not
just a single host as it happens if I searched for an existing host
before) and then outputs most of the page, but ends in the middle of
the alerts listing at line 8049 in the middle of a table:

<tr><td><font COLOR="#000000" FACE="Tahoma, Arial, Helvetica">user-c3fdb1d3f39c@xymon.invalid (R)</font></td><td align=center>5m 1s </td><td align=center>-</td><td align=center>4h </td><td align=center>-</td><td>purple</td></tr>
<tr><td valign=top rowspan=4  >ports</td><td><font COLOR="#000000" FACE="Tahoma, Arial, Helvetica">0765

At least in one of the cases it ended in the same line, but with "076
instead of 0765". So I suspect we hit some C string size limit once
again.

I don't have a proper backtrace (yet), but I had look at the source
code and I'm quite confident that the issue is inside the function
print_alert_recipients() starting at lib/loadalerts.c, line 1124.

I suspect it's an overflow of the variable "buf". "l" seems large
enough with 4kB.

		Kind regards, Axel Beckert
-- 
Axel Beckert <user-96d9963fe797@xymon.invalid>       support: +41 44 633 26 68
IT Services Group, HPT H 6                  voice: +41 44 633 41 89
Departement of Physics, ETH Zurich
CH-8093 Zurich, Switzerland		   http://nic.phys.ethz.ch/

Hi,

On Thu, Dec 10, 2015 at 01:45:49PM +0100, Axel Beckert wrote:

Here's the according backtrace. Doesn't look too helpful to me,
though:

# gdb /usr/lib/xymon/server/bin/confreport.cgi ./207855-33-33-11-1449754874-silberspitz--usr-lib-xymon-server-bin-confreport.cgi.core
GNU gdb (Debian 7.7.1+dfsg-5) 7.7.1
[...]
Reading symbols from /usr/lib/xymon/server/bin/confreport.cgi...Reading symbols from /usr/lib/debug/.build-id/f2/5c37643ea6a391c09535cc250d18cef4aa39cf.debug...done.
done.
[New LWP 207855]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `/usr/lib/xymon/server/bin/confreport.cgi --env=/usr/lib/xymon/server/etc/xymons'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  strlen () at ../sysdeps/x86_64/strlen.S:106
106     ../sysdeps/x86_64/strlen.S: No such file or directory.
(gdb) bt
#0  strlen () at ../sysdeps/x86_64/strlen.S:106
#1  0x0000000000403640 in print_host (testcount=18, testnames=0x167dcc0, host=0x166b070) at confreport.c:310
#2  main (argc=<optimized out>, argv=<optimized out>) at confreport.c:876

Line 310 of web/confreport.c looks liek this:

  newitem->visualdata = (char *)realloc(newitem->visualdata, strlen(newitem->visualdata) + strlen(visdata) + 5);

So do I read the backtrace correctly that there was no proper value
(e.g. a null pointer) was passed to strlen()?

Then again I also don't find any file named strlen.S on the system
where I build the packages either. (But it seems to be part of glibc's
source code.)

		Kind regards, Axel Beckert
-- 
Axel Beckert <user-96d9963fe797@xymon.invalid>       support: +41 44 633 26 68
IT Services Group, HPT H 6                  voice: +41 44 633 41 89
Departement of Physics, ETH Zurich
CH-8093 Zurich, Switzerland		   http://nic.phys.ethz.ch/

On Thu, December 10, 2015 5:59 am, Axel Beckert wrote:

Hmm,

Well, that's different from loadalerts.c, but there does seem to be a
problem there.

Can you see if this patch helps?

-jc

Hi JC,

On Thu, Dec 10, 2015 at 08:23:23AM -0800, J.C. Cleaver wrote:

[...]

[...]

Well, that's different from loadalerts.c,

Indeed. My first guess was solely based on where the interrupted HTML
output comes from.

but there does seem to be a problem there.

Yeah, I figured in the meanwhile that visdata likely was NULL to cause
that crash. I was just not sure which places would all need changes to
fix that.

Can you see if this patch helps?

Will check. Thanks for the patch!

Index: web/confreport.c
===================================================================
--- web/confreport.c	(revision 7835)
+++ web/confreport.c	(working copy)
@@ -288,9 +288,10 @@
 		}
 		else if (is_net_test(itm)) {
 			colname = strdup(itm);
+			visdata = strdup("");
 		}
 
• +		if (!visdata) visdata = strdup("");

Those two additionans look a little bit redundant, but that shouldn't
cause any harm. (I'd say the first addition shouldn't be necessary
if the second one is present.)

-				newitem->visualdata = (char *)realloc(newitem->visualdata, strlen(newitem->visualdata) + strlen(visdata) + 5);
+				newitem->visualdata = newitem->visualdata ?
+					(char *)realloc(newitem->visualdata, strlen(newitem->visualdata) + strlen(visdata) + 5) :
+					(char *)malloc(strlen(visdata) + 5);

That's the place where I thought, it might need changes, too, but I
had no ideas which exactly.

		Kind regards, Axel Beckert
-- 
Axel Beckert <user-96d9963fe797@xymon.invalid>       support: +41 44 633 26 68
IT Services Group, HPT H 6                  voice: +41 44 633 41 89
Departement of Physics, ETH Zurich
CH-8093 Zurich, Switzerland		   http://nic.phys.ethz.ch/

Hi JC,

On Thu, Dec 10, 2015 at 08:23:23AM -0800, J.C. Cleaver wrote:

The patched helped, thanks! Can't reproduce the issue anymore.

		Kind regards, Axel Beckert
-- 
Axel Beckert <user-96d9963fe797@xymon.invalid>       support: +41 44 633 26 68
IT Services Group, HPT H 6                  voice: +41 44 633 41 89
Departement of Physics, ETH Zurich
CH-8093 Zurich, Switzerland		   http://nic.phys.ethz.ch/

On Thu, December 10, 2015 8:53 am, Axel Beckert wrote:

Perfect, thanks!

-jc


(From earlier)

That's correct. I added that primarily because of the structures above it
(which all set visdata), and because I wasn't quite sure of ultimate
source of some of this, and what was liable to fall through or not.