Greetings,

There was recently a fairly long and involved thread on BUGTRAQ or
VULN-DEV discussing temp files. I believe the general consensus is as
follows:

1. Keeping temp files in world-writable /tmp is a Bad Thing (tm). It's
better to keep them in a directory where only authorized users can read
them. Using group permissions, it should be possible to place temp files
somewhere, even in a subdirectory of /tmp, if you like, where not everyone
in the whole world can fiddle with it.

2. Keeping world-writable files in /tmp is an Even Worse Thing (tm). This
practice opens you up to malicious users filling your /tmp disk and, if
you use tmpfs, your swap space.

3. If you must put temp files in /tmp, don't use a predictable filename,
such as a constant, $$, $$^$$, or any of the other classics. Use a
library function such as mkstemp to create a unique file that doesn't
already exist.

With respect to your code, the only problem I can see (aside from the
tmpfile handling) is that it might not do what you want it to do, since I
don't see the filehandle $_dbstatfh being closed anywhere, and the mtime
on the file is only updated when the file is closed or flushed. Of
course, if you did a "select $_dbstatfh; $| = 1;" (enable autoflush)
prior to calling this function, that would take care of that problem.

Regards,
Pedro

On Sat, 20 Jan 2001, Paul J. Baker wrote:

> 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];
>
> }
>

• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]