[ net-snmp-Bugs-1203376 ] net-snmp fixproc insecure temporary file creation

[SourceForge.net <no...@so...>]

Bugs item #1203376, was opened at 2005-05-17 10:12
Message generated for change (Comment added) made by tanders
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=112694&aid=1203376&group_id=12694

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: security
Group: linux
Status: Open
Resolution: None
Priority: 5
Submitted By: eromang (eromang)
Assigned to: Nobody/Anonymous (nobody)
Summary: net-snmp fixproc insecure temporary file creation

Initial Comment:
Hello,

My name is Eric Romang from ZATAZ.net (er...@za...)

Just take a look at /usr/bin/fixproc

Line 233 :

# it must be "shell", so execute the shell script
defined in database

local ($tmpfile) = "/tmp/fix_$$";
 	
&create_sh_script ($fix{$proc}, $tmpfile);
 
# return code is number divided by 256
$error_code = (system "$tmpfile") / 256;

-----------------------------------------

We see that the tmp file is created with $$ value and
this script is execute by the perl system command

The subfunction do only this :

-------------------------------------

sub create_sh_script
{
  local ($file) = pop (@_);
  local ($i) = pop (@_);

  printf (stderr "create_sh_script\n") if ($debug > 0);

  $! = $fixproc_error;
  open (file, ">"."$file") || die "$0: cannot open
$file\n";
  while ( $shell_lines[$i] ne $shell_end_marker )
    {
      printf (file "%s", $shell_lines[$i]);
      $i++;
    }
  close (file);
  system "chmod +x $file";
  return file;
}

----------------------------------------

My knowledge in perl is not so good, but maybe a race
condition could be exploited here, and permit to a
basic user to run arbitrairie commands on the system
with root privileges ?

The same for :

sub do_check
{
  local ($proc) = pop(@_);

  printf (stderr "do_check\n") if ($debug > 0);

  if ($check{$proc} eq '')
    {
      $! = $fixproc_error;
      die "$0: internal error 2\n";
    }

  if ($check{$proc} ne 'exist')
    {
      # if not "exist", then it must be "shell", so
execute the shell script
      # defined in database

      local ($tmpfile) = "/tmp/check_$$";

      &create_sh_script ($check{$proc}, $tmpfile);

        # return code is number divided by 256
      $error_code = (system "$tmpfile") / 256;
      system "rm $tmpfile";
      return ($check_failed_error) if ($error_code != 0);

      # check passed, continue
    }
  return &do_exist ($proc);
}

Regards?



----------------------------------------------------------------------

Comment By: Thomas Anders (tanders)
Date: 2005-05-24 14:10

Message:
Logged In: YES 
user_id=848638

Wes has committed a fix (using File::Temp) to all 5.x.y
branches. This bug can be closed now.

----------------------------------------------------------------------

Comment By: Thomas Anders (tanders)
Date: 2005-05-23 18:15

Message:
Logged In: YES 
user_id=848638

Here's a proposed patch against CVS MAIN, utilizing
mktemp(1), taken from the SuSE Linux 9.3 RPM (net-snmp 5.2.1).

- --- snip ---
--- /bc/net-snmp-5.3cvs/bin/fixproc     2005-05-20
14:44:30.000000000 +0200
+++ /usr/bin/fixproc    2005-03-19 21:16:16.000000000 +0100
@@ -231,7 +231,7 @@
     {
       # it must be "shell", so execute the shell script
defined in database

-      local ($tmpfile) = "/tmp/fix_$$";
+      local ($tmpfile) = `mktemp /tmp/fix.XXXXXXXX`;

       &create_sh_script ($fix{$proc}, $tmpfile);

@@ -262,7 +262,7 @@
       # if not "exist", then it must be "shell", so execute
the shell script
       # defined in database

-      local ($tmpfile) = "/tmp/check_$$";
+      local ($tmpfile) = `mktemp /tmp/check.XXXXXXXX`;

       &create_sh_script ($check{$proc}, $tmpfile);

- --- snap ---

----------------------------------------------------------------------

Comment By: Thomas Anders (tanders)
Date: 2005-05-23 17:43

Message:
Logged In: YES 
user_id=848638

Indeed, using "$$" smells a lot like insecure temporary file
handling. We should really consider using safer approaches
like described in e.g.

http://www.opennet.ru/base/audit/18.txt.html

Do we consider this a show-stopper for 5.0.10? I'd vote to
fix this *before* release.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=112694&aid=1203376&group_id=12694