On Thu, Jun 26, 2008 at 8:44 PM, Michael Stone <[EMAIL PROTECTED]> wrote: > On Thu, Jun 26, 2008 at 07:10:18PM -0400, [EMAIL PROTECTED] 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 -- [EMAIL PROTECTED] -- School Server Architect - ask interesting questions - don't get distracted with shiny stuff - working code first - http://wiki.laptop.org/go/User:Martinlanghoff _______________________________________________ Server-devel mailing list Server-devel@lists.laptop.org http://lists.laptop.org/listinfo/server-devel
