[Server-devel] [PATCH] postprocess.py: an incrond-triggered script to cleanup file transfers

[Michael Stone]

On Thu, Jun 26, 2008 at 07:10:18PM -0400, martin.langhoff at gmail.com wrote:

> diff --git a/server/postprocess.py b/server/postprocess.py
> new file mode 100755
> index 0000000..82a2418
> --- /dev/null
> +++ b/server/postprocess.py
> +homebasepath = '/library/users'
> +dirpath      = sys.argv[1]
> +fname        = sys.argv[2]
> +fpath        = dirpath + '/' + fname

Are we content with the exceptions that might result from
postprocessing.py if run with fewer than two arguments? Do we ever
expect that postprocess.py will be run manually?

> +# Sanity checks:
> +# - must be a file
> +# - username must be ^\w+$
> +# - uid must match username

Do you mean that the file must be owned by the correct user?

> +# - must exist in /library/users
> +#
> +# Note: there are race conditions here.
> +#       We will drop privs before doing
> +#       potentially damanging stuff.

s/damanging/damaging

If we check the path name, then open the file, then fstat the resulting
descriptor, would we avoid the races?

> +# we'll hit a KeyError exception and exit 1
> +# if the user is not found
> +user  = pwd.getpwnam(fname)

Might be worth a remark at the beginning of the file that uncaught
exceptions are intended to kill the process. Do we ensure that the
resulting tracebacks are logged appropriately?

> +# match with /library/users
> +if not re.match(homebasepath, user[5]):
> +    exit(1)

I think your regex is incorrect - you want '^/library/users', right?
Also, python has builtin tests for this which are fast and which don't
require regexen. Consider:

  user[5].startswith(homebasepath) # what I think you want
  homebasepath in user[5]          # equivalent to what you have

> +# user uid must match file owner uid
> +if not (user[3] == os.stat(fpath)[4]):
> +    exit(1)

Correctness analysis:

You seem to have implemented: 

  1. Read <username>.
  2. stat64(fpath); check that fpath resolves to a file.
  3. lstat64(fpath); check that fpath resolves to a not-link.
  4. Look up the passwd data for <username>.
  5. Check fpath contains '/library/users'.
  6. stat64(fpath); check that fpath resolves to a file owned by <username>.
  7. Assume the identity of <username>.
  8. ...

This is not correct for two reasons, the most important being that we
have a classic TOCTTOU race - i.e. that any attacker with write access
to /library/users and with read access to a dirent pointing to an inode
owned by <username> can link() and rename() that inode into
/library/users in order to pass your check, then replace the resulting
dirent at any time before you re-use the path.

Now, as it happens, your present code merely unlinks fpath. Since
unlink() doesn't follow symlinks, why not just strengthen the conditions
on fpath to something like:

  "Check that fpath begins with /library/users/ and contains exactly 3
   slashes - i.e. that '/' not in fpath[len(prefix):]."

and then unlink fpath without any other conditions? 

If, for some reason, you still want to check the ownership on the inode
pointed to by fpath and you don't care about the race I described above,
then you might:

  1. Read <username>.
  2. Check that fpath begins with /library/users/ and contains exactly 3
     slashes - i.e. that '/' not in fpath[len(prefix):].
  3. Open fpath with O_NOFOLLOW.
  4. fstat64() the resulting descriptor. Verify from the stat data that
     the descriptor points to a file and that the file is owned by
     <username>'s uid.

Michael

P.S. - If you haven't already filled in the rsync logic alluded to by
your final comment, then poke me when you do so and we can attempt to
construct a correct guard for it.