Item10408: Temporary File Hazard in Sandbox.pm and FileCache.pm

pencil
Priority: Urgent
Current State: Closed
Released In: 1.1.3
Target Release: patch
Applies To: Engine
Component: Sandbox.pm
Branches:
Reported By: RaymondLutz
Waiting For:
Last Change By: KennethLavrsen
Sandbox.pm (line 509) uses File::Spec->tmpdir() to get the name of a temporary director (normally '/tmp') and then creates a file named $$.'stderr', where $$ is the process ID.

I have found that with an installation where separate installs of foswiki exist with different users, this will create an error if the code encounters a file with the same proposed name but under a different user ID. The code "dies" at that point. This has been verified in error.log, etc. This bug results in significant loss of stability, i.e. frequent server crashes (but it may not be the only problem)

Here is the current code:

    my $stderrCache = File::Spec->tmpdir() . '/' . $$ . '.stderr';

One solution is simply to make the name a bit more unique, and try again if it already exists:

    my $stderrCache;
    do {$stderrCache = File::Spec->tmpdir() . '/stderr-' . $$ . '-' . time . '.txt'} while (-e $stderrCache);

Perhaps the following would be better:

    my $stderrCache = new File::Temp( DIR => File::Spec->tmpdir(), UNLINK => 1 );

Which finds an arbitrary name and also opens it, plus deals with cleanup. This avoids the additional hazard later on when the file is used.

However, the subsequent use of this temporary file uses some tricky code to catch STDERR. For example. So use of the File::Temp() may need careful review.

            # Child - run the command
            untie(*STDERR);
            open( STDERR, '>', $stderrCache )
              || die "Can't redirect STDERR: '$!'";

            unless ( exec( $path, @args ) ) {
                syswrite( STDOUT, $key . ": $!\n" );
                exit($key);

I am not going to try to suggest changes to this without extensive testing.

I searched the source for other similar uses of File::Spec->tmpdir().

It appears that the use in FileCache is hazardous as well?? I did not test this but have not been able to get Caching to work yet, and this may be part of the reason.

# by default, the root of the cache is located in 'FileCache'.  On a
# UNIX system, this will appear in "/tmp/FileCache/"

my $DEFAULT_CACHE_ROOT = "FileCache";


# by default, the directories in the cache on the filesystem should
# be globally writable to allow for multiple users.  While this is a
# potential security concern, the actual cache entries are written
# with the user's umask, thus reducing the risk of cache poisoning

my $DEFAULT_DIRECTORY_UMASK = 000;

I haven't reviewed this in detail, but on the VPS server I use, each user has a separate user:group and when FileCache is created, current code will it allow multiple users to write into it (correct?) Would it be better to create a separate FileCache in the working directory of the foswiki system instead? That would be my suggestion instead of using this general /tmp directory and sharing of FileCache by all users.

-- RaymondLutz - 24 Feb 2011

I change this to target release 1.1.3, so it shows up on the release blockers. We can later decided if it's worth delaying 1.1.3 for a fix, but it will at least get noticed.

I agree that Foswiki should be able to control the location of temporary files, however I suspect the issue will be fairly wide reaching. Not that there is a deprecated configuration setting: $Foswiki::cfg{TempfileDir} that is used by some extensions. But it's not used by Foswiki core. Note in Foswiki.pm: " # Compatibility; not used except maybe in plugins"

It appears as though File::Spec->tempdir() does not check $ENV{TMPDIR} when taint checking is enabled, which it is for Foswiki. So there is no easy environment override available. Foswiki::Configure::UIs::CGISetup sets the ENV{TMPDIR} however it probably is not used due to taint checking.

Searching for references to File::Spec and File::Temp - the following modules appear to use temporary files.

  • Foswiki::Store::VC::Handler::mkTmpFilename
  • Foswiki::Sandbox::sysCommand() Cache to capture STDERR
  • Foswiki::Plugins::EmptyPlugin Contains examples of using File::Temp
  • Foswiki::Meta::attach() Temporary storage during attachment processing
  • Foswiki::Configure::Package Stores extension files fetched from the repository
  • Foswiki::Configure::Util Storage for expanded archive files.

Note that the Foswiki::Cache temporary file storage is explicitly set in the configuration. $Foswiki::cfg{Cache}{RootDir} = '$Foswiki::cfg{WorkingDir}/tmp/cache'; So it should not be creating files in /tmp unless the configuration has been changed from default.

-- GeorgeClark - 24 Feb 2011

The general fix is too extensive to address in this task. See ControlFoswikiTemporaryFileLocations for the feature proposal.

-- GeorgeClark - 25 Feb 2011
 

ItemTemplate edit

Summary Temporary File Hazard in Sandbox.pm and FileCache.pm
ReportedBy RaymondLutz
Codebase 1.1.2, 1.1.1, 1.1.0, trunk
SVN Range
AppliesTo Engine
Component Sandbox.pm
Priority Urgent
CurrentState Closed
WaitingFor
Checkins distro:629a25f30bff distro:83309ffaa8f0
TargetRelease patch
ReleasedIn 1.1.3
Topic revision: r7 - 16 Apr 2011, KennethLavrsen
The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License    Legal Imprint    Privacy Policy