Bug 50148

Summary: race condition in directory handling
Product: [Retired] Red Hat Linux Reporter: martin.macok
Component: tmpwatchAssignee: Preston Brown <pbrown>
Status: CLOSED RAWHIDE QA Contact: Brian Brock <bbrock>
Severity: medium Docs Contact:
Priority: medium    
Version: 7.1CC: jarno.huuskonen
Target Milestone: ---Keywords: Security
Target Release: ---   
Hardware: i386   
OS: Linux   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2001-08-01 18:19:11 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description martin.macok 2001-07-27 10:36:37 UTC
The problem was discovered by Pavel Kankovsky <peak@argo.troja.mff.cuni.cz>
and was discussed on LSAP (linux security audit project) mailing list.

Most of the work is done by the following function:
  int cleanupDirectory(char * dirname, unsigned int killTime, int flags)
cleanupDirectory() starts with safe_chdir(dirname). safe_chdir(x) does
lstat(x), chdir(x), and lstat(".") and compares the results of both
When cleanupDirectory() encounters a subdirectory, it executes the
following piece of code:
      int dd = open(".", O_RDONLY);
      if(dd != -1) {
        char *dir;
        dir = malloc(strlen(dirname) + strlen(ent->d_name) + 2);
        if(dir != NULL) {
          strcpy(dir, dirname);
          strcat(dir, "/");
          strcat(dir, ent->d_name);
          if(cleanupDirectory(dir, killTime, flags) == 0) {
            message(LOG_ERROR, "cleanup failed in %s: %s\n", dir,
i.e. it concatenates the path it has got as its first argument with the
name of the subdirectory and calls itself recursively on the resulting
I am afraid the weak point is the assumption the place specified by
dirname is the same as the current directory. If I let tmpwatch descend
into a deep hierarchy, and replace one of the parent directories with a
symlink (mv dir dir2; ln -s / dir), safe_chdir() will follow that symlink
without any complaints (assuming none of the syscalls failed) because the
symlink was not the last component of the path. This way one could
manipulate tmpwatch to leave the subtree it was supposed to work on and
wreak havoc in an arbitrary (writable) part of the filesystem.

Comment 1 martin.macok 2001-07-27 10:38:37 UTC
Jarno Huuskonen <Jarno.Huuskonen@uku.fi> replied:

What about changing the cleanupDirectory to use fchdir and all file
etc. operations use the fd or relative path ? I'm thinking about something
like this:
int cleanupDirectory(int dirfd, const char *dirname, unsigned int
   killTime, int flags, dev_t st_dev)
lstat(".", &here);
if (here.st_dev != st_dev) return 0;
if (S_ISDIR(sb.st_mode)) {
          int dd = open(".", O_RDONLY);
          if (dd != -1) {
            char *dir;
            dir = malloc(strlen(dirname) + strlen(ent->d_name) + 2);
            if ( dir != NULL ) {
              int entryfd;
              strcpy(dir, dirname);
              strcat(dir, "/");
              strcat(dir, ent->d_name);
              entryfd = open( ent->d_name, O_RDONLY | O_DIRECTORY |
                              O_NOFOLLOW );
              if ( entryfd == -1 ) {
                message(LOG_ERROR, "couldn't open directory %s: %s\n",
              if ( cleanupDirectory(entryfd, dir, killTime, flags,
                                    st_dev) == 0 )
I did some preliminary testing with this method and now tmpwatch didn't
appear to go 'crazy' on other directories.
It might be a nice feature to have some kind of resource limits: Limit
how deep the directory hierarchy can be, and log an error if this limit
is exceeded, memory/cpu limits etc.

Comment 2 martin.macok 2001-07-27 10:43:52 UTC
You can find more in following discussion in
see thread "Race condition in recent tmpwatch (2.7.1)?"

There is also another not so dangerous bug in tmpwatch that malicious
"lost+found" directories are skipped by tmpwatch - discussed in same thread.

Comment 3 martin.macok 2001-08-01 18:19:06 UTC
Date: Wed, 1 Aug 2001 20:50:01 +0300
From: Jarno Huuskonen <Jarno.Huuskonen@uku.fi>
To: security-audit@ferret.lmh.ox.ac.uk
Subject: Re: Race condition in recent tmpwatch (2.7.1)?
I made the changes to tmpwatch-2.7.4 that Pavel Kankovsky suggested:
Use relative pathname when doing file operations (use full pathname for
error messages) and check that "lost+found" directory is owned by root
before skipping it.

I've put the modified tmpwatch to my 'homepage':

(I made a bunch of formatting changes so the code is easier for me to
read, so I'm not putting up a patch ...).

I would appreciate if somebody could take the time to review/audit the
code to see if it contains any (more) 'rookie' mistakes --> so don't use
the code in production before that !


PS. Here's a short list of changes against tmpwatch-2.7.4
cleanupDirectory takes extra parameters:
- fulldirname = full pathname to the directory (used when printing error

- reldirname  = directory w/out path used for all dir/file operations

- st_dev      = device of the temporary directory (eg. /tmp) this is
                checked so cleanupDirectory will not cross filesystems

- st_ino      = inode of rel/fulldirname directory. This is checked in
                safe_chdir to make sure that the directory hasn't
                changed between lstat->cleanupDirectory / safe_chdir

- Check that lost+found directory is owned by root.

- use relative pathname in chdir. Change order of device / inode check
(from Pavel K.)
- make sure that inode / dev of reldirname are the same as the parameter

- use relative pathname
- use FILENAME_MAX buffersize

Comment 4 Preston Brown 2001-08-06 16:58:32 UTC
These fixes are integrated into tmpwatch 2.8 and later.

Comment 5 martin.macok 2001-08-06 17:36:30 UTC
Do you plan to release updates/bugfixes for Red Hat 6.x/7.x?