Re: [PATCH 3/3] verify_path: consider dos drive prefix

From: Johannes Sixt <j6t@kdbg.org>
Date: May 27, 2011, 18:58 UTC
Message-ID: <4DDFF473.7030104@kdbg.org>
In-Reply-To: <1306512040-1468-4-git-send-email-kusmabite@gmail.com>

Am 27.05.2011 18:00, schrieb Erik Faye-Lund:

Show 7 quoted lines

> If someone manage to create a repo with a 'C:' entry in the
> root-tree, files can be written outside of the working-dir. This
> opens up a can-of-worms of exploits.
> 
> Fix it by explicitly checking for a dos drive prefix when verifying
> a paht. While we're at it, make sure that paths beginning with '\' is
> considered absolute as well.

I think we do agree that the only way to avoid the security breach is to check a path before it is used to write a file. In practice, it means to disallow paths in the top-most level of the index that are two characters long and are letter-colon.

IMHO, it is pointless to avoid that an evil path enters the repository, because there are so many and a few more ways to create an evil repository.

Show 11 quoted lines

> diff --git a/read-cache.c b/read-cache.c
> index f38471c..68faa51 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -753,11 +753,14 @@ int verify_path(const char *path)
>  {
>  	char c;
>  
> +	if (has_dos_drive_prefix(path))
> +		return 0;
> +

Isn't verify_path used to avoid that a bogus path enters the index? (I don't know, I'm not familiar with this infrastructure.)

Show 7 quoted lines

>  	goto inside;
>  	for (;;) {
>  		if (!c)
>  			return 1;
> -		if (c == '/') {
> +		if (is_dir_sep(c)) {
>  inside:

And if so, at this point, all backslashes should have been converted to forward-slashes already. If not, then this would just paper over the real bug.

>  			c = *path++;
>  			switch (c) {

-- Hannes