Xymon history page does not work

[Ulric Eriksson]

Clicking the HISTORY button in Xymon returns the message "Cannot open history file ". This is because the function historybutton in lib/htmllog.c uses the function htmlquoted several times in an argument list to a single fprintf call. The htmlquoted function (located in lib/strfunc.c) returns a static buffer, which is of course overwritten by each call.

There are other places where htmlquoted is called several times in an argument list.

The bug is fixed in the current version of xymon, so simply upgrading the version used to build the xymon package in ubuntu would suffice.

Bug confirmed on several 12.04 LTS installations.

[TitusX (titusx)]

I support two hobbit/xymon (xymon=4.3.0~beta2.dfsg-9.1ubuntu0.1) server installed with Ubuntu. Last week I did an update from 10.04 to 12.04 and I can confirm this bug (I testet it with apache and nginx configurations). The URL of the history button is wrong. If I edit the URL manually the history is displayed correct.

Wrong:
http://<server-name>/hobbit-cgi/bb-hist.sh?HISTFILE=HISTORY.HISTORY&ENTRIES=50&IP=HISTORY&DISPLAYNAME=HISTORY

Right:
http://<server-name>/hobbit-cgi/bb-hist.sh?HISTFILE=<target>.<service>&ENTRIES=50&IP=<ip-address>&DISPLAYNAME=<target>

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Lars Kollstedt (lk-x)]

This bug also affects 10.04 (lucid) LTS, but 8.04 (hardy) is not affected yet.

The history form looks like this:
| <FORM ACTION="/hobbit-cgi/bb-hist.sh">
| <INPUT TYPE=SUBMIT VALUE="HISTORY">
| <INPUT TYPE=HIDDEN NAME="HISTFILE" VALUE="HISTORY.HISTORY">
| <INPUT TYPE=HIDDEN NAME="ENTRIES" VALUE="50">
| <INPUT TYPE=HIDDEN NAME="IP" VALUE="HISTORY">
| <INPUT TYPE=HIDDEN NAME="DISPLAYNAME" VALUE="HISTORY">
| </FORM>

Also the macros/variables for the header templates are affected. Most of them are containing the value of &BBHOST. Also in the &BBSVC and &BBIP Macros.

From my point of view this came with the last upgrade to 4.3.0~beta2.dfsg-9.1ubuntu0.1 on 12.04TLS and
4.3.0~beta2.dfsg-5ubuntu0.1 on 10.04 LTS. But I wondered that downgrading did not fix it.

[roemer2201 (roemer2201)]

I can confirm that it worked until the last update, because a downgrade helped for my xymon installation on ubuntu 12.04:

checked the version with:

apt-cache showpkg xymon

installed the old with:

apt-get install xymon=4.3.0~beta2.dfsg-5

[Lars Kollstedt (lk-x)]

Yes. It was my mistake.
I downgraded the virtual package hobbit. And that of course has not effect. Downgrading the xymon-Package fixes the problem.

[Lars Kollstedt (lk-x)]

Ulric Eriksson on 2013-01-23 wrote:
[...]
> The bug is fixed in the current version of xymon, so simply upgrading the version used to build the xymon package in ubuntu
> would suffice.
[..]
I think this is not what I want, since there are some heavy weithed changes in configuration behavior, I don't really want to have as bugfix within a LTS.

I can't find any reason for using "result" as a static variable so my suggestion is, to make it a usual local variable.

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "Making "result" a usual local variable." of this bug report has been identified as being a patch. The ubuntu-reviewers team has been subscribed to the bug report so that they can review the patch. In the event that this is in fact not a patch you can resolve this situation by removing the tag 'patch' from the bug report and editing the attachment so that it is not flagged as a patch. Additionally, if you are member of the ubuntu-reviewers team please also unsubscribe the team from this bug report.

[This is an automated message performed by a Launchpad user owned by Brian Murray. Please contact him regarding any issues with the action taken in this bug report.]

[Ulric Eriksson (ulric-g)]

The reason for making "result" static is to avoid memory leakage. Making it auto means it won't be freed. As long as the htmlquoted function is only used by cgi type programs, they will eventually exit and the memory freed anyway. I don't know if the function is used by any of the long running daemons in xymon.

[Lars Kollstedt (lk-x)]

Yes, of course. An old C++ chalenge. No automatic free when the last pointer to the dynamicaly allocated memory is removed. :-(

Since this is passed as an argument the free if neecessary must come from outside of the function. Other possible solutions should be:
   - To use mulitple static buffers, identified by a index passed to the function.
   - Taking the calls out of the argument lists, and copy the buffer imdiately in another one allocated by the function.

The first solution is IMHO "very crazy" and when using the second one you'll get a performance increase and shorter code by simply freeing within the function calling "htmlquoted".

The code where it is patched in 9-CVE-2011-1716 or 7-CVE-2011-1716 looks like this is only used in the cgi code. I'm running an patched package on an "in preparation" machine with 618 status icons since yesterday approx 14:00h. Memory usage does't look like a memory leak until now.

I don't know what workers, channels and the hobbitd or hobbitlaunch should do with this function. Since HTML-Code from the status messages is included in the site and this behavior is widely used as a feature, it does'nt make sense to escape this chars.

[Ulric Eriksson (ulric-g)]

FWIW, here's how I "solved" the problem when I found it. It's ugly, but running in production and works.

char *htmlquoted(char *s)
{
        /*
         * This routine converts a plain string into an html-quoted string
         */

// Ulric was here: use several result buffers to overcome overwriting problem

#if 0
        static strbuffer_t *result = NULL;
#else
        static strbuffer_t *result[] = {NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL};
        static int rn = 0;
        static int rn_max = sizeof(result)/sizeof(*result);
#endif

        char *inp, *endp;
        char c;

        rn++;
        if (rn >= rn_max) rn = 0;

        if (!result[rn]) result[rn] = newstrbuffer(4096);
        clearstrbuffer(result[rn]);

        inp = s;
        do {
                endp = inp + strcspn(inp, "\"&<> ");
                c = *endp;
                if (endp > inp) addtobufferraw(result[rn], inp, endp-inp);
                switch (c) {
                  case '"': addtobuffer(result[rn], "&quot;"); break;
                  case '&': addtobuffer(result[rn], "&amp;"); break;
                  case '<': addtobuffer(result[rn], "&lt;"); break;
                  case '>': addtobuffer(result[rn], "&gt;"); break;
                  case ' ': addtobuffer(result[rn], "&nbsp;"); break;
                  default: break;
                }
                inp = (c == '\0') ? NULL : endp+1;
        } while (inp);

        return STRBUF(result[rn]);
}

[Lars Kollstedt (lk-x)]

This works for me on Ubuntu 12.04 LTS Precise.

The htmlquoted funtion comes in via a patch in the debian/patches dir, so patching the patch. ;-) :-(

[Lars Kollstedt (lk-x)]

> FWIW, here's how I "solved" the problem when I found it. It's ugly, but running in production and works.
Indeed ughly. ;-) The multiple buffers solution with a static counter with a static rolling over counter.

Of course this works. Until there is one value not copied out of the buffer until the 10th time htmlqouted is called. When used to preprocess arguments for functions generating html-code this should'nt happen so fast. Then you'll have the wrong arguments in the buffers again.

This solves the problem to, and it's of course more a chose your poison question.

But I would like to have it as a simple bugfix within the LTS packages. Not inheriting things like "moving configuration to /etc/xymon" which is a real challenge I would like to face on a testing machine when upgrading to 14.04 LTS. And not as a bugfix package within 10.04 LTS.

[Lars Kollstedt (lk-x)]

But setting the pragma if to 1 would lead to an error beause rn is used below, and not declared any more. So the pragma if should IMHO better be removed, leaving in the fixed code permanently.

[Lars Kollstedt (lk-x)]

Now I'm running another patch on the "in preparation" machine. It is based on the code you (Ulric Eriksson) provided above. But i've removed the pragma if. Internaly I use 4.3.0~beta2.dfsg-9.1ubuntu0.1-0.3 so don't wonder it has the same version number as the old patch above.

It of course replaces the other one.

Thank you Ulric, the one for Lucid will follow.

[Lars Kollstedt (lk-x)]

Here also the one for Lucid. They are to understand as patch for the sourcecode of the last package (4.3.0~beta2.dfsg-5ubuntu0.1) for lucid.

[dlg (dlg.)]

The patch in #15 does not compile for me:

sha2.c:634:13: note: expected 'unsigned char *' but argument is of type 'char *'
gcc -g -O2 -Wall -Wno-unused -D_REENTRANT -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -DLINUX -I. -I/home/dlg/xymon/xymon-4.3.0~beta2.dfsg/include -I/usr/include -I. -I../include -D_FORTIFY_SOURCE=2 -c -o sig.o sig.c
sig.c: In function 'sigsegv_handler':
sig.c:56:7: warning: ignoring return value of 'chdir', declared with attribute warn_unused_result [-Wunused-result]
gcc -g -O2 -Wall -Wno-unused -D_REENTRANT -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -DLINUX -I. -I/home/dlg/xymon/xymon-4.3.0~beta2.dfsg/include -I/usr/include -I. -I../include -D_FORTIFY_SOURCE=2 -c -o stackio.o stackio.c
gcc -g -O2 -Wall -Wno-unused -D_REENTRANT -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -DLINUX -I. -I/home/dlg/xymon/xymon-4.3.0~beta2.dfsg/include -I/usr/include -I. -I../include -D_FORTIFY_SOURCE=2 -c -o strfunc.o strfunc.c
strfunc.c: In function 'htmlquoted':
strfunc.c:210:19: error: expected declaration or statement at end of input
strfunc.c:210:19: error: expected declaration or statement at end of input
strfunc.c:210:19: error: expected declaration or statement at end of input
strfunc.c:210:19: warning: control reaches end of non-void function [-Wreturn-type]
make[2]: *** [strfunc.o] Error 1
make[2]: Leaving directory `/home/dlg/xymon/xymon-4.3.0~beta2.dfsg/lib'
make[1]: *** [lib-build] Error 2
make[1]: Leaving directory `/home/dlg/xymon/xymon-4.3.0~beta2.dfsg'
make: *** [build-stamp] Error 2
dpkg-buildpackage: error: debian/rules build gave error exit status 2
dlg@build-precise:~/xymon/xymon-4.3.0~beta2.dfsg$ lsb_release -ac
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 12.04.2 LTS
Release: 12.04
Codename: precise
dlg@build-precise:~/xymon/xymon-4.3.0~beta2.dfsg$

[Lars Kollstedt (lk-x)]

Hmm... can't really reproduce this. The history_button2.patch does't really apply because it was an old-style diff -r patch, which seems to be deprecated. But that results in the patch beeing threaded as empty, not in the errors displayed above.

My steps are:
tar xzf xymon_4.3.0~beta2.dfsg.orig.tar.gz
gunzip xymon_4.3.0~beta2.dfsg-9.1ubuntu0.1.diff.gz
cd xymon-4.3.0-beta2/
cp ../xymon_4.3.0~beta2.dfsg-9.1ubuntu0.1.diff .
patch -p1 < xymon_4.3.0~beta2.dfsg-9.1ubuntu0.1.diff
cp ../historybutton_precise3.patch .
patch -p1 < historybutton_precise3.patch
fakeroot debian/rules clean
debuild -us -uc

Find the Patch from #15 in Unique Patch Format atached.

[Lars Kollstedt (lk-x)]

And of course the Lucid patch the should be also posted as Unique Patch.

[Lars Kollstedt (lk-x)]

For the ones who only like to have the Increment to 9-CVE-2011-1716, as it is needed for 10.04LTS Lucid.

[Lars Kollstedt (lk-x)]

And here the patch for the ones who only like to have the Increment to 7-CVE-2011-1716, as it is needed for 12.04LTS precise.

[Lars Kollstedt (lk-x)]

Patch from #22 included via debian/patches folder.

[Lars Kollstedt (lk-x)]

Patch from #21 (for lucid) included via debian/patches folder.

[Peter Toft (petoft)]

Anyone trying to make a package fix? This is bugging me badly

[Axel Beckert (xtaran)]

Hi Ulric,

Ulric Eriksson wrote:
> Clicking the HISTORY button in Xymon returns the message "Cannot open
> history file ". This is because the function historybutton in
> lib/htmllog.c uses the function htmlquoted several times in an argument
> list to a single fprintf call. The htmlquoted function (located in
> lib/strfunc.c) returns a static buffer, which is of course overwritten
> by each call.
[...]
> The bug is fixed in the current version of xymon, so simply upgrading
> the version used to build the xymon package in ubuntu would suffice.

Do you you refer to the 4.3.10 upstream release or the 4.3.7-1 Debian
package when you talk of "current version"?

To which changelog entry or commit do you refer when you write "The
bug is fixed in the current version"? I couldn't find any entry in the
upstream changelog which sounds similar enough to what you reported.
Additionally the only code which changed in htmlquoted() seems to
handle no or empty arguments.

  Regards, Axel
--
 ,''`. | Axel Beckert <email address hidden>, http://people.debian.org/~abe/
: :' : | Debian Developer, ftp.ch.debian.org Admin
`. `' | 1024D: F067 EA27 26B9 C3FC 1486 202E C09E 1D89 9593 0EDE
  `- | 4096R: 2517 B724 C5F6 CA99 5329 6E61 2FF9 CD59 6126 16B5

[Lars Kollstedt (lk-x)]

Hi Axel,

I can't find Ulric's patch in neither in 4.3.10 and 4.3.11 from http://xymon.sourceforge.net/ nor in 4.3.7-1. They all still contain the bug in strfunc.c:
| static strbuffer_t *result = NULL;
| char *inp, *endp;
| char c;
|
| if (!result) result = newstrbuffer(4096);
| clearstrbuffer(result);

Might be the patch was in in for a short time.

But they all use /var/lib/xymon and /etc/xymon which is not suitable to the very complex installations of Xymon Servers running Ubuntu 10.04LTS (lucid) and 12.04LTS (precise). Speaking from the Admin's view.
They are using /etc/hobbit at the moment, and might loose their whole configuration and data when this is changed withing the stable release.

The Bug came in with the 2011-1716 Patch.

And should be all affected by this Bug. The buffer result is reused before the code is copied out of it.
There are two ways to fix:
   - One way would be to remove the static. Which was set to prevent memory leaks. When doing this the memory must be freed in the calling functions again, at least when they are used in long running daemons.
   - Ulric choses the other one. This fixes the bug for a maximum amount of 10 concurrently used arguments, but is aware of producing memory leaks at all.

I'm using packages containing a modified Version of Ulric's Patch since 2013-01-29 (some days after Peters post I've put them in my PPA). They does'nt contain the
|#if 0
| static strbuffer_t *result = NULL;
|#else
and the #endif of course. This if IMHO assumes that the fix could be disabled by turning the if to 1, which wouldn't work.

I'm sorry I selected the wrong patch file in #22. I'll corect it, now.

Kind regards,
    Lars

[Axel Beckert (xtaran)]

Hi Lars,

Lars Kollstedt wrote:
> I can't find Ulric's patch in neither in 4.3.10 and 4.3.11 from
> http://xymon.sourceforge.net/ nor in 4.3.7-1. They all still contain
> the bug in strfunc.c:
>
> | static strbuffer_t *result = NULL;
> | char *inp, *endp;
> | char c;
> |
> | if (!result) result = newstrbuffer(4096);
> | clearstrbuffer(result);

Same impression here. Except for whitespace garbage and an additional
line which has been added upstream, the patch can be used against
4.3.11.

> Might be the patch was in in for a short time.

I rather expected that it was fixed in another way, but I haven't
found any evidence.

I can apply that patch for the upcoming 4.3.11-1 experimental package
if wanted. I though never ran into that issue (running
4.3.0~beta2.dfsg-6+squeeze1 on Squeeze and a 4.3.10 packaging snapshot
on Squeeze and Wheezy) and hence can't check if it works. (I also ran
4.3.7-1 for a while on Squeeze and didn't notice it there, but also
didn't use history a lot there.)

> But they all use /var/lib/xymon and /etc/xymon which is not suitable
> to the very complex installations of Xymon Servers running Ubuntu
> 10.04LTS (lucid) and 12.04LTS (precise). Speaking from the Admin's
> view.
>
> They are using /etc/hobbit at the moment, and might loose their
> whole configuration and data when this is changed withing the stable
> release.

That (and not the freeze for Wheezy) is the reason why newer Xymon
packages are just in Debian Experimental and should never have made it
into Ubuntu Quantal. :-/ We're still working on a sane automated
upgrade path and we're not yet there.

> The Bug came in with the 2011-1716 Patch.

I guess you mean CVE-2011-1716.

So 4.3.0~beta2.dfsg-9.1 is fine, but 4.3.0~beta2.dfsg-9.1ubuntu0.1
introduced this issue? Because upstream CVE-2011-1716 is only fixed
with 4.3.1. (I suspect with the same fix, since the patch matched to
the 4.3.10 or 4.3.11 code -- can't remember against which I checked.)

> And should be all affected by this Bug.

I didn't understand that sentence. Did you mean "And all should be
affected by this bug"?

> I'm sorry I selected the wrong patch file in #22. I'll corect it, now.

Thanks for the update and thanks for the patch in general!

> ** Patch added: "This is what I wanted to post in #22 (Patch for lucid for the patch dir)."
> https://bugs.launchpad.net/ubuntu/+source/xymon/+bug/1103428/+attachment/3685152/+files/lk1103428-lucid_2013-01-29.patch

This patch though has, as the one before, unnecessary whitespace
changes in it, which makes it hard to see the real functional changes.
I had to check every single line manually to actually see the
functional changes. Could you please post a patch without changing the
indentation of unchanged lines? TIA!

P.S.:

Launchpad Bug Tracker schrieb am Thu, May 23, 2013 at 05:58:21PM -0000:
> You have been subscribed to a public bug by Lars Kollstedt (lk-x):

Huh, why that? I'm already subscribed for all bugs affecting Xymon in
Ubuntu, so this shouldn't be necessary. (Nor change anything, so I
wonder why Launchpad notifies me about a no-op...)

  Regards, Axel
--
 ,''`. | Axel Beckert <email address hidden>, http://people.debian.org/~abe/
: :' : | ...

[Lars Kollstedt (lk-x)]

Posting a modified version of my last mail to Axel Beckert... with some updates:

On Thuesday, 23. May 2013, 22:32:33 Axel Beckert wrote:
[...]
> I rather expected that it was fixed in another way, but I haven't
> found any evidence.

Indeed they did:
| web/history.c: fprintf(htmlrep, "<BR><FONT %s><B>%s",
| xgetenv("XYMONPAGEROWFONT"), htmlquoted(displayname));
| web/history.c: fprintf(htmlrep, " - %s</B></FONT><BR>\n",
| htmlquoted(service));
| web/history.c: sprintf(p, "&amp;IP=%s", htmlquoted(ip))

They seperated all calls to htmlquoted, and called the function each time a
variable from the HTTP-Request is inserted into the HTML-Code. The code using
the htmlqouted-Function looks quite different from the code I read in January.

The old code did this when the code was passed to the output funtion. And it
did many calls before copying the strings out of the buffer.

Things like this:
 | sethostenv(htmlquoted(displayname), htmlquoted(ip), htmlquoted(service),
 | colorname(color), htmlquoted(hostname));
And this example results in the hostname beeing inserted as color, service, ip
and displayname also.

I had a look to xymon-4.3.11, and found 94 different calls to "htmlquoted".
The old code of 4.3.0~beta2.dfsg-5ubuntu0.1 and 4.3.0~beta2.dfsg-9.1ubuntu0.1
contains only a few, about 8 ... 10 calls when I remember this correct.

This changes of course would also fix the bug. And it fixes it in a more clean
way. But is much more modification to the Code than Ulric's fix in the
htmlqouted function itself.

I think the new fix would be much better to debug for future releases of
xymon. But I'm not sure it is a good idea to port it back to a stable release
or even to a LTS release, since many scattered lines of change are IMHO many
possibilities for mistakes.

>
> I can apply that patch for the upcoming 4.3.11-1 experimental package
> if wanted.
I don't think it makes sense to integrate it there under this circumstances.

I'm more interessted to get a patch for this issue into the packages for
precise and lucid. At least into future patched versions.

The version 4-3-7 already contains the new bugfix code. I didn't have a look at the packages for quantal yet.

> I though never ran into that issue (running
> 4.3.0~beta2.dfsg-6+squeeze1 on Squeeze and a 4.3.10 packaging snapshot
> on Squeeze and Wheezy) and hence can't check if it works. (I also ran
> 4.3.7-1 for a while on Squeeze and didn't notice it there, but also
> didn't use history a lot there.)
The error is absolutely reproduceable. My very first quickfix was some
javascript-Code in the page footer. But this didn't fix the headlines on the
details and history pages. It only made the history button working again,
by filling the forms again with corrected values.

This was on Thu, 17 Jan 2013, one day after the bug has also reached lucid.

>
> > The Bug came in with the 2011-1716 Patch.
>
> I guess you mean CVE-2011-1716.
Yes, I cut the leading 7 since it has a different number in this place for
lucid/precis and also cut the CVE by mistake.

> I didn't understand that sentence. Did you mean "And all should be
> affected by this bug...

[Lars Kollstedt (lk-x)]

And as I already said. I did get the version dependency out. So here is the patch file without whitespace garbage for lucid.

[Chris Brun (cbrun)]

Just installed xymon on ubuntu server .... used the apt-get install to get the latest, so I'm at 4.3.0-0.beta2. I have the exact problem as titusx described. The history works if I manually edit the URL. I see the patch, but my question is what do I do with it...how do I apply it to my system? Thanks.

[Lars Kollstedt (lk-x)]

Dear Mr. Brun,

on Friday, 4. April 2014, 18:07:05 you wrote:
> Just installed xymon on ubuntu server .... used the apt-get install to
> get the latest, so I'm at 4.3.0-0.beta2. I have the exact problem as
> titusx described. The history works if I manually edit the URL. I
> see the patch, but my question is what do I do with it...how do I apply
> it to my system? Thanks.

The patches discussed here have not been taken into the offical ubuntu
repositories yet. The bug is now in for more than a year and destroys some
important features of the xymon webfrontend.

The Bug is fixed in the xymon packages of Ubuntu 13.4 (raring) and later, by
modifying much more code than the patches showed here. But this packages also
are containing modifications not suitable for a bugfix version of packages and
are requiring lots of modifications when upgrading in a complex xymon
environment.

The most importent changes are taking:
/etc/hobbit to /etc/xymon
/etc/hobbit/hobbitlauch.d to /etc/launch.d
/etc/hobbit/bb-hosts to /etc/xymon/hosts
/usr/lib/hobbit to /usr/lib/xymon
/var/lib/hobbit to /var/lib/xymon

All Config-Variables in hobbitserver.cfg and hobbit-client.cfg formerly
containing the word HOBBIT are changed to XYMON. There might be some more
changing, since I had a look at this new packages, but I'm still using only
the old ones.

If you are installing a new xymon server this packages or even the ubuntu
versions using it might be a good choice. Starting with one of these newer
packages might prevent you from changing all the stuff mentioned above.

But I think it should be clear to everyone what this means to existing
installations.

How to use the patches:

The Patches from #29 an #30 are applied to the packages for precise and lucid
by copying them into the debian/patches of the source code and appending them
to debian/patches/serial.

The Packages resulting from this, including the source-code can be found in my
ppa:

https://launchpad.net/~lk-x/+archive/ppa

Hope this was helpful.

Kind regards,
 Lars

--
Achtung: Seit 28. August 2013 neue Rufnummern!

man-da.de GmbH, AS8365 Phone: [phone]
Mornewegstraße 30 Fax: [phone]
D-64293 Darmstadt e-mail: <email address hidden>
Geschäftsführer Marcus Stögbauer AG Darmstadt, HRB 94 84

[Axel Beckert (xtaran)]

Marking as "fix released" since this has been fixed since the upload of 4.3.7-1 to Ubuntu and is not present in more recent Ubuntu releases (including 14.04 LTS).

I don't seem to have the permission to mark single releases as affected. So someone with those permissions should probably do that. I was just able to set according tags.