Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths

From: Jeff King <peff@peff.net>
Date: Oct 22, 2016, 03:26 UTC
Message-ID: <20161022032652.nfdj6mcy6mi6anx5@sigill.intra.peff.net>
In-Reply-To: <xmqqvawlblxi.fsf@gitster.mtv.corp.google.com>

On Fri, Oct 21, 2016 at 11:23:53AM -0700, Junio C Hamano wrote:

Show 23 quoted lines

> A request to "git://<site>/<string>", depending on <string>, results
> in "directory" given to path_ok() in a bit different forms.  Namely,
> connect.c::parse_connect_url() gives
> 
>     URL				directory
>     git://site/nouser.git	/nouser.git
>     git://site/~nouser.git	~nouser.git
> 
> by special casing "~user" syntax (in other words, a pathname that
> begins with a tilde _is_ special cased, and tilde is not considered
> a normal character that can be in a pathname).  Note the lack of
> leading slash in the second one.
> 
> Because that is how the shipped clients work, the daemon needs a bit
> of adjustment, because interpolation and base-path prefix codepaths
> wants to accept only paths that begin with a slash, and relies on
> the slash when interpolating it in the template or concatenating it
> to the base (e.g. roughly "sprintf(buf, "%s%s", base_path, dir)").
> 
> I came up with the following as a less invasive patch.  There no
> longer is the ase where "user-path not allowed" error is given,
> as treating tilde as just a normal pathname character even when it
> appears at the beginning is the whole point of this change.

Thanks for explaining this. It is rather gross, but I think your less-invasive patch is the best we could do given the client behavior. And it's more what I would have expected based on the original problem description.

> As I said already, I am not sure if this is a good change, though.

I also am on the fence. I'm not sure I understand a compelling reason to have a non-interpolated "~" in the repo path name. Sure, it's a limitation, but why does anybody care?

Show 8 quoted lines

> @@ -170,11 +171,11 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
>  		return NULL;
>  	}
>  
> +	if (!user_path && *dir == '~') {
> +		snprintf(tilde_path, PATH_MAX, "/%s", dir);
> +		dir = tilde_path;
> +	}

I know you are following existing convention in this function to use an unchecked snprintf(), but it makes me wonder what kind of mischief a client could get up to by silently truncating via snprintf. This function is, after all, supposed to be checking the quality of the incoming path.

xsnprintf() is probably too blunt a hammer, but:

  if (snprintf(rpath, PATH_MAX, ...) >= PATH_MAX) {
	logerror("path too long");
	return NULL;
  }

in various places may be appropriate.

-Peff