unsafe_symlink change (Re: CVS update: rsync)

[Dave Dykstra]

Sorry, another mistake on my part -- I didn't realize the strtok side
effect.  It doesn't look to me like your new code looks at the last
component of the path like the old one might have.  It would probably
be a good idea to put tests ending in "/..", for example "foo/../.."
and "foo/..", into testsuite/unsafe-byname.test.

I don't expect that the strdups are of significant overhead, but as long
as you've gone through the trouble to rewrite it I suppose we might as
well use it (assuming you have handled the last component ok).

- Dave

On Thu, Jan 16, 2003 at 05:34:25PM -0800, Wayne Davison wrote:
> On Thu, Jan 16, 2003 at 02:33:26PM -0600, Dave Dykstra wrote:
> > I think you could get rid of the calls to strdup() and the new local
> > variables and possibly do a couple casts inside the function instead.
> 
> You wouldn't want to carve up the const strings with strtok() (which
> adds nulls to the strings).  The function could be rewritten to scan the
> string for '/'s without using strtok(), which would be a more efficient
> way to go (eliminating the need for the strdups).
> 
> Here's a quick rewrite that I minimally tested.  See if you like it.
> 
> ...wayne..
> 
> ---8<------8<------8<------8<---cut here--->8------>8------>8------>8---
> Index: util.c
> --- util.c	15 Jan 2003 17:49:44 -0000	1.118
> +++ util.c	17 Jan 2003 01:26:36 -0000
> @@ -793,49 +793,38 @@
>   *
>   * @sa t_unsafe.c
>   **/
> -int unsafe_symlink(const char *dest_path, const char *src_path)
> +int unsafe_symlink(const char *dest, const char *src)
>  {
> -	char *tok, *src, *dest;
> +	const char *name, *slash;
>  	int depth = 0;
>  
>  	/* all absolute and null symlinks are unsafe */
> -	if (!dest_path || !*dest_path || *dest_path == '/') return 1;
> -
> -	src = strdup(src_path);
> -	if (!src) out_of_memory("unsafe_symlink");
> +	if (!dest || !*dest || *dest == '/') return 1;
>  
>  	/* find out what our safety margin is */
> -	for (tok=strtok(src,"/"); tok; tok=strtok(NULL,"/")) {
> -		if (strcmp(tok,"..") == 0) {
> +	for (name = src; (slash = strchr(name, '/')); name = slash+1) {
> +		if (strncmp(name, "../", 3) == 0) {
>  			depth=0;
> -		} else if (strcmp(tok,".") == 0) {
> +		} else if (strncmp(name, "./", 2) == 0) {
>  			/* nothing */
>  		} else {
>  			depth++;
>  		}
>  	}
> -	free(src);
> -
> -	/* drop by one to account for the filename portion */
> -	depth--;
> -
> -	dest = strdup(dest_path);
> -	if (!dest) out_of_memory("unsafe_symlink");
>  
> -	for (tok=strtok(dest,"/"); tok; tok=strtok(NULL,"/")) {
> -		if (strcmp(tok,"..") == 0) {
> -			depth--;
> -		} else if (strcmp(tok,".") == 0) {
> +	for (name = dest; (slash = strchr(name, '/')); name = slash+1) {
> +		if (strncmp(name, "../", 3) == 0) {
> +			/* if at any point we go outside the current directory
> +			   then stop - it is unsafe */
> +			if (--depth < 0)
> +				break;
> +		} else if (strncmp(name, "./", 2) == 0) {
>  			/* nothing */
>  		} else {
>  			depth++;
>  		}
> -		/* if at any point we go outside the current directory then
> -		   stop - it is unsafe */
> -		if (depth < 0) break;
>  	}
>  
> -	free(dest);
>  	return (depth < 0);
>  }
>  
> ---8<------8<------8<------8<---cut here--->8------>8------>8------>8---