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

[Martin Langhoff]

On Thu, Jun 26, 2008 at 8:44 PM, Michael Stone <michael at laptop.org> wrote:
> On Thu, Jun 26, 2008 at 07:10:18PM -0400, martin.langhoff at gmail.com wrote:
> 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?

Yes, no. The doctor's prescription has 2 params, and the only expected
caller is from incrond. Diminishing returns region...

>> +# 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?

Yes.

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

I thought about that, but in the end, we'll drop privs to the relevant
user, and the next steps will fail. Winning a race here does not lead
to priv escalation - and anyone sophisticated and dedicated enough to
play these games with us won't care about deleting their pal's data.

>> +# 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?

No, and we aren't catching errorcodes either. Low priority/diminishing
returns - I am selectively OC.

>> +# match with /library/users
>> +if not re.match(homebasepath, user[5]):
>> +    exit(1)
>
> I think your regex is incorrect - you want '^/library/users', right?

AIUI, match is implicitly left-anchored and search is what perl-heads
like me are used to. So no, I think my code is right, and it tests
right - if you can show me otherwise...

>> +# 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'.

minor nit, check that ~user begins with /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

As I noted in the comments in the code and in this email, we are
assuming the idenity of a student acct, and we'll perform 3 commands
with no user input: unlink, cp -al, ln -snf. Yes, someone could race
with us, but there is a cost in the complexity for the attacker, and I
can see no priv escalation path for her/him to make it worthwhile.

So it enters diminishing returns twilight for me.

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

There is still a race unless I flock it.

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

The rsync part is done, and is all client side. On the server side,
all we need is cp -aln -- see the patches that follow...

These reviews are fantastic. I didn't want to spend too much time
(yours or mine) on the racyness issue -- I only plan to close *all*
the foreseeable security gaps when my request of infinite time is
granted. In as much as I am still mortal :-) I am focusing on making
it hard enough to that this isn't an easy target, and moving on to the
next feature.

So as you probably guessed, it's a fully intentional tradeoff. If you
see a gaping whole in what I am doing (priv escalation), yes, I'd want
to know, but from what I see, we both see a race condition. As it
stands, with the correct perms in userdirs, the risk we have is that a
user can trick the unlink() to delete one of his/her own files.
Whoppee!

Our time is very constrained - AFAICS this ain't worth it.

cheers,



m
-- 
 martin at laptop.org -- School Server Architect
 - ask interesting questions
 - don't get distracted with shiny stuff - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff