Delivery-Date: Fri, 26 Jan 2001 19:38:05 +0000
Approved-By: of@SECURITYFOCUS.COM
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Importance: Normal
Message-ID:  <000e01c08736$b9b674a0$6400000a@internal.home.blockdev.net>
Date:         Thu, 25 Jan 2001 20:24:22 -0500
Reply-To: Matt Block <senior@HOME.BLOCKDEV.NET>
Sender: Secure Programming Mailing List <SECPROG@SECURITYFOCUS.COM>
From: Matt Block <senior@HOME.BLOCKDEV.NET>
Subject:      Re: Does anyone see a problem with this code?
To: SECPROG@SECURITYFOCUS.COM
In-Reply-To:  <3A6A440E.7030808@paulbaker.net>

Paul,

If I understand your specification, all that is important (security-wise)
for this system is that it not be used to clobber anything else on the
system.  You don't particularly care if the file gets mangled (so doing
more than fstat is a waste of energy,) or very large (or you wouldn't use
/tmp) or ...

Your code appears to guarantee that other files won't get clobbered in
two ways.  One, by checking for symlinks.  Two, by running with the
priveleges of whomever called the program.  Let me suggest a third way
that might make improve things even a bit more... make it suid <nonsense>
user and world executable.  Then make sure that this file is the only
thing on the system that <nonsense> owns.  This has a side benefit of
making the file only <nonsense> writeable, meaning this program will be
the only thing that writes to it (so flock() and append will actually
have meaning).

  -- Matt
  Brainbench Linux MVP

-----Original Message-----
From: Secure Programming Mailing List
[mailto:SECPROG@SECURITYFOCUS.COM]On Behalf Of Paul J. Baker
Sent: Saturday, January 20, 2001 9:06 PM
To: SECPROG@SECURITYFOCUS.COM
Subject: Does anyone see a problem with this code?


I need to keep a world read/writeable file so multiple processes can
stat this file to see if one of the others have made any updates to a
database. This file will be stored in /tmp and since its world writable
this could present some security risks if not taken care of properly.
Does anyone see any problems with this code?

Thanks,
Paul Baker

===================================

my $_dbstatfh;  # file handle of the stat file
my $dbstatsrc = '/tmp/dbstat';  # location of the file

sub _dbstat {

     my $self = shift;
     my $ts = shift;

     # big avoiding security holes through race conditions stuff
     # first check if the file is already open (safe)
     unless($_dbstatfh) {
         # file is not open (unsafe), check if it exists
         if ( -e $dbstatsrc ) {
             # file exists. open it but do not hurt it
             $_dbstatfh = IO::File->new($dbstatsrc, O_RDWR)
                 or confess "dbstat file could not be opened: $!";
         } else {
             # the file does not exist. lets create the file by these
specifications
             ## only create file if it does not already exist
             ## must be world rw-able
             my $tmp_umask = umask 0000;
             $_dbstatfh = IO::File->new($dbstatsrc, O_RDWR|O_EXCL|O_CREAT)
                 or confess "dbstat file race condition detected: $!";
             umask $tmp_umask;
         }
         # file is opened (unsafe), check that it is not a symlink
         -l $_dbstatfh and confess "dbstat file is a symlink!! system
integrity has been comprimised!!";
         # if we got this far, file is safe
     }

     if (defined $ts) {
         # setting new timestamp
         ## lock for write
         flock($_dbstatfh, Fcntl::LOCK_EX);
         ## seek to end of file for append
         seek($_dbstatfh, 0, 2);
         ## write timestamp
         print $_dbstatfh "$ts\n";
         ## unlock
         flock($_dbstatfh, Fcntl::LOCK_UN);
     }

     # return current timestamp
     return (stat($_dbstatfh))[9];

}