Bug 50148

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

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.

Date: Wed, 1 Aug 2001 20:50:01 +0300
From: Jarno Huuskonen <Jarno.Huuskonen>
To: security-audit.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

These fixes are integrated into tmpwatch 2.8 and later.

Do you plan to release updates/bugfixes for Red Hat 6.x/7.x?