Re: [uml-devel] [PATCH 04/11] uml - hostfs: avoid possible escapes from hostfs=.

[Blaisorblade]

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 <blaisorblade@xxxxxxxx>
> >
> > 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/