On Monday 05 March 2007 23:03, Jeff Dike wrote: > On Mon, Mar 05, 2007 at 09:49:02PM +0100, Paolo 'Blaisorblade' Giarrusso wrote: > > From: Paolo 'Blaisorblade' Giarrusso <[EMAIL PROTECTED]> > > > > Avoid accepting things like -o .., -o dir/../../dir2, -o dir/../.. . > > This may be considered useless, but YMMV. I consider that this has a > > limited security value, exactly like disabling module support (in many > > case it is useful). > > Two comments on this one: > > + return strstr(path, "/../") != NULL || > > + strcmp(path, "..") == 0 || > > + strncmp(path, "../", strlen("../")) == 0 || > > + str_ends_with(path, "/.."); > > Minor style point - I'd be happier with more parens: > > + return (strstr(path, "/../") != NULL) || > + (strcmp(path, "..") == 0) || > + (strncmp(path, "../", strlen("../")) == 0) || > + str_ends_with(path, "/..");
Hmm. Personally I prefer the earlier style, but I haven't the last word on this. > C gets operator precedence wrong in one or two cases, so I just put parens > any place it might matter. Indeed. For instance this patch is wrong, I did this once in a patch, and I saw it another time in current Ubuntu kernels: - a + b / 4 + a + b >> 2 This is instead needed: + a + (b >> 2) > Second, there is code in externfs which does the same thing without > parsing paths which you might consider instead. I must, indeed - your comment points out the symlink issue, which I didn't think of, and which makes my patch moot. > It sees whether the > requested directory is outside the jail by cd-ing to it and then > repeatedly cd .. until it either reaches / or the jail root. > > A copy is below for your reading pleasure. I gave a look, and it's nice. Except that maybe "escapes_jail" would be a clearer name (there's confusion about the subject of "escaping"). Also, what about concurrent UML threads caring about current directory? I know that without SMP/preemption we can't have this problem, but when I see this I count a future bug unless this is checked (I think I reason mathematically about correctness, even if not formally). And I haven't the time to check this - I think your version could be merged anyway, but I'm not sure. > Jeff -- Inform me of my mistakes, so I can add them to my list! Paolo Giarrusso, aka Blaisorblade http://www.user-mode-linux.org/~blaisorblade Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/