The new version of p5-Net-Server has a fix for a problem I found earlier that prevents munin-node from restarting on a HUP signal. But the fix is incomplete because it does not take perl's taint mode into account. I have created a ticket on rt.cpan.org: http://rt.cpan.org//Ticket/Display.html?id=17090 Fix: This fix also corrects another problem - relative pathes are not recognized except when they start with a '.'. How-To-Repeat: Install sysutils/munin-node. Start munin-node. Send the process a HUP.
On Mon, Jan 16, 2006 at 04:44:07PM +0100, Lupe Christoph wrote: > --- Net-Server.patch begins here --- > --- /usr/local/lib/perl5/site_perl/5.8.7/Net/Server.pm.orig Mon Dec 5 22:13:04 2005 > +++ /usr/local/lib/perl5/site_perl/5.8.7/Net/Server.pm Mon Jan 16 16:26:49 2006 ... > Can you please submit a patch that applies to the port, not the installed software. E.g. /usr/ports/net/p5-Net-Server not /usr/local/lib/perl5/site_perl/5.8.7/ You may do * backup p5-Net-Server to p5-Net-Server.orig * make extract * prepare a patch relative to (should be named like patch-something) /usr/ports/net/p5-Net-Server/work/Net-Server-0.90/ and place it in /usr/ports/net/p5-Net-Server/files * send the result from diff -urN p5-Net-Server.orig p5-Net-Server before your changes. -- Vasil Dimov
On Tuesday, 2006-01-24 at 11:03:21 +0200, Vasil Dimov wrote: > On Mon, Jan 16, 2006 at 04:44:07PM +0100, Lupe Christoph wrote: > > --- Net-Server.patch begins here --- > > --- /usr/local/lib/perl5/site_perl/5.8.7/Net/Server.pm.orig Mon Dec 5 22:13:04 2005 > > +++ /usr/local/lib/perl5/site_perl/5.8.7/Net/Server.pm Mon Jan 16 16:26:49 2006 > ... > Can you please submit a patch that applies to the port, not the > installed software. > E.g. /usr/ports/net/p5-Net-Server not /usr/local/lib/perl5/site_perl/5.8.7/ As you wish, master! ;-) diff -ruN p5-Net-Server.orig/files/patch-Server.pm p5-Net-Server/files/patch-Server.pm --- p5-Net-Server.orig/files/patch-Server.pm Thu Jan 1 01:00:00 1970 +++ p5-Net-Server/files/patch-Server.pm Wed Jan 25 08:21:13 2006 @@ -0,0 +1,21 @@ +diff -ruN lib/Net/Server.pm lib/Net/Server.pm +--- lib/Net/Server.pm Mon Dec 5 22:13:04 2005 ++++ lib/Net/Server.pm Wed Jan 25 08:18:25 2006 +@@ -133,6 +133,7 @@ + ### see if we can find the full command line + if (open _CMDLINE, "/proc/$$/cmdline") { # unix specific + my $line = do { local $/ = undef; <_CMDLINE> }; ++ ($line) = $line =~ /^(.*)$/; # untaint + close _CMDLINE; + if ($line) { + return [split /\0/, $line]; +@@ -140,7 +141,8 @@ + } + + my $script = $0; +- $script = $ENV{'PWD'} .'/'. $script if $script =~ m|^\.+/| && $ENV{'PWD'}; # add absolute to relative ++ $script = $ENV{'PWD'} .'/'. $script if $script !~ m|^\//| && $ENV{'PWD'}; # add absolute to relative ++ ($script) = $script =~ /^(.*)$/; # untaint + return [ $script, @ARGV ] + } + Lupe Christoph -- | You know we're sitting on four million pounds of fuel, one nuclear | | weapon and a thing that has 270,000 moving parts built by the lowest | | bidder. Makes you feel good, doesn't it? | | Rockhound in "Armageddon", 1998, about the Space Shuttle |
Responsible Changed From-To: freebsd-ports-bugs->vd I will take it
On Mon, Jan 16, 2006 at 04:44:07PM +0100, Lupe Christoph wrote: > - $script = $ENV{'PWD'} .'/'. $script if $script =~ m|^\.+/| && $ENV{'PWD'}; # add absolute to relative > + $script = $ENV{'PWD'} .'/'. $script if $script !~ m|^\//| && $ENV{'PWD'}; # add absolute to relative 1. Probably you made a typo with that double slash. Now it prepends the current directory even if $script is "/a". Also the slash need not be esaped, so the correct one is: $script = $ENV{'PWD'} .'/'. $script if $script !~ m|^/| && $ENV{'PWD'}; # add absolute to relative 2. Why do you need to untaint the $script variable in p5-Net-Server? If there is some problem in munin-node shouldn't the variable be untainted there, thus not changing p5-Net-Server because other software that uses p5-Net-Server may depend on that variable being tainted? 3. 2. applies to the $line variable also but I would say that this code: if (open _CMDLINE, "/proc/$$/cmdline") { # unix specific my $line = do { local $/ = undef; <_CMDLINE> }; + ($line) = $line =~ /^(.*)$/; # untaint is not reached at all, because there is no /proc/ by default in recent FreeBSD versions. What do you think? -- Vasil Dimov
Quoting Vasil Dimov <vd@FreeBSD.org>: > On Mon, Jan 16, 2006 at 04:44:07PM +0100, Lupe Christoph wrote: > > - $script = $ENV{'PWD'} .'/'. $script if $script =~ m|^\.+/| && > $ENV{'PWD'}; # add absolute to relative > > + $script = $ENV{'PWD'} .'/'. $script if $script !~ m|^\//| && > $ENV{'PWD'}; # add absolute to relative > 1. Probably you made a typo with that double slash. Now it prepends the > current directory even if $script is "/a". Also the slash need not be > esaped, so the correct one is: > $script = $ENV{'PWD'} .'/'. $script if $script !~ m|^/| && $ENV{'PWD'}; # add > absolute to relative I think you're right with this. > 2. Why do you need to untaint the $script variable in p5-Net-Server? If > there is some problem in munin-node shouldn't the variable be untainted > there, thus not changing p5-Net-Server because other software that uses > p5-Net-Server may depend on that variable being tainted? Because p5-Net-Server reads the path itself. It is nothing supplied by the server-specific code. It is only used by the Net::Server code. > 3. 2. applies to the $line variable also but I would say that this code: > if (open _CMDLINE, "/proc/$$/cmdline") { # unix specific > my $line = do { local $/ = undef; <_CMDLINE> }; > + ($line) = $line =~ /^(.*)$/; # untaint > is not reached at all, because there is no /proc/ by default in recent > FreeBSD versions. I used the same changes I sent to the module author. Linux and Solaris will have it by default. And FreeBSD machines that do mount procfs *will* have /proc/$$/cmdline, AFAIK (I have no access to a FreeBSD system where I am now). That means that the code that tries to use /proc/$$/cmdline will be used on those machines, and if $line is tainted (having been read from a file), so Munin will fail to restart. And any other daemon using Net::Server that also uses taint mode as it should IMNSHO. > What do you think? I think you should correct my mistake with the m|^\//| but leave the untainting as it is. Thank you, Luep Christoph -- | lupe@lupe-christoph.de | http://www.lupe-christoph.de/ | | "... putting a mail server on the Internet without filtering is like | | covering yourself with barbecue sauce and breaking into the Charity | | Home for Badgers with Rabies. Michael Lucas |
State Changed From-To: open->closed Committed with m|^//| changed to m|^/|, thanks!