Bug 78095 (CVE-2005-1127)

FYI 
http://w3d.suse.de/Dev/Components/Packages/PackMan/pm_pr_fixing_bug.html#pm_pr_fb_bt_security_bugs 

I created a patch:
--- Net-Server-0.87/lib/Net/Server.pm.ORG       2005-04-18 15:22:18.002797897 +0200
+++ Net-Server-0.87/lib/Net/Server.pm   2005-04-18 15:22:25.153319236 +0200
@@ -1046,7 +1046,9 @@
   ### log only to syslog if setup to do syslog
   if( $prop->{log_file} eq 'Sys::Syslog' ){
     $level = $level!~/^\d+$/ ? $level : $Net::Server::syslog_map->{$level} ;
-    Sys::Syslog::syslog($level,@_);
+    ### check for formatstring, if none take only the first string
+    my $fstring = sprintf shift @_, @_;
+    Sys::Syslog::syslog($level, $fstring);
     return;
   }

that checks for a formatstring and uses this, otherwise takes the first string

submitted for

9.0,9.1.9.2, 9.3

this is not the correct way, you actually open a new hole. 
 
Note that the problem is that the first element of @_ is evaluated 
as format string and can be chosen by an attacker. 
 
Your fix just lets it evaluate by sprintf ... this might be 
implemented in perl and not as dangerous as before. 
 
However you then pass the string to syslog again unprotected, 
so an attacker just needed %% to a get a % and then could proceed 
as before. 
 
Please use: 
 
-    Sys::Syslog::syslog($level,@_); 
+    Sys::Syslog::syslog($level, "%s", @_); 
 
as a better approach. Reading the source does not show any call passing 
more than 1 argument to the function. 

So this would be what keeps all arguments, in case there is a formatstring in @_
and make sure it is not misinterpreted as formatstring:

-    Sys::Syslog::syslog($level,@_);
+    ### check for formatstring, if none take only the first string
+    my $fstring = sprintf shift @_, @_;
+    Sys::Syslog::syslog($level, "%s", $fstring);
     return;
   }

right?
(Just to make sure to not have an impact as described in the comment#1)

partially. 
 
sprintf is still evaluating the % xpressions in the first 
array element. 
 
check this code: 
#!/usr/bin/perl 
 
sub foo { 
 
        my $str = sprintf shift @_, @_; 
 
        print STDERR "$str" . "\n"; 
} 
 
 
foo('hallohallo','bar'); 
foo('hallo%shallo','bar'); 
foo('%s'); 
foo('hallo%%shallo','bar'); 
foo('%n'); 
 
 
$ perl xx.pl 
hallohallo 
hallobarhallo 
 
hallo%shallo 
Modification of a read-only value attempted at xx.pl line 5. 
$  
 
the string gets evaluated.  
 
i think we should skip the sprintf step and just allow one argument which is 
printed verbatim (my suggeste4d patch) 

Ok, after some discussion, I changed the patch accordingly.

submitted for 8.2 9.0 9.1 9.2 9.3 STABLE

swampid: 969  

pinfos submitted. 

I don't see how this is exploitable as SyS::Syslog::syslog() uses Perl's 
sprintf and I don't see how this can be exploited. This isn't C, you know... 
 
I'm not sure if it is wise to cripple the log() function. If there any 
feedback from the author of Net-Server? 

i agree, it only seem to be passed to perl sprintf. 
 
However, the whole module just uses ->log(level,string); and does 
not use format strings as far as I can see. 
 
Also, there is one log() call in perl-Net-Server where it feeds userdata 
directly from the user input and must be fixed. (this is the original cause of 
problem) 
 
$self->log(5,$_); # very verbose log 
should be: 
$self->log(5,"%s",$_); # very verbose log 

Yeah, but it's just a DoS, you can't get root with it. I don't mind changing 
the log function when the author of Net-Server says it's ok, otherwise we 
should change the call to log(). 

contacted author on what is he best fix. 

Marcus,
does he respond?

not until now, no. 
 
I think we should be treating this as a security bug, but as a "bug bug" only. 

CAN-2005-1127 

no feedback received yet. 
 
I think we should proceed with our fix as-is. 
 
mls, please checkin patchinfos... 

Please fix the patchinfo files. This cannot be used to execute code, it is 
just a DoS. 

done 

packages approved 

CVE-2005-1127: CVSS v2 Base Score: 5.0 (AV:N/AC:L/Au:N/C:N/I:N/A:P)