| Subject: | Nasty race condition in tempory file handling |
| Date: | Tue, 21 Jan 2014 13:00:19 -0000 |
| To: | <bug-CGI [...] rt.cpan.org> |
| From: | "Steven Hartland" <killing [...] multiplay.co.uk> |
The following code has nasty race condition which can cause
problems with multiple simultaneous file uploads.
# choose a relatively unpredictable tmpfile sequence number
my $seqno = unpack("%16C*",join('',localtime,grep {defined $_} values %ENV));
for (my $cnt=10;$cnt>0;$cnt--) {
next unless $tmpfile = CGITempFile->new($seqno);
$tmp = $tmpfile->as_string;
last if defined($filehandle = Fh->new($filename,$tmp,$PRIVATE_TEMPFILES));
$seqno += int rand(100);
}
Here CGITempFile creates a file based on checks for which it
believes is a valid new and unique temporary filename.
Fh then tries to create said file but can fail if the file
already exists e.g. when two uploads are executing at the
same time.
If a Fh does fail detect a conflict another iteration of
the loop occurs to look for a new name. At this point the
previous loop iterations CGITempFile goes out of scope
which triggers its DESTORY which unlinks the file this
process never opened.
Due to this unlink a third process can then allocate the
same filename as process #1. Now if process #1 does anything
with the filename it can access via $cgi->tmpFileName it
will access the file created by #3 which is not the intention.
Ideally CGITempFile should be responsible for the creation
of the file, so the details it returns (which should include
a filehandle) are guaranteed to work.
Alternatively an additional method could be added to CGITempFile
to prevent it deleting the file so the following something
like the following code (untested) could be used.
for (my $cnt=10;$cnt>0;$cnt--) {
next unless $tmpfile = CGITempFile->new($seqno);
$tmp = $tmpfile->as_string;
last if defined($filehandle = Fh->new($filename,$tmp,$PRIVATE_TEMPFILES));
# Tell CGITempFile that the filename is invalid so it
# should not perform an unlink on delete
$tmpfile->unused();
$seqno += int rand(100);
}
sub unused {
my ($self) = @_
$$self = '';
}
sub DESTROY {
my($self) = @_;
return if ($$safe eq ''); # filename wasn't unused
$$self =~ m!^([a-zA-Z0-9_ \'\":/.\$\\~-]+)$! || return;
my $safe = $1; # untaint operation
unlink $safe; # get rid of the file
}
Finally it may be a good idea to update $seqno to generate
more random values as the current method virtually always
results in clashes for multiple simultaneous processes.
The following produces a pretty random number:
my $seqno = (localtime())[0] . $$;
Regards
Steve
================================================
This e.mail is private and confidential between Multiplay (UK) Ltd. and the person or entity to whom it is addressed. In the event of misdirection, the recipient is prohibited from using, copying, printing or otherwise disseminating it or any information contained in it.
In the event of misdirection, illegible or incomplete transmission please telephone +44 845 868 1337
or return the E.mail to postmaster@multiplay.co.uk.