Subject: Re: [Pkg-cacti-maint] error in cacti

From: Paul Gevers <[email-masked]>
Date: Sat, 21 Jan 2012 10:18:01 +0100

In the mean time, a proper bug was filed for this issue [1], and I have
committed the patch to the packaging Git repo [2]. Mahyuddin, you were
quite successful in getting the previous security update into Lenny.
What are the steps needed to get it in? Do you want to do it now as well?

In other words, how do we go from here.

Paul

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=656613
[2]
http://anonscm.debian.org/gitweb/?p=pkg-cacti/cacti.git;a=log;h=refs/heads/debian-lenny

---

From: Paul Gevers <[email-masked]>
Date: Sun, 22 Jan 2012 10:06:19 +0100

Hi all,

>> I haven't seen any response to the latest mail related to the
>> regression in cacti old-stable. Kostya, could you please file the
>> bug with reportbug. Also I have not been able to reproduce the
>> problem, but I think you are complete right. So what exactly did
>> you do to get the bug to appear? If you don't want to file the bug
>> yourself, please let us know. Can I use your text in the first
>> e-mail for it if your not filing yourself?
> 
> Ok, now we have confirmation that there is a regression.

I also confirm myself that there is regression.

>> Mayuddin, I think your patch is not right. It changes the interface
>> of the function, where all the calls to it don't change. I attach a
>> patch that I believe is better. Shall I commit it to the git
>> repository?
> 
> The interface of the function can be extended as long as one makes
> sure to add default values for the extra arguments. This easies
> backports of other fixes and is generally cleaner than hardcoding
> within the code.

Ok. I agree that it is cleaner, but in newer upstream code, the max_oid
argument is not the last argument of the function. I am not so much into
php that I can tell if the order of your arguments make a difference,
but as the arguments are unnamed, I expect that you can not add extra
arguments in front of existing argument without careful checking. The
point is, I am afraid of introducing even more regressions. That is why
I prefer to revert to the hardcoded value. But as I am not so
experienced in these kind of things in security fixes, I am willing to
make the changes as you propose today, but can somebody check them?
Please let me know a.s.a.p.

>> but where exactly should we ask for
>> security help? You, Luk?
> 
> If we agree on the patch, I can indeed proceed with the process of
> getting the regression fix in the security (or maybe the general) archive.

What do you mean with the general archive? That the security fix with
regression fix is moved to the lenny archive?

Paul

---

From: Paul Gevers <[email-masked]>
Date: Sun, 22 Jan 2012 16:39:33 +0100

> Please let me know a.s.a.p.

Hmm, I really don't see how this can be done without changing quite a
lot of code. cacti_snmp_walk is called in 5 files:

./lib/data_query.php
./scripts/ss_host_cpu.php
./scripts/query_host_cpu.php
./scripts/query_host_partitions.php
./scripts/ss_host_disk.php

And as upstream put the max_oids argument as the next-to-last argument
we need to change all those functions. But what to put in that field?
The max_oids is not available from somewhere as-is (see upstream bug

---

From: Paul Gevers <[email-masked]>
Date: Sun, 22 Jan 2012 16:47:03 +0100

Oops, the mail was not meant to be sent yet. Finished here.

>> Please let me know a.s.a.p.

Hmm, I really don't see how this can be done without changing quite some
of the code not fully in line with upstream. cacti_snmp_walk is called
in 5 files:

./lib/data_query.php

And as upstream put the max_oids argument as the next-to-last argument
we need to change all those calls. But what to put in that field? The
max_oids is not available from somewhere as-is (see e.g. upstream bug
1882 [1]). My *guess* is that we *could* add a
read_config_option("max_get_size") to all the calling functions, or even
a zero, and add the following check to cacti_snmp_walk:

/* determine default max_oids */
if (($max_oids == 0) || (!is_numeric($max_oids))) {
$max_oids = read_config_option("max_get_size");
if ($max_oids == "") $max_oids = 10;
}

Do we want this? I prefer to have your response before I continue.

Paul

[1] http://bugs.cacti.net/view.php?id=1882

---

From: Luk Claes <[email-masked]>
Date: Sun, 22 Jan 2012 20:27:44 +0100

Paul

Please have a look at #656816, I just put the max_oids argument as
last argument as not having it is worse than having it as the one but
last argument IMHO.

Cheers

Luk

On 01/22/2012 04:47 PM, Paul Gevers wrote:
>>> Ok. I agree that it is cleaner, but in newer upstream code, the
>>> max_oid argument is not the last argument of the function. I am
>>> not so much into php that I can tell if the order of your
>>> arguments make a difference, but as the arguments are unnamed,
>>> I expect that you can not add extra arguments in front of
>>> existing argument without careful checking. The point is, I am
>>> afraid of introducing even more regressions. That is why I
>>> prefer to revert to the hardcoded value. But as I am not so 
>>> experienced in these kind of things in security fixes, I am
>>> willing to make the changes as you propose today, but can
>>> somebody check them? Please let me know a.s.a.p.
>> 
> 
> Hmm, I really don't see how this can be done without changing quite
> some of the code not fully in line with upstream. cacti_snmp_walk
> is called in 5 files:
> 
> ./lib/data_query.php ./scripts/ss_host_cpu.php 
> ./scripts/query_host_cpu.php ./scripts/query_host_partitions.php 
> And as upstream put the max_oids argument as the next-to-last
> argument we need to change all those calls. But what to put in that
> field? The max_oids is not available from somewhere as-is (see e.g.
> upstream bug 1882 [1]). My *guess* is that we *could* add a 
> read_config_option("max_get_size") to all the calling functions, or
> even a zero, and add the following check to cacti_snmp_walk:
> 
> /* determine default max_oids */ if (($max_oids == 0) ||
> (!is_numeric($max_oids))) { $max_oids =
> read_config_option("max_get_size"); if ($max_oids == "") $max_oids
> = 10; }
>

---

From: Paul Gevers <[email-masked]>
Date: Sun, 22 Jan 2012 20:56:23 +0100

Luk,

Fine with me, but still. Your argument was that back ports would be
easier, but I believe you make it slightly more difficult, as the
arguments are now all there, but in different order than in newer
upstream code. I think that will be very difficult to catch if somebody
needs to catch it. But then again, this is more compatible to additions
that people might have made locally.

By the way, why a new bug? The problem described in the bug is 100% the
same as #656613.

And of course, thanks for taking care.

Paul

---

From: Luk Claes <[email-masked]>
Date: Sun, 22 Jan 2012 22:40:32 +0100

Hi Paul

The release.debian.org bug is used in the release process, while the
other one is the actual bug in cacti.

The order of the arguments only matters when changing either the calls
of the function or the definition of the function. It does not matter
for the use of max_oids within the function.

Cheers

Luk

On 01/22/2012 08:56 PM, Paul Gevers wrote:
> Luk,
> 
> Fine with me, but still. Your argument was that back ports would
> be easier, but I believe you make it slightly more difficult, as
> the arguments are now all there, but in different order than in
> newer upstream code. I think that will be very difficult to catch
> if somebody needs to catch it. But then again, this is more
> compatible to additions that people might have made locally.
> 
> By the way, why a new bug? The problem described in the bug is 100%
> the same as #656613.
> 
>> Please have a look at #656816, I just put the max_oids argument
>> as last argument as not having it is worse than having it as the
>> one but last argument IMHO.
>> 
>> Cheers
>> 
>> Luk
>> 
>>>>> Ok. I agree that it is cleaner, but in newer upstream code,
>>>>> the max_oid argument is not the last argument of the
>>>>> function. I am not so much into php that I can tell if the
>>>>> order of your arguments make a difference, but as the
>>>>> arguments are unnamed, I expect that you can not add extra
>>>>> arguments in front of existing argument without careful
>>>>> checking. The point is, I am afraid of introducing even
>>>>> more regressions. That is why I prefer to revert to the
>>>>> hardcoded value. But as I am not so experienced in these
>>>>> kind of things in security fixes, I am willing to make the
>>>>> changes as you propose today, but can somebody check them?
>>> Hmm, I really don't see how this can be done without changing
>>> quite some of the code not fully in line with upstream.
>>> cacti_snmp_walk is called in 5 files:
>>> 
>>> ./scripts/query_host_partitions.php ./scripts/ss_host_disk.php
>>> 
>>> argument we need to change all those calls. But what to put in
>>> that field? The max_oids is not available from somewhere as-is
>>> (see e.g. upstream bug 1882 [1]). My *guess* is that we *could*
>>> add a read_config_option("max_get_size") to all the calling
>>> functions, or even a zero, and add the following check to
>>> cacti_snmp_walk:
>>> 
>>> read_config_option("max_get_size"); if ($max_oids == "")
>>> $max_oids = 10; }
>>>

---

From: Paul Gevers <[email-masked]>
Date: Thu, 02 Feb 2012 21:00:57 +0100

Luk,

Unfortunately that bug was closed, as not appropriate for normal release
update. Do you take the actions necessary to get the bug fixed in security?

Paul

---

From: Luk Claes <[email-masked]>
Date: Sat, 04 Feb 2012 16:53:11 +0100

Hi Paul

Sorry, I had exams yesterday, so it took a bit longer. There should
follow a DSA soon now (package is uploaded and DSA is prepared).

Cheers

Luk