Bug 50148 - race condition in directory handling
race condition in directory handling
Status: CLOSED RAWHIDE
Product: Red Hat Linux
Classification: Retired
Component: tmpwatch (Show other bugs)
7.1
i386 Linux
medium Severity medium
: ---
: ---
Assigned To: Preston Brown
Brian Brock
: Security
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2001-07-27 06:36 EDT by martin.macok
Modified: 2007-04-18 12:35 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2001-08-01 14:19:11 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description martin.macok 2001-07-27 06:36:37 EDT
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
lstats().
 
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,
                    strerror(errno));
          }
          free(dir);
        }
        fchdir(dd);
        close(dd);
 
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
path.
 
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 06:38:37 EDT
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)
...
fchdir(dirfd);
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",
                dir,
                        strerror(errno));
                free(dir);
                continue;
              }
 
              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 06:43:52 EDT
You can find more in following discussion in
http://security-archive.merton.ox.ac.uk/security-audit-200107/
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 14:19:06 EDT
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':
http://www.uku.fi/~jhuuskon/tmpwatch/tmpwatch.c

(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 !

Cheers,
-Jarno

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
                messages)

- 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.


safe_chdir:
- 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
  inode/dev.

check_fuser:
- use relative pathname
- use FILENAME_MAX buffersize
Comment 4 Preston Brown 2001-08-06 12:58:32 EDT
These fixes are integrated into tmpwatch 2.8 and later.
Comment 5 martin.macok 2001-08-06 13:36:30 EDT
Do you plan to release updates/bugfixes for Red Hat 6.x/7.x?

Note You need to log in before you can comment on or make changes to this bug.